Re: [lmi] How InputSequenceEditor::remove_row() works [Was: master 78a4de0 5/7: Enable '-Wnull-dereference'; fix the issues it flags]

[Greg Chicares]

On 2019-03-23 16:42, Vadim Zeitlin wrote:
> On Fri, 22 Mar 2019 21:08:27 +0000 Greg Chicares <address@hidden> wrote:

[...deleting one row from a two-dimensional array...]

> GC> I'd point out how trivial this would be in APL, but

[...until the wxAPL port is completed...]

>  I don't believe that even the power of APL would help to deal with the
> poor wxSizer API here. The real problem is that it doesn't provide
> something like erase(begin, end) method that would allow doing what this
> code does without using an explicit loop and, more generally, that it uses
> indices instead of iterators. I think it's too late to change the latter,
> but I think it would be generally useful to do something about the former.

I don't understand.

  https://docs.wxwidgets.org/3.0/classwx_sizer.html

  wxSizerItemList& wxSizer::GetChildren() const
  // The elements of type-safe wxList wxSizerItemList are pointers to
  // objects of type wxSizerItem.

I take that to mean that there's a data member of type wxList<wxSizerItem>
that contains all the sizer-items, and we can get a writable reference to
it. Then can't we get iterators like std::list::begin(), and erase ranges
of iterators as with std::list::erase(), can't we?

> Unfortunately it's not as simple as just adding another Remove() overload
> because we need to destroy the window as well, but adding Destroy() family
> of methods similar to the existing Remove/Detach ones and including an
> overload working with a range could be a good solution.
> 
>  And then the lmi code could become just
> 
>         int index = row * Col_Max;
>         sizer_->Destroy(index, index + Col_Max);
> 
> which would be much simpler to understand.

Yes, it would greatly simplify this particular case. However...

>  What do you think?

The way I understand it, no sizer should own or delete a wxWindow,
so I was thinking along these lines instead:

    int index = row * Col_Max;
    // new function to get a subrange of items:
    auto& a_row_of_items = sizer_->GetItems(index, index + Col_Max);
    for(wxSizerItem& x : a_row_of_items) {x.DeleteWindows()}

According to the comment just added in HEAD:

    sizer_->Detach(index); // Superfluous--Destroy() does this anyway.
    win->Destroy();

we shouldn't need to call any non-const member of wxSizer here;
we just delete the wxWindows, and the sizer adapts to the deletion.

In this case, we have a grid sizer:
    wxFlexGridSizer* sizer_;
to which a member like this might be added:
  wxSizerItemList& wxGridSizer::GetChildrenInRow(int row_number);
and then we could write:

    auto& a_row_of_items = sizer_->GetChildrenInRow(int row_number);
    for(wxSizerItem& x : a_row_of_items) {x.DeleteWindows();}

or, eliminating the intermediate variable:

    for(wxSizerItem& x : sizer_->GetChildrenInRow(int row_number))
      {x.DeleteWindows();}