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

[Vadim Zeitlin]

On Wed, 19 Aug 2020 21:50:02 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2020-08-19 10:49, Vadim Zeitlin wrote:
GC> [...]
GC> >  It's nice to be ahead of schedule for once (apparently removing code is
GC> > faster than adding it, who would have thought) -- Ilya has already done 
and
GC> > tested the changes and here is the PR with them:
GC> > 
GC> >   https://github.com/vadz/lmi/pull/148
GC> 
GC> I've cherry-picked and pushed that single-commit PR. Thanks.

 Thanks for applying it!

GC> BTW, I'm glad to see that the number of grid rows or columns is a regular
GC> 'int'. A grid really did benefit by allowing 2^16-1 rows instead of 2^15-1
GC> in the old 16-bit days, but today 2^63-1 is quite sufficient.

 Yes, I've converted to the "int is the default integer type and there is
no better integer type than int" religion^W school of thought now, thanks,
in no small part, to your arguments.

GC> I only have the minutest question--as for this change:
GC>   git diff 3e9f8af6^ -- wx_test_validate_output.cpp
GC> would the following patch be a (very slight) improvement?
GC> 
GC> --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--
GC> diff --git a/wx_test_validate_output.cpp b/wx_test_validate_output.cpp
GC> index c527848d..7b730731 100644
GC> --- a/wx_test_validate_output.cpp
GC> +++ b/wx_test_validate_output.cpp
GC> @@ -344,9 +344,8 @@ void validate_run_cell_and_copy_output
GC>  
GC>      wxUIActionSimulator ui;
GC>  
GC> -    ui.Char(WXK_UP);                  // Clear the current selection if 
any.
GC> +    ui.Char(WXK_ESCAPE);              // Clear selection; end editing.
GC>      ui.Char(WXK_HOME, wxMOD_CONTROL); // Go to the left top cell.
GC> -    ui.Char(WXK_RIGHT, wxMOD_SHIFT);  // Select the first row.
GC>  
GC>      ui.Char('r', wxMOD_CONTROL);      // "Census|Run cell"
GC>      wxYield();
GC> --8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--8<--
GC> 
GC> WXK_ESCAPE seems to fulfill the purpose of the WXK_UP above,
GC> and it also aborts editing of any text control that might be
GC> open for editing (e.g., due to failure of some future test
GC> that hasn't been written yet).

 The wonders of wxGrid are indeed infinite because even after spending so
much time on it, I still didn't realize that WXK_ESCAPE could be used for
clearing selection -- but you're right, it does, and using it here would be
better, if only because it's less arbitrary than WXK_UP.

 I'm less sure about the "end editing" part -- there should be no in place
editor in this place, and we should arguably add an assert checking that
this is not the case if we care about it, rather than implicitly dismissing
it. But this is an even more minute detail.

GC> And I don't think it's necessary explicitly to select any row,
GC> because Ctrl-Home already implicitly selects the first row.

 Yes, thank you, this must be a left over from the old version of the
grid-based census view which didn't do this.

 Would you like another PR applying the patch above or will you just
test/commit it yourself?

GC> Here's a question in the same vein, which has nothing to do
GC> with any recent changes. Here:
GC> 
GC>         // There could be an existing comment in this field, delete it 
first.
GC>         // This does assume MSW-like key bindings.
GC>         ui.Char(WXK_HOME);
GC>         ui.Char(WXK_END, wxMOD_SHIFT);
GC>         ui.Char(WXK_BACK);
GC> 
GC> what would GTK-like key bindings look like? AFAICT, the
GC> {Home; Shift-End; Backspace} sequence above should work
GC> there, too.

 De facto it does, but in principle nothing guarantees it, as GTK bindings
are fully customizable. I'm not sure what could we do about this however.
We could, of course, just manipulate comments_text directly, using
wxTextCtrl API, but this might (again, in principle, even though probably
not in practice) work differently from the (actual or simulated) user
entry. So I'd just leave this in the current state until we really run into
a problem due to these keys not working somewhere.

 Regards,
VZ

pgpy7OcAWm7Cc.pgp
Description: PGP signature