Re: [lmi] PATCH: wxGrid-based census view

[Vadim Zeitlin]

On Tue, 14 Jul 2020 20:09:53 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2020-07-01 13:55, Vadim Zeitlin wrote:
GC> [...]
GC> >  So here, finally, are the changes implementing support for using wxGrid
GC> > instead of wxDataViewCtrl in the census view: the first, main, PR which
GC> > I'd like to ask you to merge is https://github.com/vadz/lmi/pull/143 and
GC> > adds support for using wxGrid, while keeping using wxDataViewCtrl by
GC> > default. To enable the new implementation, "--pyx=use_census_grid" command
GC> > line option must be used.
GC> 
GC> Okay, I'm starting to look at it, and I have some preliminary questions.
GC> We have PR #143 and a separate PR #144. You say #143 is the "first, main"
GC> PR. Then is #144 a second, auxiliary PR that should be merged after we
GC> finish merging #143? Or is PR #144 intended only as a supplementary
GC> demonstration, no part of which should actually be merged?

 Yes, exactly, sorry for not explaining this better. PR #144 is provided
just to better show how the new code replaces the old one, which is not
something you can really see with PR #143, which just adds new code.

GC> I.e., can I accomplish the total mission just as well if I locally
GC> remove #144 to avoid confusion?

 I'm not sure what do you mean by "removing it". You can avoid looking at
it completely, but IMO https://github.com/vadz/lmi/pull/144/files provides
a better view of some of the changes. At least it was helpful to me when I
was reviewing them myself.

GC> PR #143 has forty-five commits, but #144 has only two. Does the second
GC> of those two commits in #144 represent the forty-five in #143 all
GC> squashed together?

 Yes.

GC> I suppose the answer must be "no" because it's such
GC> a small commit (five old wxDVC lines replace by five new wxGrid lines);

 I think you must be looking at the wrong commit. The right one is
https://github.com/vadz/lmi/pull/144/commits/b3ccd39fc2fa13424fc41b1a41044e2c2955708f
and it definitely contains more than 5 lines.

GC> Each PR's first commit:
GC>   2f38b532d5b521ddeab59efcee37ed63990b2dee [PR #143]
GC>   68af9a099e8ca233efc6121fed3349b6febbd90b [PR #144]
GC> is described as
GC>   Upgrade wxWidgets and wxPdfDoc
GC> and they seem to do exactly the same thing. I'm guessing that the
GC> SHA1s differ only because they reside on different branches.

 Yes, and so were done at separate times, which is already enough to make
the SHA-1s differ.


GC> Here's a more immediate question. From your POV, the first commit
GC>   2f38b532d5b521ddeab59efcee37ed63990b2dee [PR #143]
GC>   Upgrade wxWidgets and wxPdfDoc
GC> may be just a tiny preliminary step that sets the stage, but from
GC> our POV it seems like half of the total testing effort--because
GC> all the rest of the commits presumably affect the census manager
GC> only, but upgrading wx and wxPdfDoc could conceivably introduce
GC> a new defect (or expose an existing, latent one) almost anywhere.

 There are actually very few changes in wxPdfDoc, I've only upgraded it to
get rid of an annoying warning which appeared when using the currently used
version with the latest wxWidgets. If it's really such a risk to upgrade it
now, I could cherry-pick only the commit fixing this warning into our
version. Would this be helpful?

GC> For that reason, what I'd like to do right now is "merge" that
GC> first commit only, in isolation, and push it to origin/master.
GC> How can I do that without destroying the history on your branch?

 I could explain how to do this (and it's not difficult, you'd just have to
merge a branch containing this commit only), but I think it would be
simpler if you just applied this commit in any way you see fit, i.e. either
by running "git cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee" or by
just doing it manually if it's simpler for you and I could then rebase both
PRs on your latest master -- this is something that would take me a couple
of minutes, so it definitely wouldn't be a problem for me. So I think you
should just do it like this, what do you think?

 Unless you prefer to make an even more minimal wxPdfDoc update as
described above, of course.

 Also, another complicating factor is that someone has made a further
change to wxGrid combobox editor (which is used in the new census editor)
which is supposed to improve its behaviour under GTK and I hoped to
integrate their PR at https://github.com/wxWidgets/wxWidgets/pull/1941 soon
and surreptitiously update my PRs to use a newer wxWidgets commit including
it instead. In fact, I really, really hope to release wxWidgets 3.1.4 in a
week and I even thought about updating lmi to use it, but I didn't have
time to test this fully yet and now I don't know if you feel like waiting
for it or not.

 So there are several reasonable things to do:

1. You can cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee right now.
2. You can wait until tomorrow (I'm not sure I can manage this today...)
   for an updated wx commit including more wxGrid improvements from me.
3. You can wait until Sunday for a commit that will be sufficiently close
   to be almost indistinguishable from the official 3.1.4 release (I'll
   only make documentation/distribution, but not code changes after this).

 Ah, and also, each of these could be combined with using the wxPdfDoc
commit in the current PRs or the minimal changes on top of the existing
version just to fix the warnings.

 Please let me know which one would you prefer.


 The rest of this message is just some Git stuff which is not that
important or interesting, but let me answer this just in case it can be
useful:

GC> - I suppose I could merge #143 in totality, locally:
GC>     git merge 
GC>     git remote add xanadu https://github.com/vadz/lmi.git
GC>     git fetch xanadu
GC>     git checkout master
GC>     git merge ..xanadu/census-view-grid

 You can't merge a range of commits, which is what "A..B" produces
(remember that "..B" is just a shortcut for "HEAD..B"), so this would be

        git merge xanadu/census-view-grid

This would produce a new "merge commit" with 2 parents: the current lmi
master and the last commit of xanadu/census-view-grid branch. I.e. the
commit graph will become

        Current lmi master ------------------> New lmi master (merge)
          (ce7f5fa)                                 ^
                                                    |
        wx update commit ---- .... ---- last branch commit
          (2f38b53)                         (5b96490)

GC> and then push only the first commit:
GC>     git push origin 2f38b532d5b521ddeab59efcee37ed63990b2dee
GC> [or some new SHA1, if 'git merge' changes it].

 You wouldn't be able to do this because this commit will not have the
current lmi master as its ancestor.

GC> - I suppose I could cherry-pick that first commit only
GC>     git cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee
GC>     git push
GC> but I fear that's exactly what you don't want me to do.

 No, this is fine, I'll just rebase the branches on top of this. This
commit is special and I don't really care about it. I do care about the
history of the other commits, as it will really help me to understand what
went through my head when I have to debug some problem in this code in a
couple of years.

GC> Is there some other, better way to accomplish this? Please
GC> bear in mind that I never run "git merge" and have only a
GC> vague conceptual notion of what it does, so it would be
GC> most helpful if you specified exact literal commands that
GC> I could just copy and paste.

 The literal command would be just git-cherry-pick above. The only
potential problem with doing it is that if you have already created any
local versions of remote branches in my repository you would need to
recreate them once I rebase my branches (or just one branch, if you're
absolutely sure that you don't care about PR #144 and its corresponding
branch), as rebasing changes all the SHA-1s, of course.

 But if you refer to them as xanadu/branch-name anyhow, you don't even need
to do this, just rerunning "git fetch xanadu" would be enough (and it will
tell you that some branches were force-updated, which is just its way of
warning you about the potential problem mentioned above).

 Please let me know if I you have any more questions and, especially, if
you'd like me to produce an updated wx*-upgrade commit with different
versions of either wxWidgets or wxPdfDoc or both.

 Thanks in advance!
VZ

pgpCaNL2O8_OV.pgp
Description: PGP signature