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

[Greg Chicares]

On 2020-07-01 13:55, Vadim Zeitlin wrote:
[...]
>  So here, finally, are the changes implementing support for using wxGrid
> instead of wxDataViewCtrl in the census view: the first, main, PR which
> I'd like to ask you to merge is https://github.com/vadz/lmi/pull/143 and
> adds support for using wxGrid, while keeping using wxDataViewCtrl by
> default. To enable the new implementation, "--pyx=use_census_grid" command
> line option must be used.

Okay, I'm starting to look at it, and I have some preliminary questions.
We have PR #143 and a separate PR #144. You say #143 is the "first, main"
PR. Then is #144 a second, auxiliary PR that should be merged after we
finish merging #143? Or is PR #144 intended only as a supplementary
demonstration, no part of which should actually be merged? I.e., can I
accomplish the total mission just as well if I locally remove #144 to
avoid confusion?

>  The diff (https://github.com/vadz/lmi/pull/143/files) for this PR shows
> the new code in census_view.cpp as being added, which may be nice, as it
> allows you to relatively easily review the additions, but this doesn't
> paint the full picture, as you can't easily see that most of the new code
> relatively closely parallels the existing code. In order to show it better,
> I've also created https://github.com/vadz/lmi/pull/144, which replaces the
> existing implementation with the new one. As such, it is not meant to be
> applied, but I think looking at the diff in it can be useful because it
> shows what the old code has been effectively replaced with in the new
> version.

PR #143 has forty-five commits, but #144 has only two. Does the second
of those two commits in #144 represent the forty-five in #143 all
squashed together? I suppose the answer must be "no" because it's such
a small commit (five old wxDVC lines replace by five new wxGrid lines);
but then what does it represent? Is it just the net effect on
'census_document.{c,h}pp', that one pair of files being chosen because
they clearly replace wxDVC with wxGrid and do nothing else?

Or, since this second commit in #144 is described as
  Use wxGrid instead of wxDataViewCtrl for the census view
and that sounds like the final step in the complete process, am I to
cherry-pick that after merging #143?

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

>  Finally, as always, I think it would be great if you could please merge
> the existing branch on which PR 143 is based "as is", as it shouldn't
> change the default lmi behaviour in any way nor have any other drawbacks,
> and then make any changes as you see fit, because I'm a (commit) history
> junkie and hate losing it. And if you run "git merge census-view-grid"
> right now, it won't even create an actual merge, as I've just rebased our
> branch on the latest lmi master. But this is just a reminder of my
> preferences and not a full-blown attempt to restart the discussion about
> the merits of using Git branches (that I promised not to return to again).

Here's a more immediate question. From your POV, the first commit
  2f38b532d5b521ddeab59efcee37ed63990b2dee [PR #143]
  Upgrade wxWidgets and wxPdfDoc
may be just a tiny preliminary step that sets the stage, but from
our POV it seems like half of the total testing effort--because
all the rest of the commits presumably affect the census manager
only, but upgrading wx and wxPdfDoc could conceivably introduce
a new defect (or expose an existing, latent one) almost anywhere.
For that reason, what I'd like to do right now is "merge" that
first commit only, in isolation, and push it to origin/master.
How can I do that without destroying the history on your branch?

- I suppose I could merge #143 in totality, locally:
    git merge 
    git remote add xanadu https://github.com/vadz/lmi.git
    git fetch xanadu
    git checkout master
    git merge ..xanadu/census-view-grid
and then push only the first commit:
    git push origin 2f38b532d5b521ddeab59efcee37ed63990b2dee
[or some new SHA1, if 'git merge' changes it]. Then I'm left
with forty-four unpushed changes, and as a practical matter
I can't push any other changes until I push those, so that's
inconvenient for me. But I think it does what you want.

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

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