lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] PATCH: wxGrid-based census view


From: Vadim Zeitlin
Subject: [lmi] PATCH: wxGrid-based census view
Date: Wed, 1 Jul 2020 15:55:41 +0200

 Hello,

 I promised to do it before the end of June and, of course, I failed to do
it again, but, at least, not by much (in fact, I seriously thought about
posting this message 2 hours before the midnight in your time zone, when I
had actually finished all the changes, but finally decided to be serious
and rather postpone it to the morning, as a few more hours are almost
certainly not critical for something that was in development since many
months, already).

 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.

 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.


 As an aside, I'm afraid the fact that the new code is structurally similar
to the old one isn't the news you've been waiting for, as I know you hoped
that the new version would be simpler and easier to understand, but
unfortunately I just don't think there is much scope for any drastic
simplifications here. This could be seen in the positive light too by
proving that we didn't do anything particularly stupid when implementing
the original version 10 years ago and, indeed, I think the complexity here
is simply inherent to the task: we need to map lmi internal data to the
grid (or data view control) rows and columns in a type-safe way and there
is just no escaping all this boilerplate serving as an adaptor between the
business logic and the UI. Still, the only good news I can report is that
the new code is not any more complex than the old version -- but I can't
really claim that it's significantly less complex neither.


 Returning to the main topic, we've spent a lot of time on testing the new
code. The problem is that every time we tested it, we found new bugs or, at
least, aspects that could be improved, in wxGrid, so I'm still not sure,
even now, that we've found and fixed all of them. But we did fix many, many
problems (there have been almost 200 commits to wxGrid-related files) and
the grid-based census view is definitely much more usable now. There are
still a few things to improve, but I've decided that we had to stop at some
moment and so postponed them for later. Please let me know if you'd like me
to post about them right now or if you'd prefer if I gave you time to
integrate these changes before starting to discuss the other ones.

 In particular, one of the things I still hadn't had time to do, was to
build the PR under Linux using the official lmi makefiles in the official
lmi chroot created by the official lmi scripts. However we did test the
build under Cygwin and I also tested building lmi under Linux using
configure both natively and cross-compiling it to MSW, so I'm relatively
confident that there should be no problems with building it in the official
way neither -- but I'll try to confirm this a.s.a.p. And, of course, in
addition to (a lot of) interactive testing, we also checked that the unit
tests still pass too and they do, in all the builds we've tested (well,
except for the already mentioned problem with the outdated test data).

 Also, while I'm aware of several possible improvements, we are not aware
of any regressions compared to the current version in the latest version,
so if you find anything which doesn't work in the same (or better!) way as
before, please let me know.

 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).


 Please let me know if you have any questions and I hope that even if
you're disappointed by the lack of simplifications in the new code, you
will at least like the new grid-based UI, as it's indeed much more
spreadsheet-like and so better suited to lmi census view than the old one.

 Looking forward to your reactions,
VZ

Attachment: pgp3I_lLTkj4S.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]