GNOME Bugzilla – Bug 760353
Add option to include/exclude markers/layers when exporting to PNG
Last modified: 2016-01-18 17:36:14 UTC
When exporting the champlain view to cairo surface one can send a boolean to determine wether to include markers in the exported surface or not. We could expose this to the user, so that one can have PNG with or without our markers and route layers and userlocation markers and stuff. this would be in the exportViewDialog.js and export-view-dialog.ui code at least.
Link to champlain view: https://developer.gnome.org/libchamplain/unstable/ChamplainView.html#champlain-view-to-surface
Created attachment 318648 [details] [review] exportViewDialog: Add option to include/exclude markers/layers Add a checkbox in the export view dialog to allow the user to decide whether or not to include markers/layers in the export.
Review of attachment 318648 [details] [review]: Thanks Alex, looks great! For the commit message, just "layers" will do I think. ::: data/ui/export-view-dialog.ui @@ +121,3 @@ </child> + <child> + <object class="GtkCheckButton" id="includeMarkersCheckButton"> maybe just layersCheckButton? @@ +122,3 @@ + <child> + <object class="GtkCheckButton" id="includeMarkersCheckButton"> + <property name="label" translatable="yes">Include Markers and Route</property> I think we need to do better with this label. When we include layers in export we will include everything we paint upon the map tiles. That will be route and markers, yes, but can be other stuff as well. Hmm, we need a phrase. Any suggestions? "[x] Include layers and markers" ? Not sure, we might need designer input, maybe you could ask andreasn on #gnome-maps? Or in general in #gnome-design? ::: src/exportViewDialog.js @@ +61,3 @@ this.parent(params); + this._includeMarkersAndLayers = true; Is this needed? @@ +62,3 @@ + this._includeMarkersAndLayers = true; + this._includeMarkersCheckButton.set_active(true); Set this from the ui file. <property name="active">True</property> @@ +182,3 @@ + _includeMarkersChanged: function() { + this._includeMarkersAndLayers = this._includeMarkersCheckButton.get_active(); + this._surface = this._mapView.view.to_surface(this._includeMarkersAndLayers); Just use .get_active() directly here? And we should update the preview right? Force a redraw of the drawing area, check the docs.
Review of attachment 318648 [details] [review]: ::: data/ui/export-view-dialog.ui @@ +122,3 @@ + <child> + <object class="GtkCheckButton" id="includeMarkersCheckButton"> + <property name="label" translatable="yes">Include Markers and Route</property> Or maybe we just invert it? "[ ] Export map only" or similar?
Created attachment 318669 [details] [review] exportViewDialog: Add option to include/exclude markers/layers Add a checkbox in the export view dialog to allow the user to decide whether or not to include markers/layers in the export.
Made the changes, apart from the label change. I don't think "Export map only" explains what is being excluded very well. I'll have a think. The same surface is used for the preview and the export, so re-exporting the view to a surface updates the preview.
(In reply to Alex Anthony from comment #6) > Made the changes, apart from the label change. I don't think "Export map > only" explains what is being excluded very well. I'll have a think. > > The same surface is used for the preview and the export, so re-exporting the > view to a surface updates the preview. Sure the same surface is used. But we need to redraw the previewWidget. See the videos I post, the first is without redraw and the second is with a this._previewArea.queue_draw(); After the export.
Created attachment 318674 [details] Cast of checkbox without queue_draw
Created attachment 318675 [details] Cast of checkbox with queue_draw
Review of attachment 318669 [details] [review]: Thanks! For the commit message and body it should just be layers. For the user it is tricky. Talking about code it is clear. We include/exclude layers. Markers are part of MarkerLayer. ::: src/exportViewDialog.js @@ +178,3 @@ + + _includeLayersChanged: function() { + this._surface = this._mapView.view.to_surface(this._layersCheckButton.get_active()); Add this._previewArea.queue_draw(); here to trigger the draw-callback that redraws the preview widget from this._surface.
(In reply to Alex Anthony from comment #6) > Made the changes, apart from the label change. I don't think "Export map > only" explains what is being excluded very well. I'll have a think. Yeah, maybe we will have to defer to our designer Andreas for this one. Andreas, see cast, what should be the label on the checkbox you feel?
That's really strange, for me the preview was updating anyway. Anyway, another iteration coming with that added.
Created attachment 318680 [details] [review] exportViewDialog: Add option to include/exclude layers Add a checkbox in the export view dialog to allow the user to decide whether or not to include layers in the export.
Review of attachment 318680 [details] [review]: Nice! ::: data/ui/export-view-dialog.ui @@ +122,3 @@ + <child> + <object class="GtkCheckButton" id="layersCheckButton"> + <property name="label" translatable="yes">Include Markers and Route</property> Maybe only "[x] Include markers", is ok? the route line is kind of a marker and everything else is.
Created attachment 319112 [details] screenshot I get this black box under the map preview. Apart from that it looks good to me.
It's actually a bit hard to see the actual Marker on the preview, since the preview is so tiny.
So I get the black box any time the layout of the map view before trying to export is too wide. If the ratio of the map view is wider than the ratio of the map preview, the surface is too small so it gets filled with black. This happened before this change, but is worse now because the dialog is taller. I think the logic on setting up the preview size needs changing a bit. Does it want to be included in this bug, or in a separate one?
(In reply to Alex Anthony from comment #17) > I think the logic on setting up the preview size needs changing a bit. Does > it want to be included in this bug, or in a separate one? I've created the ticket for that if you want to take a look: https://bugzilla.gnome.org/show_bug.cgi?id=760760
Review of attachment 318680 [details] [review]: ::: data/ui/export-view-dialog.ui @@ +122,3 @@ + <child> + <object class="GtkCheckButton" id="layersCheckButton"> + <property name="label" translatable="yes">Include Markers and Route</property> I don't know, I don't see the route as a marker, maybe is more understandable the current wording. One think I would do is to lowercase the words "Markers" and "Route". What about geojson? those geometries are considered markers as well? ::: src/exportViewDialog.js @@ +178,3 @@ + + _includeLayersChanged: function() { + this._surface = this._mapView.view.to_surface(this._layersCheckButton.get_active()); If we are going to do this, I prefer to not send the "surface" parameter in the constructor and use this._mapView directly.
Review of attachment 318680 [details] [review]: ::: data/ui/export-view-dialog.ui @@ +122,3 @@ + <child> + <object class="GtkCheckButton" id="layersCheckButton"> + <property name="label" translatable="yes">Include Markers and Route</property> Sure, I would consider anything drawn on the map a marker in this case.
(In reply to Damián Nohales from comment #18) > (In reply to Alex Anthony from comment #17) > > I think the logic on setting up the preview size needs changing a bit. Does > > it want to be included in this bug, or in a separate one? > > I've created the ticket for that if you want to take a look: > > https://bugzilla.gnome.org/show_bug.cgi?id=760760 Does this fix the general case of having a black fill-out?
Attachment 318680 [details] pushed as 6482845 - exportViewDialog: Add option to include/exclude layers