Iterator interface for accessing path data
Submitted by Jonathon Jongsma @jjongsma
Assigned to Murray Cumming
Description
Currently there's no way to access the path data without reverting to the C struct and mucking around with pointers. I'd like to have a more STL-like iterator interface for this (which is also recommended in the Cairo bindings guidelines: http://www.cairographics.org/manual/bindings-path.html). I've been playing around with a few ideas and hope to get some feedback on them. For background on the underlying C type, see http://www.cairographics.org/manual/cairo-Paths.html#cairo-path-data-t
I'm attaching a patch and a tarball that contains a new example program to demonstrate the new API. This isn't polished by any means and I there may still be large problems with it (and there almost definitely are many small problems with it).
Any feedback would be greatly appreciated.
Version: CVS HEAD
Activity
@jjongsmasubmitted a patch:Patch 5608, "patch to add Path iterator interface":
cairomm-path-iter.patch
@jjongsmauploaded an attachment:Attachment 5609, "New example program to demonstrate new API":
cairomm-path-iter-newfiles.tar.gz
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)
@jjongsmasaid:(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.
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.
@jjongsmasaid: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.@jjongsmasaid:(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)
