Re: [lmi] 'wine' anomalies

[Greg Chicares]

On 2017-11-15 21:30, Vadim Zeitlin wrote:
> On Tue, 14 Nov 2017 16:13:12 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> BTW, the icon on this dialog is a wine bottle, whereas other lmi
> GC> windows have lmi icons.
> 
>  Well, at least I can fix this one:

Yes, as I reported yesterday, this works: it shows the lmi icon
on lmi's tabbed input dialog (though not on any messagebox, as
can be seen with various items on the 'Test' menu). Because it
makes my 'wine' world a little less strange, I'd like to apply
it, but in a slightly different way. Consider this distillation
of the patch:

>  #include "contains.hpp"
> +#include "data_directory.hpp"
>  #include "global_settings.hpp"
...
>  #include <wx/datectrl.h>
> +#include <wx/iconbndl.h>
>  #include <wx/radiobox.h>
...
> @@ -111,6 +113,8 @@ MvcController::MvcController
> +    SetIcons(wxIconBundle(AddDataDir("lmi.ico"), wxBITMAP_TYPE_ICO));

Writing a SetIcons() call that's virtually identical to this one elsewhere:

void Skeleton::InitIcon()
{
    frame_->SetIcons(wxIconBundle(AddDataDir("lmi.ico"), wxBITMAP_TYPE_ICO));
}

makes me wonder if we can write that once and only once. I also wonder
whether we can avoid including two more headers, which slightly erodes
the separation of concerns. I can do that this way:

    SetIcons(dynamic_cast<wxTopLevelWindow&>(TopWindow()).GetIcons());

which does work, and seems very safe because anything called 'frame_'
should derive from class wxTopLevelWindow. But that's unsatisfying:
it addresses my slight but real objections to the original patch, but
introduces a dynamic_cast which is objectionable in its own way (I'd
object to it less if it were inside a reusable "library-type" routine.)

But that leads me to ask: why does wxApp::GetTopWindow() return a
wxWindow* rather than a wxTopLevelWindow*? Are there really two
distinct "Top Window" and "Top Level Window" concepts? or is "Level"
just elided from GetTopWindow() because it's easier to type? I'm
guessing that it's the latter.

Now, maybe there's some unusual case where the concepts are distinct,
just as GetFrame() doesn't necessarily return a wxFrame:

http://docs.wxwidgets.org/3.0/classwx_view.html#a26d5bbf8f8dc95eb390a45f4ecc210ac
| Note that this "frame" is not a wxFrame at all in the generic MDI
| implementation which uses notebook pages instead of frames and this
| is why this method returns a wxWindow and not a wxFrame.

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

/// Safe cover function for wxApp::GetTopWindow(): throws if null.
wxWindow& TopWindow() {
    wxWindow* w = TheApp().GetTopWindow();
    [...dereference if no null...]

and, if we make that lmi function return a wxTopLevelWindow&,
isn't that a small but positive step toward better use of types?

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

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index 528f9b95..98ee34c9 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -111,6 +111,8 @@ MvcController::MvcController
         alarum() << "Unable to load dialog." << LMI_FLUSH;
         }
 
+    SetIcons(TopWindow().GetIcons());
+
     
BookControl().ChangeSelection(last_selected_page[view_.ResourceFileName()]);
 
     // This assignment must follow the call to LoadDialog().
diff --git a/wx_utility.cpp b/wx_utility.cpp
index 626ec5aa..47fd50e0 100644
--- a/wx_utility.cpp
+++ b/wx_utility.cpp
@@ -280,8 +280,12 @@ wxApp& TheApp()
 }
 
 /// Safe cover function for wxApp::GetTopWindow(): throws if null.
+///
+/// If GetTopWindow() returns null, then wx is probably starting up
+/// or shutting down; therefore, diagnostics are displayed through
+/// mechanisms that should work even in such circumstances.
 
-wxWindow& TopWindow()
+wxTopLevelWindow& TopWindow()
 {
     wxWindow* w = TheApp().GetTopWindow();
     if(!w)
@@ -289,7 +293,13 @@ wxWindow& TopWindow()
         safely_show_message("Top window not found.");
         throw 0;
         }
-    return *w;
+    wxTopLevelWindow* t = dynamic_cast<wxTopLevelWindow*>(w);
+    if(!t)
+        {
+        safely_show_message("Top window is not a wxTopLevelWindow.");
+        throw 0;
+        }
+    return *t;
 }
 
 /// Convert a filename to an NTBS std::string, throwing upon failure.
diff --git a/wx_utility.hpp b/wx_utility.hpp
index 9f36529b..7e8ccf67 100644
--- a/wx_utility.hpp
+++ b/wx_utility.hpp
@@ -28,6 +28,7 @@
 
 #include <wx/event.h>
 #include <wx/string.h>
+#include <wx/toplevel.h>
 
 #include <stdexcept>
 #include <string>
@@ -135,7 +136,7 @@ std::vector<wxWindow*> Lineage(wxWindow const*);
 std::string NameLabelId(wxWindow const*);
 
 wxApp& TheApp();
-wxWindow& TopWindow();
+wxTopLevelWindow& TopWindow();
 
 std::string ValidateAndConvertFilename(wxString const&);
 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------