Re: [lmi] Using picker controls for paths (was: Allow switching skin while lmi is running)

[Vadim Zeitlin]

On Wed, 1 Jun 2016 17:54:43 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-31 20:15, Vadim Zeitlin wrote:
GC>
GC> [...]
GC> >  I don't have anything to add to this list, I just wanted to say that it
GC> > would be better to choose wxFilePickerCtrl and wxDirPickerCtrl for
GC> > default_input_filename_ and print_directory_ respectively, instead of 
plain
GC> > wxTextCtrl.
GC> 
GC> Sounds like a good idea. Since I'm not familiar with those controls,
GC> could I prevail upon you to add them to lmi's 'transferor.?pp'?

 I've done this now, please see the simple changes at

        https://github.com/vadz/lmi/pull/39

GC> (I'm pretty sure we want wxFLP_FILE_MUST_EXIST,

 I did use it for both controls. Notice that it is still possible to enter
non-existent path by typing it in manually, but IMHO it's a feature and not
a bug: if a user _really_ wants to use a file/directory which doesn't exist
(yet?), we shouldn't prevent him from doing it, the MUST_EXIST flags are
helpful hints rather than mandatory policy.

GC> but I'm not sure exactly how, say, GetPath() and GetFileName() differ.)

 They only differ by the type of their return value, the first one returns
a string while the second one returns a wxFileName (~~ boost::path) object.

 Actually the first version of my patch uses SetFileName() with the
following comment:

        if(td == from_string_to_control)
            {
            // Use SetFileName() here instead of SetPath() directly in order to
            // format the path correctly, e.g. use more usual backslashes under
            // MSW even if the path is stored using slashes in the
            // configuration file.
            control.SetFileName(wxFileName(data));
            }
        else
            {
            // Use just GetPath() here because we want to store the path
            // exactly as the user entered it.
            data = control.GetPath().ToStdString();
            }

I.e. the idea was that the default path "C:/etc/opt/lmi/default.ill" would
be shown to the user as more usual "C:\etc\opt\lmi\default.ill". And this
worked without any problem until I started using the directory picker for
the print directory as well. At which point I started getting warnings
about "Contents of more than one control changed" whenever the preferences
dialog was opened because both the file and directory pickets were changed
due to the normalization performed by SetFileName()/SetDirName()
respectively.

 I didn't see any simple way to avoid this warning, so, finally, I've just
abandoned the idea of normalizing the paths when showing them and the final
version simply shows them as they're, which works. Maybe you don't consider
normalizing the paths be a good idea anyhow, of course, but if you do, I
could try looking more into making this work without disabling/removing the
check for the number of changes controls in MvcController::UponUpdateUI().
Should I do this?


 In addition to this, there are two minor cosmetic issues with the dialog
now. The first one is that the pickers "..." buttons are too close
together, I would add some vertical separation between these (and hence
also the other, for consistency) rows. How much of it is needed is a rather
subjective question, so I think it might be best if you just did whatever
you consider to be aesthetically pleasing there. But if you'd prefer, I
could do it too, of course.

 The second point is worse, although it only appears when themes are
enabled, meaning that you won't see it by default. However most (all?)
users will and if you enable themes, you can see that the background
between the pickers text and button is grey and not the same white as the
notebook background around them. I'm not sure why does this happen yet, but
I'm almost certain it's a bug in wxMSW. And this means that it can only be
fixed in wxMSW itself. However a possible workaround in lmi could would be
to explicitly set the pickers background to white, and, to ensure that this
looks correctly in all themes and not just the default one, also set the
white background for the panel itself. This is not ideal because it still
will override the theme chosen by the user and will look strange when
themes are disabled, so I'd prefer to fix this in wxMSW itself -- but even
if I manage to do it, this would require updating the version of wxWidgets
used by lmi once again.


GC> Could I also ask you to opine on the "TODO ??" markers in transferor.cpp',

 I'll do this next but I wanted to post this quickly to let you know about
the new PR.

 Please let me know what do you think of it and if you have any comments
about the other issues mentioned above, thanks in advance!
VZ