Created attachment 5608 [details] [review]
patch to add Path iterator interface

Created attachment 5609 [details]
New example program to demonstrate new API

Looks sensible, though I haven't thought about it in depth. These things are
never nice to implement, but we are lucky that the cairo developers have told us
what they want from the binding, and your API seems to be what is suggested.

I assume that this API is for examining the elements and their points, and _not_
for changing anything? We should maybe confirm that that is the intent of the C API.


Minor nitpicks:

These should be const:
Point operator[](unsigned int idx);
int size(void);
ElementType type(void);

And I generally use () instead of (void)

(In reply to comment #3)
> I assume that this API is for examining the elements and their points, and _not_
> for changing anything? We should maybe confirm that that is the intent of the
C API.

Yes, that's what I had assumed.  In Owen Taylor's recent response to the Path
RefPtr thread on the cairo mailing list he had referred to a path as an
'immutable type', so I assume that this is the intent of the C API.  

I really don't have a good idea about when the path data would be useful,
frankly, but I want to try to get complete coverage of the C API.

> Minor nitpicks:
> 
> These should be const:
> Point operator[](unsigned int idx);
> int size(void);
> ElementType type(void);
> 
> And I generally use () instead of (void)

Thanks.  One other thing that I'd like your opinion on since you have much more
experience than I do at wrapping C libraries:

This new API sort of implies that a Path is a container containing a set of
Path::Element objects.  So de-referencing a Path::iterator returns a reference
to a Path::Element object.  I haven't done much with custom iterators before,
but I would assume that in normal code (i.e. if you weren't constrained by
wrapping a C object), you'd have the iterator contain a pointer-to-Element and
have that interal pointer increment to point at the next Element in the list
when the iterator is advanced.  But I don't have a list of Element objects that
I can point at; I only have the underlying C array.

What I did instead was have Path::iterator own an instance of Element.  Since
Element has a data member which is a pointer to an item within the underlying C
array, I simply change this member to point at the next item in the underlying
array whenever the iterator is advanced (using the Element::reset() function). 
Then when I dereference the iterator, I return a reference to this object.  So
there's only one Element object ever instantiated, it just changes what it's
pointing at.

I'm not completely happy with this approach, but couldn't come up with a better
one.  Do you have any concerns / thoughts / ideas about it?  I thought about
trying to actually create a vector of Element objects, but I'm not sure whether
that's a good idea or not.

One big open question here is copying an Element -- should we allow copying? 
should the copy of Element point to the same underlying address as the original?
 or should the copy have its own copy of the data?  And if so, how would that
copied data get freed? Elements aren't responsible for freeing the data they
wrap; the path is all freed at once with cairo_path_destroy() (which is called
from ~Path()).  

Which brings up another question -- what if the Path object gets deleted while
we still have references to Element around?  they would now be pointing at
invalid data.

> One big open question here is copying an Element -- should we allow copying? 

I don't think we need to, and we could add it later.

> what if the Path object gets deleted while we still have references to Element 
> around?  they would now be pointing at invalid data.

I think this is OK. It's like dereferencing an iter to a std::list and expecting
to use it after you have deleted the std::list.

I'll try to take another look soon.

Were you still planning to review this in more depth at some point, Murray?

I'd pre-declare the cairo_path_data_t struct, to avoid including cairo.h in the
public header.

And I'd add a comment next to the Path::Element::Point struct saying that we are
reimplementing it just so we don't have to include the public header (and it
wouldn't be efficient to use accessor functions for such a simple struct).

The use of Path::Element::reset() does seem bad. For instance:
const Path::Element& element = *iter;
++iter;
//Now element has changed, but I didn't expect that.

what if I change Path::iterator::operator*() from 
      inline reference operator*() { return m_node; }
to
      inline value_type operator*() { return m_node; }
?

Granted, in general, you'd expect a reference from operator*(), but this is such
a rarely-used part of the API that I don't really feel too bad about breaking
with convention slightly to make the implementation easier.  But maybe you feel
differently.

I've not been able to come up with another way to do it elegantly.  The only
thing I've come up with so far is creating an independent std::list<Element>
that's a member of Path and whose members refer to the underlying
cairo_path_data_t pointers.  Then we could make use of the
std::list<Element>::iterator type and we'd also have a bunch of Element objects
that are already instantiated, so we could return references to them.  But this
seems like an unnecessary amount of overhead for this API.

I think this needs to wait for a next version of cairomm's API, such as a
cairomm 1.1/1.2, so we can freeze the 1.0 API now.

I'd like cairomm to follow the GNOME release schedule.

(In reply to comment #9)
> I think this needs to wait for a next version of cairomm's API, such as a
> cairomm 1.1/1.2, so we can freeze the 1.0 API now.
> 
> I'd like cairomm to follow the GNOME release schedule.

That's fine.  This interface isn't quite ready yet anyway.  (By the way, we're
already at 1.1/1.2.  We bumped the version up to 1.1.10 for the last release to
indicate that we were wrapping the new cairo 1.1/1.2 API.  So the next one would
be 1.3/1.4 I guess)

jonner 
Do you still experience this issue with newer drivers ?
Please check the status of your issue.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairomm/issues/2.