GNOME Bugzilla – Bug 760351
Have MarkerLayer implement ChamplainExportable
Last modified: 2016-01-12 19:45:38 UTC
Markers are tricky, since they can be implemented in many different ways. Let's make the MarkerLayer exportable, but only have a surface if it contains marker(s) that implement ChamplainExportable. That way one can create exportable markers and have them propagate to an exported view.
Created attachment 318569 [details] [review] MarkerLayer: Implement ChamplainExportable interface Make MarkerLayer exportable. Will contain non-NULL surface iff a marker in the layer implements ChamplainExportable interface.
Created attachment 318570 [details] [review] ChamplainPoint: Implement ChamplainExportable interface
Review of attachment 318569 [details] [review]: Kind of hard for me to verify because I'm not familiar with the cairo or clutter APIs But it works fine. ::: champlain/champlain-marker-layer.c @@ +123,3 @@ + break; + + extra newline @@ +204,3 @@ + cairo_surface_t *surface) +{ + /* no need */ Should you add a warning message about this being unimplemented?
Review of attachment 318570 [details] [review]: Patch is fine, but why don't we implement ChamplainExportable for ChamplainMarker rather its child class ChamplainPoint. Can't/shouldn't ChamplainPoint and its sibling ChamplainLabel share a ChamplainExportable implementation?
Review of attachment 318570 [details] [review]: So a ChamplainMarker is a ClutterActor with ClutterContent set: https://developer.gnome.org/clutter/stable/ClutterContent.html The content can be different things. For a ChamplainPoint it is a simple cairo drawing, that we can export to a surface easily by taking a ref to that surface. The ChamplainLabel is a composite actor that contains label/shadow/background and can be created from image. It is harder to convert that to a cairo_surface, I will give it a try sometime soon. And then users can subclass a ChamplainMarker and do whatever with it really. And then the base class will have no idea how to turn that content into a cairo_surface. There is no magic way of doing this. To read more about this, on the original bug where we implemented export to cairo surface, Emmanuele Bassi, maintainer of Clutter commented on the constraints of doing this: https://bugzilla.gnome.org/show_bug.cgi?id=757350#c11 So this is way there is no generic implemention of ChamplainExportable.
Review of attachment 318569 [details] [review]: ::: champlain/champlain-marker-layer.c @@ +123,3 @@ + break; + + Thx! @@ +204,3 @@ + cairo_surface_t *surface) +{ + /* no need */ Not sure, I do not really think it is needed. This is a weird cross of public/private API so that you kind of need to know what you are doing here. Jiri, what do you think?
Just had a look at the patches and they look good to me (maybe just remove the extra line). I think the empty set_surface() is fine as it is.
(In reply to Jiri Techet from comment #7) > Just had a look at the patches and they look good to me (maybe just remove > the extra line). I think the empty set_surface() is fine as it is. Thanks! Ok to push with that fix?
(In reply to Jonas Danielsson from comment #8) > (In reply to Jiri Techet from comment #7) > > Just had a look at the patches and they look good to me (maybe just remove > > the extra line). I think the empty set_surface() is fine as it is. > > Thanks! Ok to push with that fix? Sure, go ahead.
Review of attachment 318569 [details] [review]: Pushed with fixes
Review of attachment 318570 [details] [review]: Pushed.