Re: goocanvasmm: Item::get_items_at API

[Armin Burgmeier <armin arbur net>]

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.

Armin