[lmi] PreviewFrameEx questions

[Vadim Zeitlin]

 Hello,

 This is hardly the most urgent of questions, but I was looking at some
other "WX !!" comments I could fix in the (still...) upcoming wx upgrade
and there is not one but two of them in this class code:

 First there is this one at previewframe_ex.hpp:61

    // WX !! Should OnCloseWindow() be virtual in the base class?

The answer to this question is simple enough: "no, it shouldn't be",
because event handler functions are generally not virtual (the wx events
mechanism is more or less orthogonal to the C++ virtual functions
mechanism, so it's confusing to mix the two). So I think this comment could
be simply removed. But beyond this, I wonder why do we need to have the
function this comment applies to, i.e. PreviewFrameEx::UponCloseWindow(),
at all? This is used to show the previously hidden top level window, but,
again, this just raises the question of why do we hide it in the first
place? This seems like a very strange and not user-friendly thing to do, it
must be pretty surprising to see the main lmi window suddenly disappear
when the print preview is shown. And, after just testing it, I see that
it's *especially* strange when using multiple displays and maximized main
window because you see the main window disappear from one display first and
then the (maximized) print preview appear on another display later.

 So I'd argue very strongly for removing this code entirely, instead of
just removing the "WX !!" comment above. If the intention is to prevent the
user from doing anything in the main window while the preview is shown, it
should be disabled, not hidden, and I could make a trivial patch using
wxWindowDisabler in order to do this. Should I do this? Or, maybe, we don't
even need to disable it neither and should do nothing at all? In any case,
I'd like to say once again that the current behaviour is very surprising
and, IMO, not helpful at all.


 The second comment I'm interested in is the one in previewframe_ex.cpp:

    // WX !! It would seem equivalent (and better) to default the
    // 'style' argument to
    //   wxDEFAULT_FRAME_STYLE | parent->IsMaximized() ? wxMAXIMIZE : 0
    // but that causes the preview frame of a maximized parent to
    // become a full-screen window, at least with wx-2.5.1 and msw.

This one doesn't seem to be relevant at all, at least I can't see any
problem with just doing what it says and adding wxMAXIMIZE to the style.
One thing which worries me a bit here is that I don't understand what is
meant by "full-screen window", as I'm pretty sure that ShowFullScreen()
hadn't even been added yet in wx 2.5 time, so I might be misunderstanding
what it is trying to say. But in my limited testing, using wxMAXIMIZE does
work just fine now and so I could make another trivial patch implementing
this suggestion and removing this comment.

 But you won't be surprised to hear that I have some other suggestions here
as well: I think the size handling here is misguided. PreviewFrameEx uses
the main window size by default, but there is no good reason for doing
this, e.g. I use a relatively small height for the main window, and it
would make more sense to use a much taller preview window than what it
currently is. Of course, this is subjective, but this is exactly why using
the same size for everybody is not great. A much better alternative, IMO,
is to use the same size as was used the last time, which can be achieved by
just calling wxPersistentRegisterAndRestore() for this frame, as we already
do for the main frame in skeleton.cpp since 00ad371e7 (Automatically
restore the main frame geometry, 2018-03-11). Would you agree to do the
same thing here? It would be a simple change and it would get rid of the
whole issue with maximizing the window entirely (well, we'd still call
Maximize() by default if there is no saved geometry, but there would be no
reason to even speak about using wxMAXIMIZE style).


 Please let me know what, if anything, should I do about these 2 problems
(and IMO both of them are actual UI problems, especially the first one,
even if the only reason I noticed them was due to looking for the "WX !!"
comments in the code).

 Thanks in advance,
VZ

pgp8SfxzUzXXr.pgp
Description: PGP signature