Re: [lmi] Error reporting

[Vadim Zeitlin]

On Fri, 05 Dec 2014 18:51:43 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-12-05 17:04Z, Vadim Zeitlin wrote:
...
GC> >  So for me the behaviour pre-r6007, which would have shown the exact error
GC> > to the user, was preferable (although not ideal because it would actually
GC> > show two message boxes and just one would be better).
GC> 
GC> I had seen it as an improvement. Before r6007, two messages appeared
GC> in the GUI <...> After r6007, end users see only the first message

 Yes, this is what motivated my "not ideal" parenthesis above. To fix this,
i.e. provide the full information about the real reason for the error while
still showing just a single message box, the simplest solution would be to
use wxLogError() in system_command_wx.cpp like this:
---------------------------------- >8 --------------------------------------
diff --git a/skeleton.cpp b/skeleton.cpp
index 38f208c..eac5cca 100644
--- a/skeleton.cpp
+++ b/skeleton.cpp
@@ -635,11 +635,6 @@ bool Skeleton::OnInit()
 {
     try
         {
-        if(!wxIsDebuggerRunning())
-            {
-            wxLog::SetActiveTarget(new wxLogStderr);
-            }
-
         if(false == ProcessCommandLine(argc, argv))
             {
             return false;
diff --git a/system_command_wx.cpp b/system_command_wx.cpp
index a2a9a5a..f7c04fa 100644
--- a/system_command_wx.cpp
+++ b/system_command_wx.cpp
@@ -107,12 +107,7 @@ void concrete_system_command(std::string const& 
command_line)
         }
     else if(-1L == exit_code)
         {
-        fatal_error()
-            << "Command '"
-            << command_line
-            << "' not recognized."
-            << std::flush
-            ;
+        wxLogError("Command '%s' not recognized.", command_line);
         }
     else
         {
---------------------------------- >8 --------------------------------------

 This patch shouldn't be applied, of course, as it results in not showing
the debug messages on the console neither, but this can allow you to
quickly test whether this solution is acceptable to you.

 If it isn't, i.e. if you don't want to use the standard wxLogGui error
dialog, then we need to redirect wxLog output in some other way. It could
be done either locally, using e.g. this experimental/unfinished patch:
---------------------------------- >8 --------------------------------------
diff --git a/system_command_wx.cpp b/system_command_wx.cpp
index a2a9a5a..1f307c6 100644
--- a/system_command_wx.cpp
+++ b/system_command_wx.cpp
@@ -95,12 +95,19 @@ void concrete_system_command(std::string const& 
command_line)
         ;
     std::ostream& statusbar_if_available = b ? status() : null_stream();
 
+    struct silent_log_buffer : wxLogBuffer
+    {
+        virtual void Flush() {}
+    } log_buffer;
+    wxLog* const old_log = wxLog::SetActiveTarget(&log_buffer);
+
     statusbar_if_available << "Running..." << std::flush;
     wxArrayString output;
     wxArrayString errors;
     long int exit_code = wxExecute(command_line, output, errors, 
wxEXEC_NODISABLE);
     statusbar_if_available << timer.stop().elapsed_msec_str() << std::flush;
 
+    wxLog::SetActiveTarget(old_log);
     if(0L == exit_code)
         {
         return;
@@ -110,7 +117,8 @@ void concrete_system_command(std::string const& 
command_line)
         fatal_error()
             << "Command '"
             << command_line
-            << "' not recognized."
+            << "' not recognized: "
+            << log_buffer.GetBuffer()
             << std::flush
             ;
         }
---------------------------------- >8 --------------------------------------

which would result in showing a message box like this:

        ---------------------------
        Error
        ---------------------------
        Command 'foo' not recognized: 20:09:25: Error: Execution of command 
'foo' failed (error 2: the system cannot find the file specified.)

        ---------------------------
        OK   
        ---------------------------

(you can see why I called it unfinished: at the very least, we need to
change the formatting to remove the extra timestamp and "Error:", but this
is easy to do and this just illustrates how it could work).

 Or it could be done globally by defining a custom log target which would
show the errors in some other way than using a message box, e.g. in the
status bar.

 Personally I think that just using wxLogError() instead of fatal_error()
is the simplest and the most user-friendly solution. And AFAICS it keeps
the separation between GUI and non-GUI code, as this wx-specific function
is only used from wx-specific system_command_wx.cpp file. So I'd just do
this, but please let me know what do you prefer.


[...snip...]
GC> Do I understand this correctly now?

 Yes, absolutely, this is exactly it.

GC> > GC> Do we need a different test? And is there a way to automate a test for
GC> > GC> whatever GUI phenomena would be correct?
GC> > 
GC> >  Yes, there is, we can check that the expected dialog is shown or, 
simpler,
GC> > that the expected error message is logged.
GC> 
GC> I just spent a lot more time trying to understand that problem
GC> than I did writing about it. Could I prevail upon you to write
GC> an automated test for this? I guess this calls for a new
GC> 'wx_test_whatever.cpp' files, but that's certainly okay; or
GC> perhaps it fits nicely into some other file you were already
GC> going to add--it's your choice.

 I'm not sure how should this interact (if it should interact at all) with
the discussion about getting rid of the "Test" menu items completely and
replacing them with unattended tests. AFAIK no final decision was taken
about this (please correct me if I missed it somehow), but it seems a bit
strange to add tests for the commands which may disappear completely very
soon anyhow. Could you please clarify the plans for the "Test" menu before
I start doing it?


GC> > GC> If your analysis of the behavior is correct, then I suppose I should
GC> > GC> want to fix it. But that's still hypothetical for me, because I can't
GC> > GC> see what's wrong.
GC> > 
GC> >  Not showing wxWidgets errors to the users is wrong IMNSHO. And it
GC> > definitely was not done before.
GC> 
GC> I will gladly commit a patch that fixes this and a test that proves it.

 Patch returning to the old behaviour and just making the debug messages
visible is simple enough: we just need to define our own log target that
sends debug log messages to stderr (I assume here it is really more
appropriate than stdout, isn't it?) while handling the rest of them in the
usual way.

 Patch fixing the problem of two message boxes shown in a row if the
command being executed doesn't exist depends on which solution to this
problem do you prefer.

 And the test is also slightly trickier as it depends on the answer to the
question about the "Test" menu items above.

 So I'll make the first one a.s.a.p. but will need to wait for your answers
for the last two,
VZ