Re: [lmi] wxProgressDialog z-order anomaly

[Vadim Zeitlin]

On Sat, 4 Nov 2017 18:03:51 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-11-04 16:39, Vadim Zeitlin wrote:
GC> > On Sat, 4 Nov 2017 15:41:59 +0000 Greg Chicares <address@hidden> wrote:
GC> [...severe pruning...]
GC> >  But if you want to preserve the linear history, you have another choice:
GC> > replace the 3 commits of the branch on master. This can be done manually 
by
GC> > using "git cherry-pick" to pick the commits one by one:
GC> > 
GC> >         $ git cherry-pick github-vz/menu-refresh-fix~2
GC> >         $ git cherry-pick github-vz/menu-refresh-fix~1
GC> >         $ git cherry-pick github-vz/menu-refresh-fix
GC> > 
GC> > Or it can be done by cherry-picking them all at once:
GC> > 
GC> >         $ git cherry-pick ..github-vz/menu-refresh-fix
GC> 
GC> I want to study the rest of your email first before choosing a method,
GC> but if I were to do it this way, should it have the same effect as my
GC> wearisome but proven method of adding '.patch' at the end of each
GC> individual patch's URL, viz.:
GC>   
https://github.com/vadz/lmi/commit/c334ae0937894937570737f5348c0b19d83da0b6.patch
GC>   
https://github.com/vadz/lmi/commit/4bea744dfba3ddca45bb312ea61528761de9a549.patch
GC>   
https://github.com/vadz/lmi/commit/188309c53b2a759516024fef525e8f31fa07c6fc.patch
GC> and then saving their contents and running git-am on each in succession?

 Yes, this is basically what it does underneath (and so does "git rebase"
too). Using "git cherry-pick" is more convenient because it has handy
options like "--abort" to return to the initial state if something has gone
very wrong and it cleans up its temporary files automatically but, again,
fundamentally that's all it does.

 Remember that Git is just "stupid content tracker", it doesn't do anything
complicated, but just does a lot of simple things automatically for you. Of
course, as anybody who has studied dialectical materialism knows, at some
point the sheer quantity of all these small things it does for you
automatically transforms into a drastic change in quality of your workflow.


GC> BTW, I reviewed the changes, and they're tremendous improvements.
GC> (Actually, at first I made the mistake of reviewing them all reversed,
GC> and every single reversed change was shockingly awful--I don't know
GC> how I could ever have written that stuff.)

 I'm pretty sure that GetPopupMenuSelectionFromUser() didn't exist when you
wrote this code, so the first two commits are just a modernization and not
a real improvement. As for the last one, it's really just a dirty hack and
I am not at all proud of it, but it seems like a small cost to pay to solve
a problem which is not _that_ critical in the grand scheme of things and so
might not be worth spending more time on it.

GC> I have only one question,
GC> concerning this line:
GC> 
GC>     if (wxEventLoopBase* const loop = wxEventLoopBase::GetActive())
GC> 
GC> Do you think this:
GC> 
GC>     if (nullptr != wxEventLoopBase* const loop = 
wxEventLoopBase::GetActive())
GC> 
GC> would be an improvement,

 No, because it wouldn't compile.

GC> Hmm...or maybe
GC> 
GC>     wxEventLoopBase* const loop = wxEventLoopBase::GetActive();
GC>     if (nullptr != loop)
GC> 
GC> would be best of all?

 I prefer the current version because it doesn't leak the "loop" variable
outside of the block where it is used. I really don't think that the idea
of testing for a pointer validity using a simple "if" is bad on its own. In
some sense it's actually better than comparing it with nullptr explicitly
IMO because we're not really interested in a comparison, but just in its
validity. E.g. if we used smart pointers and not raw ones, we could still
write exactly the same thing with exactly the same intent and actual
effect, which is IMO on its own a good argument for preferring it.

 I do realize that this is a contentious point though...
VZ