Re: [lmi] Correctness and performance of varying column width mode in the census view

[Vadim Zeitlin]

On Thu, 28 May 2020 13:50:21 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2020-05-27 18:32, Vadim Zeitlin wrote:
GC> > On Wed, 27 May 2020 16:56:46 +0000 Greg Chicares 
<gchicares@sbcglobal.net> wrote:
[...]
GC> >  Currently it is indeed far too costly, but there is no intrinsic reason
GC> > for it to be so, at least not when changing a single cell, because all we
GC> > need to do is to measure the length of a single string and increase the
GC> > column width if the new string doesn't fit into it.
GC> 
GC> My impression is that spreadsheet programs might do that sometimes,
GC> but certainly not always--e.g., they might adjust the width of only
GC> the current column when a new cell value is typed in, but not when
GC> a cell or a block of cells is pasted. In general, though, they seem
GC> not to auto-re-size.

 Yes, indeed. FWIW Microsoft Excel (which is the de facto standard for the
spreadsheet, like it or not, and the one all the others emulate) wraps
longer strings by making the cell containing them taller if possible. I
wonder if we should consider doing this too?

GC> >  Now there are other complications, e.g. the operation would still be 
O(N),
GC> > where N is the number of rows, if we wanted to also reduce the column
GC> > width, but IMO this is much less important and we could avoid doing this.
GC> 
GC> I don't think it's very important to treat narrowing as a special case,

 I believe it's much more important to fully show the newly entered string,
i.e. widen the column enough to fit it, than to narrow the column back if
it becomes unnecessarily wide. Not doing the latter just wasts some space,
while not doing the former prevents you from fully seeing your data, which
seems much worse.

 In fact, if we don't do this, I think it would be reasonable to expect
users to often hit Ctrl-] to adjust all columns widths after entering such
overlong string, just to see it fully. And the problem is that doing full
auto-size would take much longer[*] than just checking the single cell
length, as I wrote above.

[*] To be pedantic, we could also cache row and columns best widths and
    only recompute those that really changed, but this would be even more
    complex.

GC> > There are also operations affecting all cells, such as "Edit class/case",
GC> > but those are presumably less frequent than editing individual cells, so
GC> > perhaps it's not as annoying that they take so long.
GC> 
GC> Instead of restricting the annoyance to certain cases, we should
GC> eliminate it globally.

 As I tried to explain it above, I'm not so sure about this logic. You
might train your muscle memory to hit Ctrl-] after entering long strings as
it would work just fine for small censuses and then still be annoyed when
you hit it without thinking in a big one and spend the next minute or two
waiting until the program comes back to its senses.

 More I think about this, more I become sure that we really should put some
time limit on auto-sizing wxGrid, whatever else we do, as it just shouldn't
be possible for it to take arbitrarily long...

GC> But auto-re-sizing is simply a misfeature, and that's all we need to
GC> know.

 I'm not arguing for arguing sake, but I don't really understand this. If
it's a misfeature, it should be eliminated entirely. But I don't think it
is, and you don't seem to be committed to eliminating it neither, as you'd
still like to allow auto-resizing the columns by pressing Ctrl-].

GC> It's curious that lmi today offers this choice:
GC>  - change all column widths to fit
GC>  - change all column widths to a fixed, hardcoded number
GC> but not this one, which seems obviously desirable:
GC>  - leave columns widths as they are
GC> I.e., you can toggle from auto-re-size-always to reset-to-fixed-width,
GC> but you can't toggle from auto-re-size-always to stop-auto-resizing.

 To make this more consistent, we would need to have a checkbox in the menu
and a togglable toolbar item, but we don't seem to want to do this, as you
want to go away from "auto-resizing mode" as a noun. But if it should be
"auto-resize" as a verb, then it would make sense for there to be just a
single menu command/toolbar button called "Fit column sizes to their
contents" (but shorter).

 At the risk of wandering off into metaphysical territory, it doesn't make
sense to have a UI element corresponding to "leave columns width as they
are" because this can be accomplished by simply not having any special UI
element for this and not doing anything.


GC> > GC> Does that answer all your questions?
GC> > 
GC> >  It almost does. But, to be sure that I understood correctly, let me
GC> > confirm which UI operations precisely will still need to auto-size the
GC> > columns:
GC> > 
GC> > - Toolbar/menu command "Varying column widths" will do it, of course.
GC> 
GC> Yes. And autosize_columns_ is false by default, so the speed penalty
GC> never arises unless the end user explicitly changes it.
GC> 
GC> It's a desirable optional behavior. But if it's painfully slow,
GC> we should either make it fast enough, or remove the option.

 Personally I think that making it fast is a much better option.

GC> > - Editing/adding/deleting cells will not do it, according to your answer,
GC> >   so there is no need to optimize doing it.
GC> 
GC> Agreed.
GC> 
GC> Unless, of course, you find a way to make it fast as lightning.

 For editing/adding, yes, we definitely can make it O(1). For deleting,
this is obviously not possible in general, i.e. if we want to make it 100%
precise (== "correct" in my initial message), but we could approximate it
or just not do anything in this particular case.

GC> > - But what about the operations affecting all cells (and so already
GC> >   taking O(N) time), such as editing class/case or pasting census
GC> >   from clipboard, should they still resize the columns to fit their
GC> >   contents?
GC> 
GC> No.

 OK, thanks.

GC> AFAICS, CensusView::autosize_columns_ is just a handle for the
GC> wxDVC control's wxCOL_WIDTH_AUTOSIZE flag. A brand-new wxGrid-based
GC> census manager could do something different.

 Yes, of course. Right now it behaves in the same way as wxDVC version
does, but I'll change it not to do auto-sizing automatically at all for
now. I hope we can return to this when the initial PR is merged, however,
to implement this, IMHO quite useful, functionality in a reasonably
efficient way.

 So, to summarize: in the initial version there will be no automatic
auto-sizing at all. I'll still change wxGrid to not spend too long in its
AutoSize() because this can be too annoying even if it's triggered
manually, but this won't affect lmi code complexity in any way.

 Of course, please let me know if you disagree with anything here. TIA!
VZ

pgpWhPVH8VQwV.pgp
Description: PGP signature