Re: goocanvasmm: Item::get_items_at API

["Jonathon Jongsma" <jonathon quotidian org>]

On 3/11/08, Armin Burgmeier <armin arbur net> wrote:
> On Tue, 2008-03-11 at 12:56 -0500, Jonathon Jongsma wrote:
>  > On 3/11/08, Armin Burgmeier <armin arbur net> wrote:
>  > > I believe the current implementation of Item::get_items_at is wrong:
>  > >
>  > >  #m4 _CONVERSION(`GList*',`Glib::ListHandle< Glib::RefPtr<Item> >',`
>  > >  $2($3, Glib::OWNERSHIP_NONE)')
>  > >
>  > >  _WRAP_METHOD(Glib::ListHandle< Glib::RefPtr<Item> > get_items_at(double
>  > >  x, double y, const Cairo::RefPtr<Cairo::Context>& context, bool
>  > >  is_pointer_event, bool parent_is_visible, Glib::ListHandle<
>  > >  Glib::RefPtr<Item> >& found_items), goo_canvas_item_get_items_at)
>  > >
>  > >  Since the ownership of the returned ListHandle is set to NONE, the
>  > >  actual GList* will be leaked. However, if we set shallow ownership, then
>  > >  we get memory corruption because in the C API, the returned GList* is
>  > >  meant to be the same as the one passed in, with perhaps some items added
>  > >  to the front and thus returning a new list head.
>  > >
>  > >  If I get it right, then the ownership of the found_items ListHandle
>  > >  needs to be set to none, and a new ListHandle with shallow ownership
>  > >  needs to be returned. We can't do this though, probably, because the
>  > >  ownership of the ListHandle is private.
>  > >
>  > >  I think the C++ way to handle this is to use an insert iterator such as
>  > >
>  > >  template<typename InsertIter>
>  > >  InsertIter get_items_at(double x, double y, const
>  > >  Cairo::RefPtr<Cairo::Context>& context, bool is_pointer_event, bool
>  > >  parent_is_visible, InsertIter iter);
>  > >
>  > >  to be used like this:
>  > >
>  > >  std::vector<Glib::RefPtr<Item> > items;
>  > >  some_item->get_items_at(..., std::inserter(items, items.end()));
>  > >
>  > >  This is also more powerful because items can be inserted anywhere
>  > >  instead of just at the beginning. However, I don't think other *mm
>  > >  projects do anything similar, which is why I am asking for opinions
>  > >  first.
>  > >
>  > >  The corresponding vfunc still needs special consideration then, since
>  > >  virtual functions cannot be templatized. Probably it is enough to just
>  > >  pass a specific container, such as std::vector, since the only code
>  > >  calling it is goocanvasmm itself anyway.
>  > >
>  > >  Any opinions? Do you think it is OK to change it this way, or do you
>  > >  have other ideas to tackle this?
>  > >
>  > >  Armin
>  >
>  > I haven't had time to read this proposal thoroughly yet, but I just
>  > wanted to mention that when I was wrapping this function originally, I
>  > thought it felt like a pretty poor API at the C level. So maybe it
>  > would be best to try to improve the API at the C level before
>  > attempting workarounds in the wrapper.  That said, I'll try to read
>  > your proposal more thoroughly when I get a little more time and offer
>  > comments as well.
>
>
> The C API is the same as the g_list_* API. g_list_prepend, g_list_sort
>  etc. behave the same way, so it's probably convenient for C programmers
>  this way.

I wasn't talking about the g_list api, I was talking about the
get_items_at() API.  If I remember correctly, the way you're supposed
to implement this function is that you're supposed to call
get_items_at() on all of your children, passing in the list of items
you've found so far as the 'found_items' parameter, and then the
children will maybe modify that list to add any further items that
they find, and then this updated list will be returned from the
function.  But this list that is passed to children can be modified,
so there's no guarantee that what you passed in as the 'found_items'
parameter is still valid after calling child.get_items_at() (Maybe the
child will even delete the items you've already found so far from your
list -- unlikely, but there's no guarantee that it can't happen).

Or, consider an item that has multiple children:

get_items_at (..., GList* found_items)
{
  GList* all_found = child1.get_items_at (..., found_items);
  // the call to child1.get_items_at() might have modified
found_items, or it might not have
  all_found = child2.get_items_at (..., found_items);
  // Oops, if child1 didn't modify the passed param 'found_items',
we've just essentially erased the items found by child1 because we
passed the original found_items param to child2.  We should have
passed the all_found param to child2
}

The way it's usually done in goocanvas is to just use the same
variable as the parameter and the return value, e.g.:

get_items_at (..., GList* found_items)
{
  found_items = child1.get_items_at (..., found_items);
  found_items = child2.get_items_at (..., found_items);
}

But, as I said before, this just feels like a poor interface and is
prone to confusion about what's actually going on.

I don't really understand the need to pass the found_items parameter
to the child items in the first place.  Why does the child item need
to know which of its parent items were already found at the given
coordinate?  In my opinion, it would be better just to leave that
parameter out, and just return a GList* of items found at your level
or below, Then the parent would be responsible for combining these all
together and returning that to its parent.   Or maybe a totally
different approach would be better...

-- 
jonner