Re: [lmi] Ellipses in menus

[Vadim Zeitlin]

On Fri, 8 Apr 2016 23:58:18 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2015-11-10 13:37, Vadim Zeitlin wrote:
...
GC> >  Other than that, I'm not too happy with the loop iterating over the menu
GC> > items and deleting the unwanted ones, but I'm afraid it's the best that 
can
GC> > be done with the current API: the problem is that the items themselves 
have
GC> > no relationship to each other and we can't ask the item for the next one,
GC> > so we have to iterate over the list of all of them and, as usual when
GC> > iterating and removing at the same time, the code is somewhat of a mess.
GC> > An alternative could be to iterate once to find the IDs of all the items 
to
GC> > delete and then just delete them by ID in a loop. This would be O(N^2) but
GC> > we don't care about this for N=4, the real problem is that we'd still have
GC> > to write a third loop cleaning up the consecutive separators and I thought
GC> > that one complicated loop was, on balance, preferable, to 3 slightly
GC> > simpler ones. But please let me know if you disagree.
GC> 
GC> The patched AdjustMenus() certainly is complex. In a local copy, I
GC> experimented with breaking it into two functions, in the hope of making
GC> it simpler:
GC> 
GC>   wxMenuBar* remove_print_menuitems(wxMenuBar*, ViewEx*) const
GC>   wxMenuBar* remove_test_menu(wxMenuBar*) const
GC> 
GC> Of the two places where AdjustMenus() is invoked, CreateChildFrame() would
GC> call both these new functions; InitMenuBar() would call only the second.

 This would make the function shorter but not really less complex as the
real complexity is in the loop above.

GC> And then there's the issue of removing separators, which demonstrates a
GC> conflict of paradigms. The XRC file is a document that lays out the content
GC> with separators. Then there's logic that changes the document contents,
GC> and it needs to know about those separators in order to remove them. And in
GC> addition, we have event tables in the view classes:
GC> 
GC> BEGIN_EVENT_TABLE(IllustrationView, ViewEx)
GC>     EVT_MENU(wxID_PRINT                         
,IllustrationView::UponPrint              )
GC>     EVT_MENU(wxID_PREVIEW                       
,IllustrationView::UponPreviewPdf         )
GC> ...
GC>     EVT_UPDATE_UI(wxID_PRINT                    
,IllustrationView::UponUpdateIllustration )
GC>     EVT_UPDATE_UI(wxID_PAGE_SETUP               
,IllustrationView::UponUpdateIllustration )
GC>     EVT_UPDATE_UI(wxID_PREVIEW                  
,IllustrationView::UponUpdateIllustration )
GC> 
GC> which aren't integrated with remove_print_menuitems() (or a more complex
GC> replacement for it, as mentioned above). Changing a menu now potentially
GC> requires understanding and modifying all three paradigms:

 The only alternative (and one that I would personally probably prefer) is
to construct the menu in code. As soon as you define it in the XRC and have
the event handlers for it defined in the event table in the code, there are
already two different places to modify and the new function doesn't really
change that much, after all it's complex precisely to avoid having to
modify it if the menu changes in the XRC, it's supposed to work with
whatever you put there.

GC> Taking a step back, I have to ask whether this is really a good idea
GC> after all. Sorry I didn't see this earlier, but today it does seem to
GC> me that retaining multiple menus in the XRC file is simpler than
GC> introducing complicated logic to modify a single unified menu.

 I still think that having triplicated menus in the XRC is a bad idea. For
me it's easier to work with (and to catch errors in) the code than XRC, so
I prefer to have complexity there.

GC> OTOH, if we'd rather have a single, unified menu, then maybe we can rework
GC> the logic to make it simpler. Removing items from a list by hand is fragile;
GC> can we use std::remove_if() with a wxMenuItemList?

 Sorry, I don't really see how can std::remove_if() help here, our
predicate is dynamic and this is not supported by the STL function.

GC> As for redundant menu
GC> separators, I find this function to remove them:
GC>   https://wiki.wxwidgets.org/WxMenu#Removing_unneeded_menu_separators

 Well, this is exactly the same kind of code as I wrote -- except with less
comments. Again, I don't really see how it is going to help beyond what I
suggested doing in my original message, i.e. replacing one complex loop
with several less complex ones (first to find the items to delete, then
delete them). Should I write an alternative version of this patch doing
this?


GC> If we could write a general function to do that, and test it carefully,
GC> that would remove a significant part of my concern about the complexity
GC> of this menu-adjustment function.

 Sure, writing such a function is not difficult.


GC> >  As usual, I've tested this with both MSVC and MinGW and I did by opening
GC> > or creating new documents of all possible types and verifying that the 
file
GC> > menu contained the expected items. I also checked that this was the case 
on
GC> > startup, i.e. for the main frame menu when there are no children.
GC> 
GC> I confirm that the patch introduces no regression. But I wonder whether
GC> the (unchanged) behavior is right.
GC> 
GC> The four "print" menuitems are:
GC>   File | Print
GC>   File | Page setup
GC>   File | Print preview
GC>   File | Print to PDF
GC> Here's how they are handled, with or without this patch:
GC>   cns: grayed
GC>   ill: allowed
GC>   mec: allowed, except "Print to PDF" grayed
GC>   gpt: allowed, except "Print to PDF" grayed
GC>   txt: absent
GC>   {product files}: absent
GC>   {no view}: absent
GC> It seems anomalous that census files always show all print menuitems,
GC> always grayed; is it better to remove them (and rename CanBePrinted()
GC> to uses_file_print_menuitems(), if we use it at all)?

 Yes, I think it is. As usual, I try to change only a single thing at once,
so I would still prefer to deal with this patch first, but once it's
applied the census view could be easily changed to not show the print menu
at all.

GC> The "Census" menu offers its own specialized print commands, none of
GC> which corresponds closely to "File | Print". However, OTOH, we plan to
GC> rework some of the "Census" reports (changing the contents of group
GC> quotes, and replacing XSL-FO with wxPdfDoc), and maybe we'll find that
GC> we really do want "File | Page setup" for census views.

 It won't be a problem to add it back then, of course.

GC> In general, which views really should offer "File | Page setup"? I don't
GC> have a printer attached to an msw machine, so it's hard for me to tell,
GC> but apparently this command can only invoke wxPageSetupDialog, so it's
GC> appropriate only for views that can print via wx, which means MEC and
GC> GPT only: '.cns' and '.ill' files print only to PDF, and the rest can't
GC> be printed at all. However, we plan to use wxPdfDoc for PDF creation;
GC> when we do that, will "Page setup" become relevant for these views?

 Yes, if we want it to.

GC> For example, will wxPdfDoc use wxPageSetupDialog to switch between
GC> portrait and landscape?

 Yes, I think so (I didn't test this, but it definitely appears in the
code, see 
https://github.com/utelle/wxpdfdoc/blob/c9133b4/include/wx/pdfprint.h#L53)

GC> Regardless, does it make sense to make "Page setup" available always?
GC> Suppose '.mec' and '.ill' views are open, and the '.ill' view is the
GC> currently active child window; if the end user wants to do "Page setup"
GC> in that situation (to affect the '.mec' view), should we forbid that?

 I think we should if "Page setup" doesn't do anything for printing
illustrations, otherwise it would be pretty confusing to carefully
configure the desired output format -- and have it completely ignored by
the program.

GC> (If we always offer "Page setup", then the group of menu items for
GC> printing is never empty, so we don't have to worry about removing its
GC> separator.)

 This is really not a good reason to provide sub-optimal UI.

GC> BTW, the behavior for MEC and GPT files is correct: their output is html,
GC> they're printable as html using wx's built-in support, and by design they
GC> don't create PDF output. However, we found an anomaly when testing the
GC> built-in printing--icon_monger::CreateBitmap() displays this message:
GC>             warning()
GC>                 << "Unable to find icon '"
GC>                 << icon_name
GC>                 << "' for current theme. Please report this problem."
GC>                 << "\nA blank icon will be used instead."
GC> for wxART_GOTO_FIRST, wxART_GOTO_LAST, and others. Do we need to add
GC> PNG bitmaps for all of these?

 Could it be the same issue as the one fixed by the patch at
https://github.com/vadz/lmi/pull/17 ?


 Please let me know if I should produce an updated/simplified patch or just
a function removing duplicate consecutive separators from the menu or do
anything else about this PR.

 Thanks in advance!
VZ