Re: [lmi] Using picker controls for paths

[Vadim Zeitlin]

On Tue, 7 Jun 2016 20:18:13 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-06-04 15:40, Vadim Zeitlin wrote:
GC> > On Wed, 1 Jun 2016 17:54:43 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC> but I'm not sure exactly how, say, GetPath() and GetFileName() 
differ.)
GC> > 
GC> >  They only differ by the type of their return value, the first one returns
GC> > a string while the second one returns a wxFileName (~~ boost::path) 
object.
GC> > 
GC> >  Actually the first version of my patch uses SetFileName() with the
GC> > following comment:
GC> > 
GC> >         if(td == from_string_to_control)
GC> >             {
GC> >             // Use SetFileName() here instead of SetPath() directly in 
order to
GC> >             // format the path correctly, e.g. use more usual backslashes 
under
GC> >             // MSW even if the path is stored using slashes in the
GC> >             // configuration file.
GC> >             control.SetFileName(wxFileName(data));
GC> >             }
GC> >         else
GC> >             {
GC> >             // Use just GetPath() here because we want to store the path
GC> >             // exactly as the user entered it.
GC> >             data = control.GetPath().ToStdString();
GC> >             }
GC> > 
GC> > I.e. the idea was that the default path "C:/etc/opt/lmi/default.ill" would
GC> > be shown to the user as more usual "C:\etc\opt\lmi\default.ill". And this
GC> > worked without any problem until I started using the directory picker for
GC> > the print directory as well. At which point I started getting warnings
GC> > about "Contents of more than one control changed" whenever the preferences
GC> > dialog was opened because both the file and directory pickets were changed
GC> > due to the normalization performed by SetFileName()/SetDirName()
GC> > respectively.
GC> > 
GC> >  I didn't see any simple way to avoid this warning, so, finally, I've just
GC> > abandoned the idea of normalizing the paths when showing them and the 
final
GC> > version simply shows them as they're, which works. Maybe you don't 
consider
GC> > normalizing the paths be a good idea anyhow, of course, but if you do, I
GC> > could try looking more into making this work without disabling/removing 
the
GC> > check for the number of changes controls in MvcController::UponUpdateUI().
GC> > Should I do this?
GC> 
GC> [I retained an unusually large chunk of context above so that we don't have
GC> to look up old emails.] I originally interpreted this as suggesting that
GC> only forward slashes would be used, everywhere.

 No, sorry for creating this impression. I should have mentioned that one
of the motivations behind the initial version was to avoid a mix of forward
and backward slashes as the paths returned by the native file picker dialog
do use forward slashes, of course.

GC> Now I run lmi in the background, changing the path...
GC>  - by typing nonexistent directory 'C:/opt/lmi/bdata'
GC>  - by using the dirpicker GUI to select opt|lmi|data
GC> and grep the xml for the directory as above...
GC> 
GC> /opt/lmi/bin[0]$./lmi_wx_shared --ash_nazg --data_path=/opt/lmi/data &
GC> [1] 1320
GC> /opt/lmi/bin[0]$grep print_dir ../data/configurable_settings.xml
GC>   <print_directory>C:\opt\lmi\bdata</print_directory>
GC> /opt/lmi/bin[0]$grep print_dir ../data/configurable_settings.xml
GC>   <print_directory>C:\opt\lmi\data</print_directory>
GC> 
GC> ...and my forward slashes are changed to backward.

 Yes, this is the (currently) expected behaviour.

GC> I don't know for sure whether that change might lead to a grave problem
GC> like inability to read a file,

 I really don't think so. If anything, using forward slashes might as
they're "foreign" under MSW and, annoyingly, a few native functions do no
support them -- while most of the rest of them do. Backslash is the native
path separator, so I don't see how could using it result in any problems.

GC> but I really would like to keep forward slashes everywhere and always.
GC> It looks really strange in the GUI when
GC> I change only one of these controls:
GC>       Input defaults [C:/etc/opt/lmi/default.ill]  <-- FORWARD
GC>   Printout directory [C:\opt\lmi\data]             <-- BACKWARD

 Yes, I agree with this. I still think the best solution is to consistently
use backslashes rather than [forward] slashes under MSW, but it's better to
consistently use either one of them rather than mixing both.

GC> Curiously, if I exit and reload lmi, I see only forward slashes in
GC> both GUI fields, though the xml file contains backslashes.

 This is a side effect of using fs::system_complete() in
configurable_settings ctor. FWIW I wouldn't be surprised if newer version
of Boost.Filesystem didn't behave in this way, it looks wrong to me to
change the path separators, especially for an already absolute path.

GC> Can we just have forward slashes, everywhere and always?

 I could try to do it, but personally I still think that it would be more
correct to use backslashes in the UI. We could, however, use slashes in the
XML file. Maybe this would be a good enough compromise?

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

 This is very strange because it's 100% reproducible under Windows 7 with
the default theme and I'm pretty sure it should be reproducible under
Windows XP as well. I didn't test this under Windows 10 yet, but I'm
relatively confident that this should happen there as well.

GC> I was going to try setting a theme here, but I'm currently
GC> using "Windows Classic (modified)" and am afraid of losing my forgotten
GC> but careful modifications, and this msw-xp VM is not at all like what end
GC> users have anyway. In theory, I might try it on my corporate laptop, but
GC> it's locked down so I can't run lmi (or even browse this mailing list).
GC> Anyway, you've described what you see in enough detail, but it just doesn't
GC> seem to happen in our end users' world.

 I'm very surprised by this because, again, it's certainly quite visible on
stock Windows 7 installation. But maybe this point is moot anyhow because
I've already fixed this problem and will commit the fix to wxWidgets soon.

GC> > GC> Could I also ask you to opine on the "TODO ??" markers in 
transferor.cpp',
GC> > 
GC> >  I'll do this next but I wanted to post this quickly to let you know about
GC> > the new PR.
GC> 
GC> I think I'll just go ahead and resolve all of them except the one
GC> concerning wxControlWithItems and wxItemContainerImmutable.

 I've started looking at those, but am not ready to post the patch yet, so
if you'd like to do it immediately, please go ahead, but if it's not very
urgent, I could still do it, after finishing with the "..." buttons
business.

 Please let me know if you finally decide to leave this to me, as well as
your final decision about the slash business.

 Thanks in advance,
VZ