Re: [lmi] Ellipses in menus

[Greg Chicares]

[I'm working with this patch:
  
https://github.com/vadz/lmi/commit/f91ac00bb41d22750fe88ac53156ba2f472aafb3.patch
which seems to be just a rebased version of the October original, so I'll
comment in this original thread.]

On 2015-11-10 13:37, Vadim Zeitlin wrote:
> On Mon, 19 Oct 2015 03:36:00 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2015-10-18 19:34, Vadim Zeitlin wrote:
> GC> > On Sun, 18 Oct 2015 16:56:36 +0000 Greg Chicares <address@hidden> wrote:
> GC> > 
> GC> > GC>   20151018T1612Z Refactor
> GC> > GC> Removed irregularities: two File menus used as <object_ref> without
> GC> > GC> explanation, and a third written inline with explanation.
> GC> > GC> 
> GC> > GC> Is there a more tasteful way to do this? Ideally we'd code "File | 
> Open"
> GC> > GC> once and only once.
> GC> > 
> GC> >  I think the best way to do this would be to have a single file menu, 
> with
> GC> > all the items, and then remove those that we don't need after loading 
> it.
> GC> > As usual, patches doing this could be made available if there is any
> GC> > interest in them.
> GC> 
> GC> Yes, please: I'm definitely interested. Or, more precisely, I will become
> GC> interested after Halloween. Skeleton::AdjustMenus() already does something
> GC> similar.
> 
>  Thanks for the pointer, I've indeed decided to reuse this function to do
> the file menu adjustments as well as it seemed logical to keep all of them
> in a single place. This did require adding a new ViewEx::CanBePrinted()
> method to allow assigning the proper menu to each view, but IMO it makes
> sense to have it anyhow.

Along the same lines, we may as well add:
  private:
    virtual char const* icon_xrc_resource() const;
    virtual char const* menubar_xrc_resource() const;
and implement these (non-virtually) in the base class:
    // TODO ?? Consider making virtuals nonpublic and public functions
    // nonvirtual.
    virtual wxIcon Icon() const = 0;
    virtual wxMenuBar* MenuBar() const = 0;
I've done that, and cleaned up the other "TODO" markers in 'view_ex.hpp'.

I was going to add
  virtual bool uses_file_print_menuitems() const;
as well, but I'm not sure this is the design we want. For example, an
'.ill' view uses "File | Print", but not "File | Page setup" (it uses
PDFs only), so maybe we'd want fine-grained control over particular
menu items. (More on that below.)

>  Other than that, I'm not too happy with the loop iterating over the menu
> items and deleting the unwanted ones, but I'm afraid it's the best that can
> be done with the current API: the problem is that the items themselves have
> no relationship to each other and we can't ask the item for the next one,
> so we have to iterate over the list of all of them and, as usual when
> iterating and removing at the same time, the code is somewhat of a mess.
> An alternative could be to iterate once to find the IDs of all the items to
> delete and then just delete them by ID in a loop. This would be O(N^2) but
> we don't care about this for N=4, the real problem is that we'd still have
> to write a third loop cleaning up the consecutive separators and I thought
> that one complicated loop was, on balance, preferable, to 3 slightly
> simpler ones. But please let me know if you disagree.

The patched AdjustMenus() certainly is complex. In a local copy, I
experimented with breaking it into two functions, in the hope of making
it simpler:

  wxMenuBar* remove_print_menuitems(wxMenuBar*, ViewEx*) const
  wxMenuBar* remove_test_menu(wxMenuBar*) const

Of the two places where AdjustMenus() is invoked, CreateChildFrame() would
call both these new functions; InitMenuBar() would call only the second.

But remove_print_menuitems() is still quite complex. Removing menu items
is a delicate operation. It will become more complex if we decide we want
to remove some print menu items and leave others, depending on various
contexts.

And then there's the issue of removing separators, which demonstrates a
conflict of paradigms. The XRC file is a document that lays out the content
with separators. Then there's logic that changes the document contents,
and it needs to know about those separators in order to remove them. And in
addition, we have event tables in the view classes:

BEGIN_EVENT_TABLE(IllustrationView, ViewEx)
    EVT_MENU(wxID_PRINT                         ,IllustrationView::UponPrint    
          )
    EVT_MENU(wxID_PREVIEW                       
,IllustrationView::UponPreviewPdf         )
...
    EVT_UPDATE_UI(wxID_PRINT                    
,IllustrationView::UponUpdateIllustration )
    EVT_UPDATE_UI(wxID_PAGE_SETUP               
,IllustrationView::UponUpdateIllustration )
    EVT_UPDATE_UI(wxID_PREVIEW                  
,IllustrationView::UponUpdateIllustration )

which aren't integrated with remove_print_menuitems() (or a more complex
replacement for it, as mentioned above). Changing a menu now potentially
requires understanding and modifying all three paradigms: layout in a
document; event tables; and menu-adjustment logic (which needs to fix up
separators, which are document details). Taking a step back, I have to
ask whether this is really a good idea after all. Sorry I didn't see this
earlier, but today it does seem to me that retaining multiple menus in the
XRC file is simpler than introducing complicated logic to modify a single
unified menu.

OTOH, if we'd rather have a single, unified menu, then maybe we can rework
the logic to make it simpler. Removing items from a list by hand is fragile;
can we use std::remove_if() with a wxMenuItemList? As for redundant menu
separators, I find this function to remove them:
  https://wiki.wxwidgets.org/WxMenu#Removing_unneeded_menu_separators
but
 - I can't tell who owns it or whether it's licensed;
 - "-&gt;" is hard to read, though trivial to fix; and
 - worst of all, the full snippet presented "causes a crash" according to
   the text, and it's not completely obvious how to apply the correction.
If we could write a general function to do that, and test it carefully,
that would remove a significant part of my concern about the complexity
of this menu-adjustment function.

>  As usual, I've tested this with both MSVC and MinGW and I did by opening
> or creating new documents of all possible types and verifying that the file
> menu contained the expected items. I also checked that this was the case on
> startup, i.e. for the main frame menu when there are no children.

I confirm that the patch introduces no regression. But I wonder whether
the (unchanged) behavior is right.

The four "print" menuitems are:
  File | Print
  File | Page setup
  File | Print preview
  File | Print to PDF
Here's how they are handled, with or without this patch:
  cns: grayed
  ill: allowed
  mec: allowed, except "Print to PDF" grayed
  gpt: allowed, except "Print to PDF" grayed
  txt: absent
  {product files}: absent
  {no view}: absent
It seems anomalous that census files always show all print menuitems,
always grayed; is it better to remove them (and rename CanBePrinted()
to uses_file_print_menuitems(), if we use it at all)? The "Census" menu
offers its own specialized print commands, none of which corresponds
closely to "File | Print". However, OTOH, we plan to rework some of the
"Census" reports (changing the contents of group quotes, and replacing
XSL-FO with wxPdfDoc), and maybe we'll find that we really do want
"File | Page setup" for census views. That leads to another question...

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

Regardless, does it make sense to make "Page setup" available always?
Suppose '.mec' and '.ill' views are open, and the '.ill' view is the
currently active child window; if the end user wants to do "Page setup"
in that situation (to affect the '.mec' view), should we forbid that?
(If we always offer "Page setup", then the group of menu items for
printing is never empty, so we don't have to worry about removing its
separator.)

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