Re: [lmi] UpdateUI problems in main_wx.cpp

[Greg Chicares]

On 2008-08-07 19:32Z, Vadim Zeitlin wrote:
> 
>  This is another not really urgent thing but I just got very tired of
> looking at the UI updating-related mess in main_wx.cpp (plenty of "WX !!"
> comments and commented out code) and so decided to fix the problem. And

That's excellent news. Imagine someone without your knowledge of wx
attempting to maintain this code as it stands today in HEAD.

> unless I'm missing something, this is surprisingly simple. Let me explain
> my understanding of what the problem was and how it can be fixed and please
> correct me if I'm wrong:

You're correct. When I wrote this, I didn't know what Skip() was for.
I experienced an epiphany approximately here:
  http://lists.nongnu.org/archive/html/lmi/2005-11/msg00044.html
(thank you), resolved many of the issues then, and marked others for
later research as in the present case:

> -// TODO ?? Should this call Skip()?
> -
> -void Skeleton::UponMenuOpen(wxMenuEvent&)
> +void Skeleton::UponMenuOpen(wxMenuEvent& event)

And now it's important, even though not urgent, to work through the
978 remaining '??' markers like that (down from 1207 on 2008-01-01).

>  So the solution is to allow the base EVT_MENU_OPEN() handler to run and
> then all the other workarounds become unnecessary.

IIRC, you once taught university mathematics. If a student wrote
    2 pi r
      pi r^2
  3/2 pi r^3
you could say "no, 4/3". But the best students would try to learn
where they went wrong, and pose questions to themselves like
  what's the hypervolume of a 4-sphere?
  an N-sphere?
That is why--instead of applying your patch--I studied it yesterday,
then put it aside and strove to recreate it myself today. Here are
my "laboratory notes" that begin by writing the test first:

A six-fold test:
 - freshly load lmi
   (1) toolbar   "Save"   disabled before any document opened
 - open two documents; modify one
   (2) menu "File | Save" disabled when unmodified document focused
   (3) toolbar   "Save"   disabled when unmodified document focused
   (4) menu "File | Save"  enabled when   modified document focused
   (5) toolbar   "Save"    enabled when   modified document focused
 - close both documents, leaving none open
   (6) toolbar   "Save"   disabled after all documents closed
("File | Save" menuitem doesn't exist when no document open.)
Test outcome is an ordered boolean sextuple: 0=right, 1=wrong.

  111111 outcome before changing anything (e.g., 20080822T0019Z HEAD)

Observable behavior of HEAD seems correct. The only problem is the
unspeakable bletcherousness of the code. Let's first block all ugly
code, then reconstruct it cleanly.

  111111 outcome if we comment out these two lines:
    EVT_MENU_OPEN(                                     Skeleton::UponMenuOpen   
                  )
    EVT_UPDATE_UI(wxID_SAVE                           
,Skeleton::UponUpdateFileSave               )

All six tests pass. However, "Window | Next" is not grayed when only
one document is open: that requires reactivating UponMenuOpen().

  101111 outcome if we comment out only this line:
    EVT_UPDATE_UI(wxID_SAVE                           
,Skeleton::UponUpdateFileSave               )
  and the part of UponMenuOpen() that follows this line:
        // Needed for (xrc) menu enablement: a

The problem now, as Vadim diagnosed, is that Skip() isn't called in
UponMenuOpen().

  111111 outcome if we comment out this line:
    EVT_UPDATE_UI(wxID_SAVE                           
,Skeleton::UponUpdateFileSave               )
  and the part of UponMenuOpen() that follows this line:
        // Needed for (xrc) menu enablement: a
  and furthermore call Skip() upon entry in UponMenuOpen()

Same outcome if we completely remove UponUpdateFileSave().
["laboratory notes" end]

So I conclude that we needn't keep any
  EVT_UPDATE_UI(wxID_SAVE...
handler. But a good student would ask "Can I see it at a glance?";
the reason is found in wx's own 'src/common/docview.cpp':

  void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
  {
      wxDocument *doc = GetCurrentDocument();
      event.Enable( doc && doc->IsModified() );
  }

which we needn't duplicate.

Furthermore, all of this seems to be independent of your patch here:
  http://savannah.nongnu.org/bugs/?17757
so I'll address that separately. And I'll reply to the remainder of
your current message separately, too. But I plan not to change HEAD
for about one week, lest I destabilize the 2008-09-01 release.