Re[2]: [lmi] UpdateUI problems in main_wx.cpp

[Vadim Zeitlin]

On Sat, 23 Aug 2008 02:54:01 +0000 Greg Chicares <address@hidden> wrote:

[analysis/testing snipped -- but read and understood]
GC> So I conclude that we needn't keep any
GC>   EVT_UPDATE_UI(wxID_SAVE...
GC> handler. But a good student would ask "Can I see it at a glance?";
GC> the reason is found in wx's own 'src/common/docview.cpp':
GC> 
GC>   void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
GC>   {
GC>       wxDocument *doc = GetCurrentDocument();
GC>       event.Enable( doc && doc->IsModified() );
GC>   }
GC> 
GC> which we needn't duplicate.

 Indeed, this is one of the nice things docview framework does
automatically.

GC> Furthermore, all of this seems to be independent of your patch here:
GC>   http://savannah.nongnu.org/bugs/?17757

 Yes, that patch is completely orthogonal. It does however also illustrate
an important idea which it took me some time to realize so it might be
useful if I explained it here:

 Quite often you write an event handler which relies on something being
initialized. In the simplest case you may have something like this:

        class MyPanel : public wxPanel { ... };

        BEGIN_EVENT_TABLE(MyPanel, wxPanel)
                EVT_SIZE(MyPanel::OnSize)
        END_EVENT_TABLE()

        MyPanel::MyPanel(wxWindow *parent)
               : wxPanel(parent)        
        {
                m_child1 = new wxWindow(this, ...);
                m_child2 = new wxWindow(this, ...);     
        }

        void MyPanel::OnSize(wxSizeEvent&)
        {
                const wxSize size = GetClientSize();
                m_child1->SetSize(0, 0, size.x, size.y/2);
                m_child2->SetSize(0, size.y/2, size.x, size.y/2);
        }

Putting aside small problems (like the pixel lost when the vertical size is
odd), this code has one serious bug: it crashes during run-time. This
happens because the base wxPanel class ctor emits, at least in some
wxWidgets ports, an wxEVT_SIZE event and so MyPanel::OnSize() is called
before m_child[12] were initialized and attempt to dereference garbage
values in them results in a crash.

 The usual and obvious way to correct this is to do the following:

        MyPanel::MyPanel(wxWindow *parent)
                : wxPanel() // use default ctor now
        {
                // we can't create the children before creating this window
                // (which is done by the base class Create()) but we can at
                // least initialize them
                m_child1 = m_child2 = NULL;

                Create(parent);

                m_child1 = new wxWindow(this, ...);
                m_child2 = new wxWindow(this, ...);
        }

        void MyPanel::OnSize(wxSizeEvent& event)
        {
                if ( !m_child1 ) {
                        // we're called too early, children not created yet
                        event.Skip();
                        return;
                }

                ... as before ...
        }

This is however not ideal because:

- Using non-default ctor is often more convenient than using the default
  one and then using Create()
- You have to write additional code in the event handler which has nothing
  to do with its main task (and usually you have several handlers so, to
  add insult to injury, you also need to duplicate it)
- You have to initialize m_child[12] twice which adds more unnecessary code


 So instead of the approach I now advise wx users to simply not use
EVT_SIZE() in the event table and add a call to Connect(wxEVT_SIZE,
wxSizeEventHandler(MyPanel::OnSize)) to MyPanel ctor after the children are
created. With this small change the original code becomes safe, it adds
much less extraneous code than the version above and it also feels more
natural and less artificial: we simply say that we need to position the
children only after we create them, which makes sense.

 To wrap it up, the patch mentioned above is an application of this idea to
this particular case but it might be useful elsewhere in LMI as well.
Basically anywhere where some condition is checked at the beginning of an
event handler before it does anything, you can also check this condition
somewhere else and only Connect() the handler when it's true (and sometimes
also Disconnect() it when the condition becomes false).

GC> But I plan not to change HEAD for about one week, lest I destabilize
GC> the 2008-09-01 release.

 Understood.

 Thanks for looking at this!
VZ