Re: [lmi] 'wine' anomalies

[Vadim Zeitlin]

On Wed, 7 Mar 2018 17:28:22 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-11-15 21:30, Vadim Zeitlin wrote:
GC> > On Tue, 14 Nov 2017 16:13:12 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC> BTW, the icon on this dialog is a wine bottle, whereas other lmi
GC> > GC> windows have lmi icons.
GC> > 
GC> >  Well, at least I can fix this one:
GC> 
GC> Yes, as I reported yesterday, this works: it shows the lmi icon
GC> on lmi's tabbed input dialog (though not on any messagebox, as
GC> can be seen with various items on the 'Test' menu). Because it
GC> makes my 'wine' world a little less strange, I'd like to apply
GC> it, but in a slightly different way. Consider this distillation
GC> of the patch:
GC> 
GC> >  #include "contains.hpp"
GC> > +#include "data_directory.hpp"
GC> >  #include "global_settings.hpp"
GC> ...
GC> >  #include <wx/datectrl.h>
GC> > +#include <wx/iconbndl.h>
GC> >  #include <wx/radiobox.h>
GC> ...
GC> > @@ -111,6 +113,8 @@ MvcController::MvcController
GC> > +    SetIcons(wxIconBundle(AddDataDir("lmi.ico"), wxBITMAP_TYPE_ICO));
GC> 
GC> Writing a SetIcons() call that's virtually identical to this one elsewhere:
GC> 
GC> void Skeleton::InitIcon()
GC> {
GC>     frame_->SetIcons(wxIconBundle(AddDataDir("lmi.ico"), 
wxBITMAP_TYPE_ICO));
GC> }
GC> 
GC> makes me wonder if we can write that once and only once.

 We can, but I would do it just by having some function taking
wxTopLevelWindow called InitFrame() or maybe just SetUp() in Skeleton and
then calling it for both the main frame and the MvcController dialog. This
seems to be a simple, C-like solution that you should be in favour of,
doesn't it? Moreover, such function could be useful for doing other things
later, e.g. I'd really like lmi main (and why not the other) window(s) to
remember its geometry automatically, as is common, because otherwise I need
to constantly move it to another monitor when debugging to make it more
comfortable -- and this could be done here as well.

 An alternative could be to use CRTP to define some LmiTLW<> class which
would call SetIcons() and then use LmiTLW<wxDocMDIParentFrame> in
skeleton.cpp and derive MvcController from LmiTLW<wxDialog>, but I don't
think the extra complexity is warranted here if we're only going to use it
for setting the icons.

GC> I also wonder whether we can avoid including two more headers, which
GC> slightly erodes the separation of concerns. I can do that this way:
GC> 
GC>     SetIcons(dynamic_cast<wxTopLevelWindow&>(TopWindow()).GetIcons());
GC> 
GC> which does work,

 Yes, but it's strangely indirect. I'd prefer to just set the icon(s) for
both frames rather than setting them for one of them and then copying it to
the other one.

GC> But that leads me to ask: why does wxApp::GetTopWindow() return a
GC> wxWindow* rather than a wxTopLevelWindow*?

 The simple answer is that wxApp::GetTopWindow() predates wxTopLevelWindow
existence (previously there were wxFrame, wxDialog and a couple of other
strange things that didn't have a common base class beyond wxWindow -- of
course, by "previously" I mean a very long time ago, wxTLW was added back
in 2001).

GC> and, even if there's no deep reason like that, it's hard to justify
GC> changing GetTopWindow()'s return type in wxWidgets just so that one
GC> person doesn't have to see a wine-bottle icon on one dialog.

 Yes, especially because we also have SetTopWindow() currently taking just
"wxWindow*" and we wouldn't want to change its signature to avoid breaking
the existing code and so we'd have to do our own dynamic cast in
GetTopWindow() anyhow.

GC> /// Safe cover function for wxApp::GetTopWindow(): throws if null.
GC> wxWindow& TopWindow() {
GC>     wxWindow* w = TheApp().GetTopWindow();
GC>     [...dereference if no null...]
GC> 
GC> and, if we make that lmi function return a wxTopLevelWindow&,
GC> isn't that a small but positive step toward better use of types?

 Yes, I think this is reasonable. Unlike any weird platforms that may need
to be supported by wxWidgets (or, more likely, could have needed to be
supported in the past), we definitely know that the top lmi window is a
wxFrame and hence a wxTLW, so this should be perfectly safe.

GC> In that vein, is there any reason not to apply the following change?

 I'd still prefer to add a helper Skeleton::SetUp(wxTopLevelWindow&)
function instead, but I don't see anything really wrong with this patch
neither, except for one tiny thing:

GC> diff --git a/wx_utility.hpp b/wx_utility.hpp
GC> index 9f36529b..7e8ccf67 100644
GC> --- a/wx_utility.hpp
GC> +++ b/wx_utility.hpp
GC> @@ -28,6 +28,7 @@
GC>  
GC>  #include <wx/event.h>
GC>  #include <wx/string.h>
GC> +#include <wx/toplevel.h>
GC>  
GC>  #include <stdexcept>
GC>  #include <string>
GC> @@ -135,7 +136,7 @@ std::vector<wxWindow*> Lineage(wxWindow const*);
GC>  std::string NameLabelId(wxWindow const*);
GC>  
GC>  wxApp& TheApp();
GC> -wxWindow& TopWindow();
GC> +wxTopLevelWindow& TopWindow();
GC>  
GC>  std::string ValidateAndConvertFilename(wxString const&);

 Just a forward declaration of wxTopLevelWindow would do here, so I'd
rather avoid including wx/toplevel.h header.

 Regards,
VZ