Re: [lmi] product editor patch

[Vaclav Slavik]

Greg Chicares wrote:
> I've been working through all of them, and have already committed
> most. These are the few hunks that remain:
>
> (A) mvc_controller.cpp
> s/wxControlWithItems/wxItemContainer/
> Please help me understand the benefit of this change.

It makes the code compile under wxGTK. This piece LMI code makes the 
assumption that wxComboBox derives from wxControlWithItems, but 
that's an implementation detail of wxMSW and it's not true in  wxGTK 
or wxMac (contrary to what the documentation says, unfortunately). 
It's derived from wxItemContainer on all platforms.

> (B) illustration_view.cpp
> [add a wxBusyCursor in IllustrationView::Pdf()]
> I tested this, but observed no busy cursor.

I don't understand that. I just tested the change on MSW and the busy 
cursor is shown immediately after I click Print or Print preview 
toolbar buttons (or menu items) here and stays there while "iteration 
something" messages are shown in the statusbar.

> (1) Should "File | Save" indeed be enabled for an unmodified new
> document? I see two options:
>   (a) disable it; or
>   (b) enable it, but map it to "File | Save as".
> I've always followed (a) and disabled it, but that's really just
> a habit rather than a conscious choice. Is there a reason to
> follow (b) instead? 

It makes it possible to save empty document without having to go 
through Save as (i.e. do something other than what you normally do to 
save documents). If it's impossible to have empty documents in LMI, 
then it should certainly be disabled -- but then, so should Save as.

> I've looked at various interface standards, 
> but can't find specific guidance. I see each of these behaviors
> in more than one msw application. Does another platform mandate
> one or the other?

I always considered the behavior implemented by the patch to be 
standard, but you're right that it's not consistently used. On my 
GNOME desktop, all apps [that I tried] enable Save from the 
beginning. But at least one KDE app and OpenOffice don't enable it 
until the file was modified (no idea if it's intentional, though). I 
couldn't find it in any HIGs either, except for OSX one:

"""
Save (Command-S). Saves the active document, leaves the document open, 
and provides feedback indicating that the document is being (or has 
been) saved. If the document has not previously been saved, dim the 
Save command and make sure the Save As command is active instead. If 
an open document has been previously saved and the user has made no 
changes to it, the Save command is dimmed.
"""

This is a bit different (Save should never be enabled for documents 
not yet associated with a file, Save As should always be enabled, 
even if the document wasn't edited yet). You can interpret this as 
either saying that Save should be disabled in our case or that the 
decision whether to enable it or disable for new documents doesn't 
depend on whether it was modified or not yet.

Interestingly, Apple's own TextEdit app doesn't follow this guideline 
and has Save enabled for new documents (even before it's modified). 

> (2) If we want to change from (1)(a) to (1)(b), then there are
> other source files that'll need to be changed as well.
>
> (D) inputillus_xml_io.cpp, ledger_xml_io.cpp, xml_lmi.cpp
> Apparently the purpose of these changes is to avoid writing
> empty xml child text nodes. Can you give me an example of the
> problem that this change would solve? Is it simply a matter of
> writing '<X/>' instead of '<X></X>' because the former seems
> more tasteful?

As far as I can tell, yes. (And shorter, too, and add_node() was 
already used in other places, so it made sense to use it for 
consistency alone when I saw this change.)

Vaclav

-- 
PGP key: 0x465264C9, available from http://pgp.mit.edu/