Re: [lmi] How InputSequenceEditor::remove_row() works

[Vadim Zeitlin]

On Sat, 23 Mar 2019 18:20:21 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-23 16:42, Vadim Zeitlin wrote:
GC> > On Fri, 22 Mar 2019 21:08:27 +0000 Greg Chicares <address@hidden> wrote:
GC> 
GC> [...deleting one row from a two-dimensional array...]
GC> 
GC> > GC> I'd point out how trivial this would be in APL, but
GC> 
GC> [...until the wxAPL port is completed...]
GC> 
GC> >  I don't believe that even the power of APL would help to deal with the
GC> > poor wxSizer API here. The real problem is that it doesn't provide
GC> > something like erase(begin, end) method that would allow doing what this
GC> > code does without using an explicit loop and, more generally, that it uses
GC> > indices instead of iterators. I think it's too late to change the latter,
GC> > but I think it would be generally useful to do something about the former.
GC> 
GC> I don't understand.
GC> 
GC>   https://docs.wxwidgets.org/3.0/classwx_sizer.html
GC> 
GC>   wxSizerItemList& wxSizer::GetChildren() const
GC>   // The elements of type-safe wxList wxSizerItemList are pointers to
GC>   // objects of type wxSizerItem.
GC> 
GC> I take that to mean that there's a data member of type wxList<wxSizerItem>
GC> that contains all the sizer-items, and we can get a writable reference to
GC> it. Then can't we get iterators like std::list::begin(), and erase ranges
GC> of iterators as with std::list::erase(), can't we?

 I admit I just haven't thought of doing it like this because I simply
completely forgot that wxSizer provided a non-const GetChildren() overload
(which is a mistake in the first place IMO). However now that I do look at
it, just erasing the elements from this list won't be sufficient as this
won't destroy the windows, as we want to do here.


GC> The way I understand it, no sizer should own or delete a wxWindow,

 Yes, this is definitely correct.

GC> so I was thinking along these lines instead:
GC> 
GC>     int index = row * Col_Max;
GC>     // new function to get a subrange of items:
GC>     auto& a_row_of_items = sizer_->GetItems(index, index + Col_Max);
GC>     for(wxSizerItem& x : a_row_of_items) {x.DeleteWindows()}
GC> 
GC> According to the comment just added in HEAD:
GC> 
GC>     sizer_->Detach(index); // Superfluous--Destroy() does this anyway.
GC>     win->Destroy();
GC> 
GC> we shouldn't need to call any non-const member of wxSizer here;
GC> we just delete the wxWindows, and the sizer adapts to the deletion.

 And so is this.

GC> In this case, we have a grid sizer:
GC>     wxFlexGridSizer* sizer_;
GC> to which a member like this might be added:
GC>   wxSizerItemList& wxGridSizer::GetChildrenInRow(int row_number);
GC> and then we could write:
GC> 
GC>     auto& a_row_of_items = sizer_->GetChildrenInRow(int row_number);
GC>     for(wxSizerItem& x : a_row_of_items) {x.DeleteWindows();}
GC> 
GC> or, eliminating the intermediate variable:
GC> 
GC>     for(wxSizerItem& x : sizer_->GetChildrenInRow(int row_number))
GC>       {x.DeleteWindows();}

 This indeed looks reasonably nice, but now it has become specific to
wxGridSizer. And I think in this case it could be better to just add
wxGridSizer::DeleteRow(int) because it looks like deleting the windows is
going to be the only useful thing to do with the return value of
GetChildrenInRow() anyhow and DeleteRow() could be more efficient about it
by coalescing all the deletions in a single operation. But of course now
the proposed new API has become completely specific to lmi use case and I'm
not so sure if it's a good idea to add it any more.

 So, after all this discussion, I'm probably not going to do anything for
now, but just keep this in mind if I encounter similar code in the future
and if I can find some API that would be sufficiently general to be useful
both to lmi and some other program using wxSizers, then I'd return to this
and implement it.

 Or I could add the initially propose wxSizer::Destroy(int begin, int end)
which seems reasonably generic. As you can see, I'm still undecided, even
though I waited for a few days just to see if my mind would make itself up
subconsciously. But doing nothing looks like a wiser decision in the case
of such uncertainty, so this is what's going to happen, at least for now
and at least if you don't give me any other arguments for doing something
else.

 Sorry for all these words without any conclusion,
VZ