Re: [lmi] An irreproducible anomaly

[Greg Chicares]

On 2016-06-16 01:08, Vadim Zeitlin wrote:
> On Tue, 14 Jun 2016 21:50:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> I only saw it once myself, when I was trying to reproduce it while talking
> GC> with Kim on the telephone. I didn't have lmi open at all, so I started a
> GC> single instance just to see whether I could reproduce the anomaly. I ran
> GC> through a broad variety of steps before I saw anything weird--e.g.:
> GC>  - editing the field in-place in the census manager
> GC>  - pasting into it

By "it", I intended "the field", not "the census manager": Shift-Ins,
as opposed to "Census | Paste census". I didn't notice the ambiguity.

>  On a completely separate note, when doing this with some random text
> (which turned out to be a part of another email), I got the following
> message box:
> 
>       ---------------------------
>       Error
>       ---------------------------
>       Symbol table for class class Input ascribes no member named 'VZ> '.
>       ---------------------------
>       OK   
>       ---------------------------
> 
> This is true, as far as it goes, but I'd suggest that saying "Clipboard
> doesn't contain valid census data" could be less incomprehensible to the
> end users...

It's an eternal challenge. We can write really specific error messages
that explain exactly what went wrong and help us diagnose and fix the
problem, but end users cannot understand them. Or we can easily write
vague but "friendly" error messages that don't help anyone because they
all boil down to "That didn't work--try again". Or, at considerable cost,
we can try to intercept messages from the "plumbing" and recast them in
"porcelain" language.

Here's an example that I dealt with today:
  git show 553d619341074333f6550376e33b4d3f69a4eac0
where wx was telling us:
  Assertion '!path.empty()' failed
  (using empty path with wxDOC_SILENT doesn't make sense).
  [file ../src/common/docview.cpp, line 1478]
Here's my first attempt (not what I committed):

-    doc_manager_->CreateDocument
-        (configurable_settings::instance().default_input_filename()
-        ,wxDOC_SILENT
-        );
+    std::string f = configurable_settings::instance().default_input_filename();

+    validate_filepath(f, "Default input file");

+    doc_manager_->CreateDocument(f, wxDOC_SILENT);


That's more economical than what I actually committed, but not as good.
If default_input_filename() is an empty string, it would display:
  "Default input file must not be empty."
which sounds like the file exists but contains nothing. I might have
made it say
  "Default input file '' must not be empty."
but that "''" looks like a hiccup rather than information. In another
failure situation,
  "Default input file '/opt/lmi' is a directory."
is confusing: being a directory, it *cannot* be a default input file,
but the error message seems to say that it is. And the end user may
not care why it's invalid (zero-character filename; impermissible
filename; nonexistent file; directory); it's more important to explain
how to fix the problem, which is what I tried to do in commit 553d619.

Apparently at some time in the past I hoped that a general routine
like validate_filepath() could be widely used to validate filepaths,
but I don't think that's actually possible. But trapping every possible
error and writing a really good message for each one would take months.

Returning to your example:
>       Symbol table for class class Input ascribes no member named 'VZ> '.
CensusView::UponPasteCensus() already seems to trap eight different
kinds of invalid input. I'm not convinced that trapping a ninth would
go to the root of the complex of problems this routine poses. Yet if
you feel highly motivated to write and test a patch like (untested)

+        std::vector<std::string> const& 
all_headers(case_parms()[0].member_names());
         while(std::getline(iss_line, token, '\t'))
             {
+            if(!contains(all_headers, token))
                 {
                 fatal_error() << "Name '" << token << "' not recognized." << 
LMI_FLUSH
                 }
             headers.push_back(token);
             }

then I'd be willing to apply it.

> GC>  - copying from it and pasting back in repeatedly
> GC>  - toggling "Census | {Fixed,Variable} column width"
> GC>  - resizing the field with the mouse
> GC> and then popping up the tabbed dialog by
> GC>  - right-clicking the same cell and choosing "Edit cell..."
> GC>  - right-clicking a different cell and choosing "Edit cell..."
> GC>  - clicking the magnifying-glass button on the toolbar
> GC>  - equivalent menu commands with the mouse, with alt-key accelerators,
> GC>      and with Ctrl-Shift-Whatnot
> GC> but I've repeated all those things numerous times and have seen the
> GC> anomaly only once.
> 
>  I probably don't have any better hope to reproduce the bug than you do, so
> I didn't try redoing all of the above, so instead I tried modifying
> wxWidgets sources to, first, avoid dismissing the editor while showing the
> dialog and, then, to call it from inside wxYield(). Unfortunately I still
> couldn't see the problems you described, so it looks like the only theory I
> had was wrong and I really don't know what else to do.
> 
> GC> If you can think of some extra sanity check that we might build in,
> 
>  My only remaining idea is that, somehow, wxTheApp().IsActive() is always
> false in MvcContoller::UponUpdateUI(). But I really don't see how could
> this happen nor what could we do to check for it.

I tried this, which ought to show a primitive spinner if IsActive() is
false; it would make this hypothetical circumstance obvious if it ever
arises, but the problem is that we just don't know how to make the
anomaly reproducible:

diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index aa5923a..e2bd413 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -796,6 +796,9 @@ void MvcController::UponUpdateUI(wxUpdateUIEvent& event)
     // for a discussion.
     if(!TheApp().IsActive())
         {
+        static unsigned int j = 0;
+        char const*const c = "|\\-/";
+        status() << c[++j % 4] << std::flush;
         return;
         }

> The best would, actually,
> be just to remove this check anyhow, but this would require changes to
> focus management, as discussed in the link in the comment just before this
> check, and I'm not sure you're willing to do this right now...

No way.

I think we simply have to put this aside until someone finds a
way to reproduce it reliably.