Re: [lmi] Allow switching skin while lmi is running

[Vadim Zeitlin]

On Tue, 31 May 2016 01:50:13 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-30 17:24, Vadim Zeitlin wrote:
GC> > On Mon, 30 May 2016 14:04:03 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> Now we have
GC> > GC>   ce_product_name.?pp
GC> > GC>   ce_skin_name.?pp
GC> > GC> and the first determines its enumerators in
GC> > GC>   product_names.?pp
GC> > GC> while the second does that in an unnamed namespace in the 'ce_' 
source.
GC> > GC> 
GC> > GC> I'd like to determine the enumerators in a parallel location for both:
GC> > GC> that makes code review and future maintenance easier. Do you strongly
GC> > GC> prefer that I not create new
GC> > GC>   skin_names.?pp
GC> > GC> files?
GC> > 
GC> >  No, not really strongly, but I think I prefer to keep them private in 
that
GC> > file just because I don't see any reason to "export" them. Why not merge
GC> > product_names.cpp with ce_product_name.cpp instead?
GC> 
GC> Okay, I'll do that soon.

 By the time I managed to reply to this mail you've already done it (in
cdd9aa41fb4860a00705ccb80b9c8eb98b9928ad), thanks!

GC> I plan to make a few more small changes. First of all, I'm thinking
GC> that the fetch_*_names() code can be simpler and more similar in both
GC> 'ce_*.cpp' files.

 I did try to make the new version as similar as possible to the existing
code, but there is a real difference between them: we can't select all .xrc
files in the skins case, but need to also check their name. Except for
this, they're pretty similar already I'd say.

GC> Showing basenames instead of full filenames in the GUI seems friendlier. I
GC> don't think we really need to distinguish between regexes
GC>   /skin\.xrc/ and /skin_.*\.xrc/
GC> because the single regex /skin.*\.xrc/ is good enough.

 I preferred to be more precise to avoid possible errors and the extra
complexity of checking for both versions is not big. Initially I also
wanted to discard the initial and not really significant "skin_" prefix and
show just the rest of the file name in the GUI, but I discarded this idea
because I wasn't sure what to do with "skin.xrc" itself (and there was also
a degenerate case of "skin_.xrc"). But I wanted to mention this idea just
in case you see any merit in doing it like this.

GC> And I wonder why I ever transformed '.product' filenames to lowercase, when
GC> we never distribute them with any uppercase letters; the reason is lost in
GC> the mists of time, but presumably had something to do with DOS 
compatibility.
GC> I think it's time to get rid of this.

 Very well, I don't see any point in doing this neither.

GC> Is there any particular reason for the following implementation?
GC>   std::string const& default_skin_name()
GC>   {
GC>       static std::string const default_name("default");
GC>       return default_name;
GC>   }

 No, it looks pretty useless to me. I'm afraid this was accidentally left
over from the older version of the patch which used "default" for
"skin.xrc" (and "foo" for "skin_foo.xrc"). I think leaving "value_" empty
by default would work just as well, but I didn't test it yet -- should I do
this?

 Thanks,
VZ