GNOME Bugzilla – Bug 746790
Printing of the route with instructions
Last modified: 2016-04-19 10:22:27 UTC
Created attachment 300336 [details] Example of a printed route in Google Maps It would be great if I could print the route, alongside the map, for the tour I want to make. Most of us won't have their Laptop/whatever in the car.
Hi, thanks for creating an account and suggesting a feature! Yes, I think maybe we want this. I am not 100% sure tho. Let's hope a designer can chime in with their opinion. We also need someone willing to work on it. I can provide support and mentor, but I do not own a printer and do not know much about printing API:s in general. Hopefully someone will step up. We have a full 6 month cycle ahead of us :)
A workaround for the API-question would be simply to not provide Printing-Support, but Support for export to a .PDF or .ODT file and let evince/libreoffice/gnome-documents do the dirty rest ;-). ODT would have the advantage, that I can manually edit the file for some comments or so. Although maybe .PDF would seem more professional... Would do you think about the idea?
This would be a really nice feature. Looks like we have two alternatives, do the printing directly with cairo or by generating an HTML and use the webkit2gtk to launch the print operation. The Gtk printing API looks pretty straightforward, the hard thing is the drawing I think.
We discussed this during the first Maps hackfest in spring '13 and at least then felt it was something we wanted. The designs is probably lost, but the basic thought was: Page 1: A printout of the whole route with from-via-to names and total length of travel shown. Page 2-N: zoomed in parts of the route drawn upon a base map with each turn point marked on the map with a number beside it. The travel instructions from the sidebar is then printed on the right with the same numbers printed to the left of them so you're able to scan back and forth. The hard part is finding the right zoom level to show when you're up on a highway (with large distances between turns) vs when you're in a city (with short distances between turns). Anyways these are the thoughts (from memory) from back then. I'd love this feature to be implemented, but I don't have either time or energy to work on it myself unfortunately.
Mattias: Does this resemble the sketches we had back then? https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/print-map.png
(In reply to Andreas Nilsson from comment #5) > Mattias: > Does this resemble the sketches we had back then? > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > maps/print-map.png Something along these lines yeah! We'd need to experiment a bit, maybe we want the map parts more zoomed in so you can get a feel for the surroundings. Not sure.
I thought I'd throw in some input that this should be an essential feature for the application. Most of the time people use maps and routing so they can print out directions. I have my mom using the GNOME desktop and I showed her this application and the first things she asked was "how do I print the route?" I think this would be an excellent feature to add. However, there is the other side of the coin which is people use their smart phones a lot for doing real-time routing
Thanks for commenting! Yes. We want this. The only thing holding this back is developer bandwidth atm. We do not have enough people working on Maps. I would be willing to mentor anyone that want to implement this. Cheers!
Which language is needed? I wish I was more skilled but I'm only a novice at C and Java :(
Maps is written in Javascript, using the gjs bindings to interact with GObjects. But what is needed is more an understanding of how printing works in GTK+ and a drive to figure out how to layout and print the map.
So I had a conversation with Emmanuele Bassi on IRC about this: <jonasdn> ebassi: we have this bug in Maps, https://bugzilla.gnome.org/show_bug.cgi?id=746790, for printing routes. It is also listed as a confirmed idea for Outreachy. Do you have any information to add about how one would print a ClutterActor (like a champlain-view) if it is possible in some way? Or if other paths should be looked at? <•ebassi> jonasdn: You don't "print" ClutterActors, in the same way you don't "print" GtkWidgets :-) <•ebassi> jonasdn: You can print the contents of an actor, assuming you draw to a cairo_t <jonasdn> yeah I understand that <jonasdn> yes, that is what is wanted I guess <•ebassi> jonasdn: But since champlain draws to a CoglTexture, you'd need a separate drawing path that draws to a Cairo context <jonasdn> I see. <•ebassi> There's also the option of dropping drawing to a CoglTexture, and instead using ClutterActor + ClutterCanvas; then you'd need libchamplain to only have code that takes a tile and draws it to a cairo_t* <jonasdn> mm <•ebassi> After that, you'd need two paths in Maps: one that draws to the screen, the other that draws to a surface provided you by GtkPrint* <jonasdn> atm the champlain-image-render, the one that renders tiles... uses clutter_image_new <•ebassi> Ah, that's good <jonasdn> cogl seems to be used for labels and points <•ebassi> Then you're already using Contents :-) <jonasdn> (champlain-label, champlain-point) I do not think we use them atm <jonasdn> in Maps <•ebassi> Basically, you need to replace ClutterImage with a ClutterCanvas, and write the draw() handler that takes the image buffer and draws it on a cairo surface <•ebassi> libchamplain could take the tile image data and load it up in a cairo image surface <jonasdn> and then have that cairo surface and use it to compose the final print cairo surface? <•ebassi> Yes <•ebassi> And also use the same surface to draw on a ClutterCanvas <•ebassi> So you only need one code path in libchamplain <jonasdn> yeah <jonasdn> The alternative would be to have a champlain_tile_to_cr function I guess <•ebassi> jonasdn: That would also work, I guess <•ebassi> The nice bit is that if libchamplain starts talking cairo surfaces then you can do neat things inside ClutterCanvas to draw them with cairo, instead of having a pre-generated tile
I'm interested in implementing this feature.How should I go about implementing this feature?
(In reply to Karanbir Chahal from comment #12) > I'm interested in implementing this feature.How should I go about > implementing this feature? Hi, this bug is the base for an Outreachy project that Amisha will perform. Thanks for your enthusiasm but maybe we can find another task for you?
Yes, PLease. I also want to participate in GSOC 2016 FOR GNOME.Can you please direct me to a another task?
(In reply to Karanbir Chahal from comment #14) > Yes, PLease. I also want to participate in GSOC 2016 FOR GNOME.Can you > please direct me to a another task? Hi, best is to keep watch in the gnome-maps bugzilla product. Maybe watch the gnome-love bugs special? Check out the Maps wiki: https://wiki.gnome.org/Apps/Maps/Contribute Thanks!
Screencast of printing mapview with route and instruction: https://youtu.be/ywWMJ-Vxg6Y Andreas, I need some light on initial UI i.e. how the user should be given access to print feature, some kind of button in sidebar should do i guess.A mockup will be helpful. :) Will upload the Patch of the work done so far, sooner.
I imagine a button in the top bar would do it: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/print-map.png
(In reply to Andreas Nilsson from comment #17) > I imagine a button in the top bar would do it: > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > maps/print-map.png In the headerbar? That only gets active if there is a route?
In case you don't have a route selected, it would just print the active view. I'm happy to rework it to be specific to routes though.
(In reply to Andreas Nilsson from comment #19) > In case you don't have a route selected, it would just print the active view. > I'm happy to rework it to be specific to routes though. This is new information for me actually :) I never saw this as a "print current view" feature. I saw it as a specific "Print route" feature. Hmm, sure, a print icon in header-bar. Will it not be confusing that it does different things depending on if there is a route somewhere on the map?
Yeah I agree with you Jonas. Maybe we can make the button invisible when there is no route.
If there is only for routes, I would prefer to have the print button in the sidebar. I think that's more obvious.
After going back and forth with Jonas a bit on IRC, printing of non-routes would be outside the scope of this bug. I think the print button will end up in the sidebar somehow. Just needs to get it right with all the other things in there.
Created attachment 318939 [details] [review] Temporary
Updated the mockup with how the print button should behave: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/print-map.png * Initial state: no print button * When sidebar empty: no print button * When a route search have been made: show print button * When closing sidebar: hide print button
Created attachment 319010 [details] [review] Temporary
Created attachment 319011 [details] [review] Dividing the data into pages
Review of attachment 319011 [details] [review]: Hi, I commented a bit, please combine these two patches tho, it is a bit hard to comment on both. PAtches should be split on logical changes and this does not appear so. ::: src/printRoute.js @@ +45,3 @@ + instructionSurfaces = []; + mapViewSurfaces = []; this._instructions = []; this._mapSurfaces = []; @@ +61,3 @@ _initPrintRouteView: function() { this._view = new Champlain.View(); + this._view.set_size (1353, 634); Do not use magic numbers, use constants and here init the size in the constructor: this._view = new Champlain.View({ visible: true, width: ..., height: ...}); @@ +66,1 @@ this._view.kinetic_mode = true; No need for this. @@ +66,2 @@ this._view.kinetic_mode = true; this._view.set_map_source(this._mapView.view.get_map_source()); This needs to change, our mapView source might be aerial @@ +82,3 @@ + Application.routeService.route.turnPoints.forEach(function(turnPoint) { + let instructionWidget = new Gtk.OffscreenWindow({ visible: true }); + let instructionEntry = new Sidebar.InstructionRow({ We should move InstructionRow to its own file, it does not belong in Sidebar anymore now that you are using it as well. @@ +98,3 @@ + instructionWidget.connect('damage-event', function() { + instructionSurfaces[y] = instructionWidget.get_surface(); + y += 1; no need for y, just go instructionSurfaces.push(instructionWidget.get_surface()); @@ +110,3 @@ if (this._view.state === Champlain.State.DONE) { this._view.disconnect(notifyId); + mapViewSurfaces[0] = this._view; this.mapSurfaces.push(this._view); @@ +138,3 @@ + PangoCairo.layout_path(cr, layout); + cr.setSourceRGB(1.0,0,0); + cr.fill(); Maybe have a function that creates the headline? @@ +145,3 @@ + height: context.get_page_setup().get_paper_height(Gtk.Unit.POINTS) }; + mapViewSize = { width: 0.8478 * pageSize['width'], + height: 0.3333 * pageSize['height']}; please use the form pageSize.height and pageSize.width, it is the same as pageSize['height']; @@ +147,3 @@ + height: 0.3333 * pageSize['height']}; + instructionSize = { width: 0.4043 * pageSize['width'], + height: 0.0303 * pageSize['height']}; How do you compute these? Why do you not use the actual width/height of the instructions and the mapView? @@ +148,3 @@ + instructionSize = { width: 0.4043 * pageSize['width'], + height: 0.0303 * pageSize['height']}; + margin = (pageSize['width'] -mapViewSize['width'])/2; Where do all magic numbers come from? And please avoid all these global variables, use let for local scope and this._ for class scope. @@ +152,3 @@ + let instructionNumber = { typeA: Math.floor((pageSize['height'] - mapViewSize['height'] - (3 * margin)) / instructionSize['height']), + typeB: Math.floor((pageSize['height'] - (2 * margin)) / instructionSize['height'])}; + What is happening here? what is typeA and typeB? I do not understand at all. Please add a comment explaining what is going on. But even better lets try to find away that makes it more obvious. @@ +155,3 @@ + let page = 0; + let i = 0; + while(i < instructionSurfaces.length-1) off-by-one error? Please use instructions.forEach(function(instruction) { }); @@ +172,3 @@ + page++; + } + } I do not understand what is going on here. @@ +196,3 @@ + + let j = i; + while(j <= this.data[page_num]['instruction']) please avoid these kinds of indexed loops and use forEach instead.
Created attachment 319221 [details] [review] Move InstructionRow class from Sidebar to a new file
Review of attachment 319221 [details] [review]: Thanks! One nit though: Make the commit message: "Move InstructionRow ... " not "Mbved InstructionRow..."
Comment on attachment 319221 [details] [review] Move InstructionRow class from Sidebar to a new file >From e29409308bf32ea0cafe7fa9785db72009b2c0c4 Mon Sep 17 00:00:00 2001 >From: Amisha Singla <amishas157@gmail.com> >Date: Mon, 18 Jan 2016 00:12:26 +0530 >Subject: [PATCH 1/1] Move InstructionRow class to its own file > >--- > src/instructionRow.js | 47 ++++++++++++++++++++++++++++++++++++ > src/org.gnome.Maps.src.gresource.xml | 1 + > src/sidebar.js | 27 +++------------------ > 3 files changed, 51 insertions(+), 24 deletions(-) > create mode 100644 src/instructionRow.js > >diff --git a/src/instructionRow.js b/src/instructionRow.js >new file mode 100644 >index 0000000..c91321a >--- /dev/null >+++ b/src/instructionRow.js >@@ -0,0 +1,47 @@ >+/* -*- Mode: JS2; indent-tabs-mode: nil; js2-basic-offset: 4 -*- */ >+/* vim: set et ts=4 sw=4: */ >+/* >+ * Copyright (c) 2011, 2012, 2013 Red Hat, Inc. >+ * >+ * GNOME Maps is free software; you can redistribute it and/or modify >+ * it under the terms of the GNU General Public License as published by the >+ * Free Software Foundation; either version 2 of the License, or (at your >+ * option) any later version. >+ * >+ * GNOME Maps is distributed in the hope that it will be useful, but >+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY >+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >+ * for more details. >+ * >+ * You should have received a copy of the GNU General Public License along >+ * with GNOME Maps; if not, see <http://www.gnu.org/licenses/>. >+ * >+ * Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org> >+ * Mattias Bengtsson <mattias.jc.bengtsson@gmail.com> >+ */ >+ >+const Gtk = imports.gi.Gtk; >+const Lang = imports.lang; >+const Utils = imports.utils; >+ >+const InstructionRow = new Lang.Class({ >+ Name: "InstructionRow", >+ Extends: Gtk.ListBoxRow, >+ Template: 'resource:///org/gnome/Maps/ui/instruction-row.ui', >+ InternalChildren: [ 'directionImage', >+ 'instructionLabel', >+ 'distanceLabel' ], >+ >+ _init: function(params) { >+ this.turnPoint = params.turnPoint; >+ delete params.turnPoint; >+ >+ this.parent(params); >+ >+ this._instructionLabel.label = this.turnPoint.instruction; >+ this._directionImage.icon_name = this.turnPoint.iconName; >+ >+ if (this.turnPoint.distance > 0) >+ this._distanceLabel.label = Utils.prettyDistance(this.turnPoint.distance); >+ } >+}); >diff --git a/src/org.gnome.Maps.src.gresource.xml b/src/org.gnome.Maps.src.gresource.xml >index 36f4c07..66302ac 100644 >--- a/src/org.gnome.Maps.src.gresource.xml >+++ b/src/org.gnome.Maps.src.gresource.xml >@@ -19,6 +19,7 @@ > <file>geoJSONSource.js</file> > <file>geoJSONStyle.js</file> > <file>http.js</file> >+ <file>instructionRow.js</file> > <file>layersPopover.js</file> > <file>location.js</file> > <file>locationServiceNotification.js</file> >diff --git a/src/sidebar.js b/src/sidebar.js >index 4026344..f20d535 100644 >--- a/src/sidebar.js >+++ b/src/sidebar.js >@@ -28,34 +28,13 @@ const Lang = imports.lang; > const Mainloop = imports.mainloop; > > const Application = imports.application; >+const InstructionRow = imports.instructionRow; > const PlaceStore = imports.placeStore; > const RouteEntry = imports.routeEntry; > const RouteQuery = imports.routeQuery; > const StoredRoute = imports.storedRoute; > const Utils = imports.utils; > >-const InstructionRow = new Lang.Class({ >- Name: "InstructionRow", >- Extends: Gtk.ListBoxRow, >- Template: 'resource:///org/gnome/Maps/ui/instruction-row.ui', >- InternalChildren: [ 'directionImage', >- 'instructionLabel', >- 'distanceLabel' ], >- >- _init: function(params) { >- this.turnPoint = params.turnPoint; >- delete params.turnPoint; >- >- this.parent(params); >- >- this._instructionLabel.label = this.turnPoint.instruction; >- this._directionImage.icon_name = this.turnPoint.iconName; >- >- if (this.turnPoint.distance > 0) >- this._distanceLabel.label = Utils.prettyDistance(this.turnPoint.distance); >- } >-}); >- > const Sidebar = new Lang.Class({ > Name: 'Sidebar', > Extends: Gtk.Revealer, >@@ -222,8 +201,8 @@ const Sidebar = new Lang.Class({ > }).bind(this)); > > route.turnPoints.forEach((function(turnPoint) { >- let row = new InstructionRow({ visible: true, >- turnPoint: turnPoint }); >+ let row = new InstructionRow.InstructionRow({ visible: true, >+ turnPoint: turnPoint }); > this._instructionList.add(row); > }).bind(this)); > >-- >2.4.0 >
Created attachment 319240 [details] [review] Move InstructionRow class from Sidebar to a new file
Review of attachment 319240 [details] [review]: Thanks!
Created attachment 319241 [details] [review] Add Print Route class
Created attachment 319242 [details] [review] Add Print UI
this._height and this._width are original sizes of instructionWidget and it should not be used as array. But it wasn't working out otherwise.Maybe we can find some solution together. And i may require little input on naming conventions.
Review of attachment 319241 [details] [review]: Thanks! Sorry for drive-by review, there will be more, bur right now this is what I got time for. Hopefully Hashem or Damián got time for more? Excellent work! ::: src/printRoute.js @@ +31,3 @@ + +const OriginalPageWidth = 17; +const OriginalPageHeight = 24; Explain this more, what is the unit? And if they are private to the class please prefix with _, and make them CAPS @@ +52,3 @@ + this._operation.connect('begin-print', this.begin_print.bind(this)); + this._operation.connect('draw-page', this.draw_page.bind(this)); + this.vfunc_activate(); using the vfunc_ prefix means that we are overriding a virtual function of a GObjectClass, we are not doing that here, so it is not something we should use. @@ +58,3 @@ + _initPrintRouteView: function() { + let instructions = this._instructions; + let mapSurfaces = this._mapSurfaces; why?why can't we just use the class variables? @@ +60,3 @@ + let mapSurfaces = this._mapSurfaces; + let height = this._height; + let width = this._width; same here @@ +62,3 @@ + let width = this._width; + this._view = new Champlain.View(); + this._view.set_size (this._mapView.view.get_width(), this._mapView.view.get_height()); do not use set_size, init in the constructor. this._view = new Champlain.View({ width: ..., height: ... }); @@ +65,3 @@ + this._view.visible = true; + this._view.zoom_level = this._mapView.view.zoom_level; + this._view.set_map_source(this._mapView.view.get_map_source()); Later on this needs to change @@ +95,3 @@ + instructions.push(widget.get_surface()); + height.push(widget.get_allocated_height()); + width.push(widget.get_allocated_width()); why two arrays? Why can't you just use the width/height of the surface? And I thought we talked about having the instruction always the same height / width? @@ +109,3 @@ + } + else + { format this else like other elses @@ +116,3 @@ + }, + + _printWholeRoute: function(context) { make it take cr instead @@ +123,3 @@ + }, + + _printHeaderLine: function(context) { make it take cr instead @@ +147,3 @@ + + let instructionSize = { width: this._pageRatio.x * (width / context.get_dpi_x()), + height: this._pageRatio.y * (height / context.get_dpi_y())}; Explain this and format it better @@ +152,3 @@ + + this._instructionNumber = { page0: Math.floor((pageSize.height - mapViewSize.height - (3 * this._margin)) / instructionSize.height), + pageN: Math.floor((pageSize.height - (2 * this._margin)) / instructionSize.height)}; Can't we figure this out in draw-page instead? Since you have a if page_num === 0 there in any case, I am also worried that this approach will turn messy later when we do different types of layout depending on route. We do not want to much if statements @@ +154,3 @@ + pageN: Math.floor((pageSize.height - (2 * this._margin)) / instructionSize.height)}; + + operation.set_n_pages(1 + Math.ceil((this._instructions.length -this._instructionNumber.page0 )/this._instructionNumber.pageN)); This is pure magic. Make a function of this and explain what is going on. In general try to make the code as obvoius and easy to read as possible, and if that fails we need comments, but that should be last resort. @@ +168,3 @@ + y += this._margin * context.get_dpi_y(); + + if(page_num == 0) if (page_num ===) { @@ +172,3 @@ + this._printHeaderLine(context); + this._printWholeRoute(context); + y += this._mapView.view.get_height(); This canät be right? What does the mapViews views height got todo with anything? @@ +181,3 @@ + } + + while(i <= j) This wants to be a for loop
Review of attachment 319241 [details] [review]: ::: src/printRoute.js @@ +56,3 @@ + }, + + _initPrintRouteView: function() { And also, what guarantees that this is done when we get to draw-page? And what guarantees that it is not error X's because we failed to load? What happens if network is cut or really slow?
Review of attachment 319242 [details] [review]: Thanks! ::: src/mainWindow.js @@ +265,3 @@ + this._printRouteButton.set_visible(false); + + this._printRouteButton.set_visible(true); No, we should not do it like this, you should use gbindings to make it set visibility automagicly when the routeVisible property of mapView changes. Then all this would become: this._printRoutebutton.bind_property('visible', this._mapView, 'routeVisible', GObject.BindingFlags.DEFAULT); @@ +418,3 @@ + _printRoute: function() { + let print = new PrintRoute.PrintRoute({ mapView: this._mapView }); This should only work when there is a route, right?
Review of attachment 319241 [details] [review]: ::: src/printRoute.js @@ +56,3 @@ + }, + + _initPrintRouteView: function() { what does initPrintRouteView mean? What does thing function do? Can we come up with a better name? @@ +171,3 @@ + { + this._printHeaderLine(context); + this._printWholeRoute(context); I feel the print prefix here is misleading, maybe render? renderHeader() renderMapView() ?
Review of attachment 319241 [details] [review]: ::: src/printRoute.js @@ +25,3 @@ +const Clutter = imports.gi.Clutter; +const Mainloop = imports.mainloop; +const Cairo = imports.cairo; Order these by name @@ +40,3 @@ + + this._mapView = params.mapView; + delete params.mapView; I do not think we actually need this? Right? What do we need our current mapView for? @@ +62,3 @@ + let width = this._width; + this._view = new Champlain.View(); + this._view.set_size (this._mapView.view.get_width(), this._mapView.view.get_height()); And why use the size from the current view? that can vary alot @@ +106,3 @@ + mapSurfaces.push(this._view.to_surface(true)); + } + }).bind(this)); Should not need the idle with latest champlain, right? @@ +126,3 @@ + let cr = context.get_cairo_context(); + let layout = context.create_pango_layout(); + cr.moveTo(20.0,50.0) Magic numbers @@ +127,3 @@ + let layout = context.create_pango_layout(); + cr.moveTo(20.0,50.0) + layout.set_text("From: " + Application.routeService.query.points[0].place.state + " " + "To: " +Application.routeService.query.points[1].place.state, -1); Please use ' ' for non translatable. But ... this should be translated, right? And why place.state? And it should be the last place, right? Not the next one. Something like: let header = '%s %s %s %s'.format(_("From"), query.points[0].place.name, _("to"), query[-1].place.name); layout.set_text(header, -1); ?
I was thinking some while doing the dishes... How would you, and others, feel about introducing some kind of PrintLayout class? That would contain some stuff needed to generate the printed page. Like it could calculate how many mapViews and instructions are needed to be rendered. So that the code would be something like, very rough: press print => // this will decide the layout based on the route lthis._layout = PrintLayout.newLayout(Application.routeService.route); // this signal would fire when all mapViews and instructionRows are complete layout.connect('render-complete', printoperation_begin); // this will render all the cairo surfaces needed layOut.render(); and the begin_print callback would use layout, like layout.numPages property would be the number of pages, and layOut could also contain stuff like margins and other things. What do you think?
(In reply to Andreas Nilsson from comment #25) > Updated the mockup with how the print button should behave: > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > maps/print-map.png > > * Initial state: no print button > * When sidebar empty: no print button > * When a route search have been made: show print button > * When closing sidebar: hide print button A question about the minimaps, should there always be just two? One starting and one finishing? Or could there be an arbitrary amount of minimaps? Should there be two per page?
(In reply to Jonas Danielsson from comment #42) > I was thinking some while doing the dishes... > > How would you, and others, feel about introducing some kind of PrintLayout > class? > That would contain some stuff needed to generate the printed page. > > Like it could calculate how many mapViews and instructions are needed to be > rendered. > > So that the code would be something like, very rough: > > > press print => > // this will decide the layout based on the route > lthis._layout = PrintLayout.newLayout(Application.routeService.route); > > // this signal would fire when all mapViews and instructionRows are > complete > layout.connect('render-complete', printoperation_begin); > > // this will render all the cairo surfaces needed > layOut.render(); > > > > and the begin_print callback would use layout, like layout.numPages property > would be the number of pages, and layOut could also contain stuff like > margins and other things. > > What do you think? I think it's a nice idea as seen from point of code modularity.Plus scalable too, If some new layout also needs to be printed in future, this is going to take care. We can have one class for actual printing i.e. rendering and other one for deciding the layout.
(In reply to Jonas Danielsson from comment #43) > (In reply to Andreas Nilsson from comment #25) > > Updated the mockup with how the print button should behave > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > > maps/print-map.png > > > > * Initial state: no print button > > * When sidebar empty: no print button > > * When a route search have been made: show print button > > * When closing sidebar: hide print button > > A question about the minimaps, should there always be just two? One starting > and one finishing? Or could there be an arbitrary amount of minimaps? Should > there be two per page? I feel like we can keep size of minimap fixed and also the number of minimaps per page. Now what is going to be dynamic is total number of minimaps required. What i thought on dividing is, we can keep a cap on number of instructions which are to be displayed along a mininap. Now we will try to get these instructions plotted on minimap. If they are visible with appropriate zoom level, we can continue for rest of pairs(minimaps and instructions) otherwise, we can start removing instructions from the end of the set and plot the rest and check if they are visible or not. What say?
(In reply to amisha from comment #45) > (In reply to Jonas Danielsson from comment #43) > > (In reply to Andreas Nilsson from comment #25) > > > Updated the mockup with how the print button should behave > > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > > > maps/print-map.png > > > > > > * Initial state: no print button > > > * When sidebar empty: no print button > > > * When a route search have been made: show print button > > > * When closing sidebar: hide print button > > > > A question about the minimaps, should there always be just two? One starting > > and one finishing? Or could there be an arbitrary amount of minimaps? Should > > there be two per page? > > I feel like we can keep size of minimap fixed and also the number of > minimaps per page. Now what is going to be dynamic is total number of > minimaps required. What i thought on dividing is, we can keep a cap on > number of instructions which are to be displayed along a mininap. Now we > will try to get these instructions plotted on minimap. If they are visible > with appropriate zoom level, we can continue for rest of pairs(minimaps and > instructions) otherwise, we can start removing instructions from the end of > the set and plot the rest and check if they are visible or not. What say? Sounds good in general. But I would say the opposite about zoom. Set the zoom-level / view so that the requested number of instructions are visible, using ensure_visible. To start with at least. Then we can experiment with optimal zoom levels. Because it is really tricky. Number of instruction versus actual distance vs utility.
(In reply to Jonas Danielsson from comment #43) > A question about the minimaps, should there always be just two? One starting > and one finishing? Or could there be an arbitrary amount of minimaps? Should > there be two per page? I had imagined one for start and one for stop. Those are usually hardest to navigate as you have to get out and in of neighborhoods with funky layouts on and off big highways. If you're unfamiliar with the layout of the neighborhood, that's extra hard ("Is it this house? What street is supposed to cross it? Where exactly is the entrance to the house?") If there are several pages, it might make sense to put the end on the last page (so you can dismiss the previous ones during the ride, or just put them behind the current sheet). One case where it could make sense to add an extra minimap would be if you had a in-between-stop during the way.
(In reply to Andreas Nilsson from comment #47) > (In reply to Jonas Danielsson from comment #43) > > > A question about the minimaps, should there always be just two? One starting > > and one finishing? Or could there be an arbitrary amount of minimaps? Should > > there be two per page? > > I had imagined one for start and one for stop. Those are usually hardest to > navigate as you have to get out and in of neighborhoods with funky layouts > on and off big highways. If you're unfamiliar with the layout of the > neighborhood, that's extra hard ("Is it this house? What street is supposed > to cross it? Where exactly is the entrance to the house?") > > If there are several pages, it might make sense to put the end on the last > page (so you can dismiss the previous ones during the ride, or just put them > behind the current sheet). > > One case where it could make sense to add an extra minimap would be if you > had a in-between-stop during the way. Yes, maybe. But I think I agree with this. But I think it is not what Amisha is working on? Amisha, so just having two minimaps, ever, one for the x starting instructions and one for the y ending instructions, that would help a bit with the layout, right?
(In reply to Jonas Danielsson from comment #48) > (In reply to Andreas Nilsson from comment #47) > > (In reply to Jonas Danielsson from comment #43) > > > > > A question about the minimaps, should there always be just two? One starting > > > and one finishing? Or could there be an arbitrary amount of minimaps? Should > > > there be two per page? > > > > I had imagined one for start and one for stop. Those are usually hardest to > > navigate as you have to get out and in of neighborhoods with funky layouts > > on and off big highways. If you're unfamiliar with the layout of the > > neighborhood, that's extra hard ("Is it this house? What street is supposed > > to cross it? Where exactly is the entrance to the house?") > > > > If there are several pages, it might make sense to put the end on the last > > page (so you can dismiss the previous ones during the ride, or just put them > > behind the current sheet). > > > > One case where it could make sense to add an extra minimap would be if you > > had a in-between-stop during the way. > > Yes, maybe. But I think I agree with this. But I think it is not what Amisha > is working on? > > Amisha, so just having two minimaps, ever, one for the x starting > instructions and one for the y ending instructions, that would help a bit > with the layout, right? Yeah that would help in the layout. That means ,only few instructions associated with start and stop are to be displayed. And there won't be minimaps to instructions association. Minimaps for FROM-VIA-TO are to displayed on left and all instructions are to be displayed together on right. Did I understand correctly?
(In reply to amisha from comment #49) > (In reply to Jonas Danielsson from comment #48) > > (In reply to Andreas Nilsson from comment #47) > > > (In reply to Jonas Danielsson from comment #43) > > > > > > > A question about the minimaps, should there always be just two? One starting > > > > and one finishing? Or could there be an arbitrary amount of minimaps? Should > > > > there be two per page? > > > > > > I had imagined one for start and one for stop. Those are usually hardest to > > > navigate as you have to get out and in of neighborhoods with funky layouts > > > on and off big highways. If you're unfamiliar with the layout of the > > > neighborhood, that's extra hard ("Is it this house? What street is supposed > > > to cross it? Where exactly is the entrance to the house?") > > > > > > If there are several pages, it might make sense to put the end on the last > > > page (so you can dismiss the previous ones during the ride, or just put them > > > behind the current sheet). > > > > > > One case where it could make sense to add an extra minimap would be if you > > > had a in-between-stop during the way. > > > > Yes, maybe. But I think I agree with this. But I think it is not what Amisha > > is working on? > > > > Amisha, so just having two minimaps, ever, one for the x starting > > instructions and one for the y ending instructions, that would help a bit > > with the layout, right? > > Yeah that would help in the layout. That means ,only few instructions > associated with start and stop are to be displayed. And there won't be > minimaps to instructions association. Minimaps for FROM-VIA-TO are to > displayed on left and all instructions are to be displayed together on > right. Did I understand correctly? I don't know... All instructions in the route are to be displayed, but only minimaps next to the N starting ones and N ending ones is what I understand. Andreas?
(In reply to Jonas Danielsson from comment #50) > > All instructions in the route are to be displayed, but only minimaps next to > the N starting ones and N ending ones is what I understand. > > Andreas? Yes, that's what I had in mind.
Created attachment 319657 [details] [review] Add Print UI
Created attachment 319658 [details] [review] Add Print Route Feature
Review of attachment 319658 [details] [review]: Thanks! When you attach patches like these, that do not even compile and run. Please also state here what level of review you are after and what your questions about the design are. That way more people can help out with review and answering those questions. ANd we need that if we want to get this in to 3.20. I can not do it alone. So do not ask specifically for me on IRC, ask the channel the question and state more questions here in the bug where more people are subscribed! Great work! ::: src/longRouteLayout.js @@ +52,3 @@ + blue: 255, + green: 0, + alpha: 100 }); This seems like a constant that could go on top of the file? const _RouteColor = new ... @@ +72,3 @@ + boundingBoxFrom.push(turnPoints[i].coordinate); + boundingBoxTo.push(turnPoints[turnPoints.length - 1 - i ].coordinate); + } What is happening here? What if there is fewer instructions? This is more complex than this loop, please split into functions! @@ +89,3 @@ + }).bind(this)); + } + else { else always on same line as } @@ +113,3 @@ + } + } + } This far far to much stuff going on in _init, please split out in functions with descriptive names! ::: src/printLayout.js @@ +73,3 @@ +const PrintLayout = new Lang.Class({ + Name: 'PrintLayout', + Extends: GObject.Object, What is the reason you want this to be a GObject? @@ +91,3 @@ + _drawMapView: function() { + let factory = Champlain.MapSourceFactory.dup_default(); + this._mapSource = factory.create_cached_source(Champlain.MAP_SOURCE_OSM_MAPQUEST); This could be MapView.MapType.STREET, right? @@ +158,3 @@ + get NumberOfPages() { + return this._numberOfPages; + }, just pages? get pages() { ... } or numPages @@ +174,3 @@ + get Instructions() { + return this._instructions; + } Do not use CapsLikeThis, please use camelCase instead. ::: src/printOperation.js @@ +32,3 @@ + _init: function() { + PrintLayout.SupportedTypes.push(LongRouteLayout.LongRouteLayout); + this._layout = PrintLayout.newFromRoute(Application.routeService.route); white space damage? @@ +39,3 @@ + + this._layout.connect('render-complete', (function() { + this._layout._calculateNumberOfPages(); never call a private function from outside the class! If you need calculateNumberOfPages here then it is not private. Why is it needed here? @@ +48,3 @@ + let surface = this._layout.MapSurfaces.MapView; + cr.setSourceSurface(surface, x, y); + cr.paint(); whitespace damage? @@ +51,3 @@ + }, + + _renderHeader: function(context, x, y) { Can't this be part of the layout classes? So that we only need to get a surface here? @@ +56,3 @@ + cr.moveTo(x, y); + let query = Application.routeService.query; + let header = '%s %s %s %s'.format(_("From"), query.points[0].place.name, _("To"), query.points[query.points.length -1].place.name); try to keep the rows at 80 chars max @@ +67,3 @@ + _renderMinimapFrom: function(cr, x, y) { + cr.setSourceSurface(this._layout.MapSurfaces.FromView, x, y); + cr.paint(); What is the indentation level here? Please conform to rest of maps in all files and functions @@ +100,3 @@ + let x = 0, y = 0; + x += this._layout.Margin + y += this._layout.Margin; please mind alignment and whitespace! And try to keep the variables declaration on top of the function. @@ +121,3 @@ + + (page_num * this._layout.InstructionsPerPage.pageN); + instructionEndIndex = (this._layout.InstructionsPerPage.page0 - 1) + (page_num * this._layout.InstructionsPerPage.pageN); + } Either break this up to multiple functions or have comments to describe what is going on. It should either be obvious for everybody reading or we need a comment. We want to keep understanding this code for a long time!
Review of attachment 319658 [details] [review]: ::: src/longRouteLayout.js @@ +66,3 @@ + + let boundingBoxFrom = []; + let boundingBoxTo = []; These are not really bounding boxen, right? They are arrays of instructions? @@ +84,3 @@ + this._mapSurfaces.FromView = viewFrom.to_surface(true); + if(viewTo.state === Champlain.State.DONE) { + this.emit('render-complete'); My understanding was that we should only emit this when _all_ rendering is complete? All mapViews and all instructions? @@ +112,3 @@ + this.emit('render-complete'); + } + } You have the same code twice for creating the from and two views? Maybe a good indication to make it a function?
(In reply to Jonas Danielsson from comment #54) > Review of attachment 319658 [details] [review] [review]: > > Thanks! > > When you attach patches like these, that do not even compile and run. Please > also state here what level of review you are after and what your questions > about the design are. That way more people can help out with review and > answering those questions. ANd we need that if we want to get this in to > 3.20. I can not do it alone. So do not ask specifically for me on IRC, ask > the channel the question and state more questions here in the bug where more > people are subscribed! > > Great work! Thanks for the review, Jonas. Yes, will ensure it from next time. The two patches I attached were to be applied together to compile and run together. Maybe I should have kept them together. Sorry. > > ::: src/longRouteLayout.js > @@ +52,3 @@ > + blue: 255, > + green: 0, > + alpha: 100 }); > > This seems like a constant that could go on top of the file? > > > const _RouteColor = new ... > > @@ +72,3 @@ > + boundingBoxFrom.push(turnPoints[i].coordinate); > + boundingBoxTo.push(turnPoints[turnPoints.length - 1 - i > ].coordinate); > + } > > What is happening here? What if there is fewer instructions? This is more > complex than this loop, please split into functions! Yes, some check should be applied to ensure the array doesn't go out of bound. Thanks. > > @@ +89,3 @@ > + }).bind(this)); > + } > + else { > > else always on same line as } Okay. > > @@ +113,3 @@ > + } > + } > + } > > This far far to much stuff going on in _init, please split out in functions > with descriptive names! > Okay. > ::: src/printLayout.js > @@ +73,3 @@ > +const PrintLayout = new Lang.Class({ > + Name: 'PrintLayout', > + Extends: GObject.Object, > > What is the reason you want this to be a GObject? > I read in styleGuide of Gnome, Extends is optional,If we leave it blank, it by default extends the Object. Maybe mistaken. Reference : https://wiki.gnome.org/Projects/Gjs/StyleGuide#Classes > @@ +91,3 @@ > + _drawMapView: function() { > + let factory = Champlain.MapSourceFactory.dup_default(); > + this._mapSource = > factory.create_cached_source(Champlain.MAP_SOURCE_OSM_MAPQUEST); > > This could be MapView.MapType.STREET, right? Oops. Yeah right. > > @@ +158,3 @@ > + get NumberOfPages() { > + return this._numberOfPages; > + }, > > just pages? > > get pages() { ... } > > or numPages okay. numPages will do. Thanks. > > @@ +174,3 @@ > + get Instructions() { > + return this._instructions; > + } > > Do not use CapsLikeThis, please use camelCase instead. Sure > > ::: src/printOperation.js > @@ +32,3 @@ > + _init: function() { > + PrintLayout.SupportedTypes.push(LongRouteLayout.LongRouteLayout); > + this._layout = > PrintLayout.newFromRoute(Application.routeService.route); > > white space damage? > Will rectify. > @@ +39,3 @@ > + > + this._layout.connect('render-complete', (function() { > + this._layout._calculateNumberOfPages(); > > never call a private function from outside the class! If you need > calculateNumberOfPages here then it is not private. Why is it needed here? OKay will make it calculateNumberOfPages. It is needed here because after all the rendering is done, we can calculate the number of pages required. Internally it calculates number of instructions too. And that is set after all the rendering of instructions is done. > > @@ +48,3 @@ > + let surface = this._layout.MapSurfaces.MapView; > + cr.setSourceSurface(surface, x, y); > + cr.paint(); > > whitespace damage? Will rectify. > > @@ +51,3 @@ > + }, > + > + _renderHeader: function(context, x, y) { > > Can't this be part of the layout classes? So that we only need to get a > surface here? Yes. Nice idea. Thanks :) > > @@ +56,3 @@ > + cr.moveTo(x, y); > + let query = Application.routeService.query; > + let header = '%s %s %s %s'.format(_("From"), > query.points[0].place.name, _("To"), query.points[query.points.length > -1].place.name); > > try to keep the rows at 80 chars max Okay > > @@ +67,3 @@ > + _renderMinimapFrom: function(cr, x, y) { > + cr.setSourceSurface(this._layout.MapSurfaces.FromView, x, y); > + cr.paint(); > > What is the indentation level here? Please conform to rest of maps in all > files and functions Sure. Will stick to that. > > @@ +100,3 @@ > + let x = 0, y = 0; > + x += this._layout.Margin > + y += this._layout.Margin; > > please mind alignment and whitespace! And try to keep the variables > declaration on top of the function. OKay. > > @@ +121,3 @@ > + + (page_num * this._layout.InstructionsPerPage.pageN); > + instructionEndIndex = (this._layout.InstructionsPerPage.page0 - > 1) + (page_num * this._layout.InstructionsPerPage.pageN); > + } > > Either break this up to multiple functions or have comments to describe what > is going on. It should either be obvious for everybody reading or we need a > comment. We want to keep understanding this code for a long time! Okay maybe little comments can help. Thanks.
Tried this out, here are some quick comments: * There needs to be a little breathing room between the text "From Origin To Destination" and the big map. * The route direction row is really slim and a lot of rows become double rows. There is a lot of space on the right, so might as well use that. * The length of each instruction have a break between the number and the unit.
(In reply to Andreas Nilsson from comment #57) Thanks Andreas for the review. :) > Tried this out, here are some quick comments: > * There needs to be a little breathing room between the text "From Origin To > Destination" and the big map. Yes, definitely.Will increase the gap. > * The route direction row is really slim and a lot of rows become double > rows. There is a lot of space on the right, so might as well use that. > * The length of each instruction have a break between the number and the > unit. Yeah right. Both of the above issues can be dealt by increasing size of instruction widget.Wil change it. Thanks.
Created attachment 320028 [details] [review] Add Print Route Feature Print Operation class is added to draw the cairo surfaces given to it. Print Layout acts as a tool-box which sub-classes like short Route layout , long route layout use to layout the surfaces as per the respective requirements of rendering various surfaces. GUI is added to give users an ability to access print Route feature. A review based on refactoring of code, conceptual errors, naming conventions will be helpful. Also , a query. In long route layouts , below the complete mapview surface, both instructions and minimaps surfaces are going down in a flow. But the page divide is set to be based on instructions only. Is that appropriate or should some other way needs to be adopted? Also need little review from design aspect, like the font style of header, sizes , locations of various surfaces, how should they be?
Review of attachment 320028 [details] [review]: This looks so great! We are getting there! I would like the longRouteLayout and shortLauoutRoute to be named as shortPrintLayout/longPrintLayout I think, since that is what they are. I also would like to have the markers for start and end included in the mapView. As they are in Maps and the mockups. Also, it takes quite some time here in my hotell in Belgium between pressing the button and when we reach render-complete and start print. Maybe we could use the mainWindow markBusy method or something to indicate that something is happening? Really great work Amisha! ::: src/longRouteLayout.js @@ +48,3 @@ + HEIGHT: 1810 +}; + The numbers are still very magical to me, why were they chosen? Arbitrary? @@ +59,3 @@ + delete params.route; + + this._pageNum = -1; Why does this need to be -1? @@ +71,3 @@ + + render: function() { + let totalSurfaces = 0, x = 0, y = 0, dy = 0; I do not think we use this syntax elsewhere? Maybe move each to own line. @@ +79,3 @@ + /* Before rendering each surface , it is checked if it can be adjusted in current page, + other wise a new page is created */ + This new line not needed @@ +110,3 @@ + + let nStartLocations = this._createLocationArray(0, + Math.min(_MINIMAP_LOCATIONS_NUM - 1, this._route.turnPoints.length - 1)); Why the -1? @@ +138,3 @@ + let nEndLocations = this._createLocationArray(Math.max(0, + this._route.turnPoints.length - _MINIMAP_LOCATIONS_NUM ), + this._route.turnPoints.length - 1); Maybe break out these max and min statements to variables to avoid the awkward line breaks? ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); I do not like this, and it is not needed, reference short and long route here directly instead of this indirection. Remove the supported types array and concept. @@ +52,3 @@ + Signals: { + 'render-complete': { }, + 'render-surface': { } Couldn't this signal include an integer which would be the number of surface rendered with this one included? So you would be able to do: this.connect('render-surface', (function(surfacesRendered) { }); Or maybe, one can perform the checks in this class, and never emit render-surface, but only render-complete when all are rendered? This class can take a parameter "totalSurfaces" ? so below would be: _init: function(params) { this._totalSurfaces = params.totalSurfaces; delete params.totalSurfaces; } @@ +62,3 @@ + + _drawMapView: function(width, height, zoomLevel, locations, x, y, pageNum) { + let factory = Champlain.MapSourceFactory.dup_default(); Is the default really the one we want? We might end up with error tiles? @@ +68,3 @@ + view.visible = true; + view.zoom_level = zoomLevel; + view.set_map_source(mapSource); Can't all of these be set above while constructing? ... height: ..., visible: true, zoom_level: ..., map_source: ... }); @@ +74,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + + if (view.state !== Champlain.State.DONE) { Do we need some sort of timeout? Are we guaranteed to always reach DONE here? @@ +90,3 @@ + x: x, + y: y }); + this.emit('render-surface'); Maybe break this out to a _addSurface(surface, x, y); ? @@ +108,3 @@ + stroke_color: _STROKE_COLOR }); + view.add_layer(routeLayer); + this._route.path.forEach(routeLayer.add_node.bind(routeLayer)); Do we always add the whole route? @@ +137,3 @@ + this.emit('render-surface'); + }).bind(this)); + }, Nice function! ::: src/printOperation.js @@ +31,3 @@ + _init: function() { + PrintLayout.SUPPORTED_TYPES.push(LongRouteLayout.LongRouteLayout); + PrintLayout.SUPPORTED_TYPES.push(ShortRouteLayout.ShortRouteLayout); These are not needed @@ +57,3 @@ + + let pageRatio = { x: pageSize.width / this._layout.pageWidth, + y: pageSize.height / this._layout.pageHeight }; Are these objects really called for? maybe just: let width = context... let height = context... let xScale = width / ... let yScale = height / ... ? @@ +73,3 @@ + log(self.operation.get_error()); + } + } I love how simple this class has gotten! ::: src/shortRouteLayout.js @@ +40,3 @@ + WIDTH: 1135, + HEIGHT: 1810 +}; A lot of these seems shared? Maybe belong in the base class printLayout?
Review of attachment 320028 [details] [review]: Thanks Jonas for the review. Yes will definitely reach there pretty soon. :) ::: src/longRouteLayout.js @@ +48,3 @@ + HEIGHT: 1810 +}; + Actually these numbers are taken just to decide a ratio , page vs mapview, page vs instructions, etc. Some other numbers can also be used.It is matter about the ratio which is to be used in actual sense. @@ +59,3 @@ + delete params.route; + + this._pageNum = -1; because when i call the render method, before starting anything it first create a new page which increases the pageNum by 1. Initially pageNum as 0 is required to save its respective surfaces. @@ +71,3 @@ + + render: function() { + let totalSurfaces = 0, x = 0, y = 0, dy = 0; Okay. Will rearrange. @@ +79,3 @@ + /* Before rendering each surface , it is checked if it can be adjusted in current page, + other wise a new page is created */ + Okay. @@ +110,3 @@ + + let nStartLocations = this._createLocationArray(0, + Math.min(_MINIMAP_LOCATIONS_NUM - 1, this._route.turnPoints.length - 1)); The values within these indexes are inclusive .If -1 is not used, in the function definition, for loop can be refactored accordingly to make end index exclusive. Will remove these -1's @@ +138,3 @@ + let nEndLocations = this._createLocationArray(Math.max(0, + this._route.turnPoints.length - _MINIMAP_LOCATIONS_NUM ), + this._route.turnPoints.length - 1); Okay. Sure ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); If i will import these two classes in printLayout.js, there will be issue of circular dependencies because these two classes import printLayout.js class @@ +52,3 @@ + Signals: { + 'render-complete': { }, + 'render-surface': { } Okay will try this one. @@ +62,3 @@ + + _drawMapView: function(width, height, zoomLevel, locations, x, y, pageNum) { + let factory = Champlain.MapSourceFactory.dup_default(); Alternative? umm @@ +68,3 @@ + view.visible = true; + view.zoom_level = zoomLevel; + view.set_map_source(mapSource); Sure. @@ +74,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + + if (view.state !== Champlain.State.DONE) { Yeah ,I missed it.It should be there. Thanks. @@ +90,3 @@ + x: x, + y: y }); + this.emit('render-surface'); yeah. a function can be made. pagenum also needs to be send as parameter, right? @@ +108,3 @@ + stroke_color: _STROKE_COLOR }); + view.add_layer(routeLayer); + this._route.path.forEach(routeLayer.add_node.bind(routeLayer)); Is there a way to break up the route in pieces? @@ +137,3 @@ + this.emit('render-surface'); + }).bind(this)); + }, Thanks. ::: src/printOperation.js @@ +57,3 @@ + + let pageRatio = { x: pageSize.width / this._layout.pageWidth, + y: pageSize.height / this._layout.pageHeight }; Sure. @@ +73,3 @@ + log(self.operation.get_error()); + } + } :) ::: src/shortRouteLayout.js @@ +40,3 @@ + WIDTH: 1135, + HEIGHT: 1810 +}; Yeah that can be done. But each one was put in separate classes keeping in mind the size can vary for different layout. For example, instruction size is smaller in long routes as compared to shorter routes. as per the mockup.
Review of attachment 320028 [details] [review]: We need to make sure that when is offline, and no Map is shown that the print-button is not pressable. Look at code here: https://git.gnome.org/browse/gnome-maps/tree/src/mainWindow.js#n254 ::: src/longRouteLayout.js @@ +48,3 @@ + HEIGHT: 1810 +}; + Well couldn't we then skip them and just define ratios? @@ +148,3 @@ + totalSurfaces += 1; + + this.numPages = this._pageNum + 1; Do we really need this._pageNum? Can't this be this.numPages++; and init this.numPages to 0? ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); I do not understand, I did the following and it was no problem: http://paste.fedoraproject.org/316703/95050145 @@ +74,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + + if (view.state !== Champlain.State.DONE) { So what should happen if we timeout? If we do not get to DONE in X seconds? @@ +108,3 @@ + stroke_color: _STROKE_COLOR }); + view.add_layer(routeLayer); + this._route.path.forEach(routeLayer.add_node.bind(routeLayer)); the route already is in pieces, right? What you are doing here is iterating through the array of the route and adding each as a node in a champlain path layer.
Review of attachment 320028 [details] [review]: Awesome, really nice work! My comments below. ::: data/ui/main-window.ui @@ +99,3 @@ </child> + <child> + <object class="GtkButton" id="printRouteButton"> I think we should define an action-name and add it in MainWindow._initActions, check, for example, how the gotoUserLocationButton is made. We could also add an accel for it (<Primary>P, for example). ::: src/longRouteLayout.js @@ +155,3 @@ + this.emit('render-complete'); + } + }).bind(this)); We are doing this exact counting here and in ShortRouteLayout, I would do this in the _addSurface method that Jonas proposed. ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); Maybe you can add the imports inside the newFromRoute, personally, I prefer this workaround instead of this confusing SUPPORTED_TYPES array. @@ +46,3 @@ +} + +const PrintLayout = new Lang.Class({ I'd add this somewhere in this class. //Abstract render: function() { }, @@ +65,3 @@ + let mapSource = factory.create_cached_source(MapView.MapType.STREET); + let view = new Champlain.View({ width: width, + height: height }); Hmmm... what about reusing our application MapView and save the old zoom_level and current location to restore it later? @@ +90,3 @@ + x: x, + y: y }); + this.emit('render-surface'); Yes, I think so. @@ +140,3 @@ + + _drawHeader: function(width, height, x, y, pageNum) { + let surface = new Cairo.ImageSurface (Cairo.Format.ARGB32, width, height); Don't add space between "e" and "(", same in the next line (and possibly in other places too :) ) @@ +144,3 @@ + let layout = PangoCairo.create_layout(cr); + let query = Application.routeService.query; + let header = '%s %s %s %s'.format(_("From"), This must be _("From %s to %s").format(...) @@ +147,3 @@ + query.points[0].place.name, + _("To"), + query.points[query.points.length -1].place.name); Add space between - and 1 @@ +158,3 @@ + this.surfaceObjects[pageNum].push({ surface: surface, + x: x, + y: y }); Bad alignment @@ +177,3 @@ + let renderedSurfaces = 0; + + for (let k = 0; k < this.surfaceObjects.length; k++) { for..in ::: src/printOperation.js @@ +35,3 @@ + this._layout = PrintLayout.newFromRoute(Application.routeService.route); + + this._operation = new Gtk.PrintOperation(); We should use embed_page_setup to allow user to setup page sizes. @@ +60,3 @@ + + let cr = context.get_cairo_context(); + cr.scale(pageRatio.x, pageRatio.y); Hmmm, I don't think this is what we want to do. We are rendering the layout using a fixed page size and aspect ratio and I don't think that's correct, we are actually making all the rendering without having any of the printing user settings, like page size and orientation. I think the correct way to do this is to create the layout and set the canvas size for it (using context.get_width() and context.get_height() that gives us the size of the printing area in pixels, with margins and everything taken into account, getting rid of _PAGE constant and the _margin variable) in the GtkOperation::paginate signal handler and start the rendering. Then we can use the GtkOperation::paginate signal handler to check if the rendering is finished, once is finished, in that handler, we can set the pages number to the operation object and return true to star the GtkOperation::draw-page signal emission. The problem is that we can complicate the rendering algorithm if we have variable page size. Though I think we cannot escape to this... we can get really ugly scaling if we use different page setups (think of #10 Envelope in Landscape). @@ +68,3 @@ + + _runPrintOperation: function() { + let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); Do we want to save the print settings here (maybe persist those in a file, GtkPrintSettings has the ability to do that) and use it to instantiate further GtkPrintOperation objects? @@ +70,3 @@ + let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); + + if (result == Gtk.PrintOperationResult.ERROR) { Use === for comparison. @@ +71,3 @@ + + if (result == Gtk.PrintOperationResult.ERROR) { + log(self.operation.get_error()); 3 things here: 1. "self" is to Pythonic for this JavaScript code :D 2. If we are going to show an error in the console, please use Utils.debug 3. Is this something we want to notify in the GUI? ::: src/route.js @@ +68,3 @@ }, + createBBox: function(coordinates) { I think this should be converted to a property (get bbox() {}).
Review of attachment 320028 [details] [review]: ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); Jonas, be careful, your patch works only by chance. If you remove the unused LongRouteLayout and ShortRouteLayout imports in printOperation.js, that diff no longer works.
Review of attachment 320028 [details] [review]: ::: data/ui/main-window.ui @@ +99,3 @@ </child> + <child> + <object class="GtkButton" id="printRouteButton"> Yes, that would be great. We can do that. ::: src/longRouteLayout.js @@ +48,3 @@ + HEIGHT: 1810 +}; + Umm yes. Can be done. Then it will like mapview width/ page width ratio = x(some number), similar for other surfaces and margins. @@ +155,3 @@ + this.emit('render-complete'); + } + }).bind(this)); Yeah nice. Will do that way. ::: src/printLayout.js @@ +43,3 @@ + return new SUPPORTED_TYPES[0]({ route: route }); + else + return new SUPPORTED_TYPES[1]({ route: route }); Yeah. That's working. I don't know i tried the same earlier. It was giving issues. But now it's working fine. Will remove confusing SUPPORTED_TYPES array. @@ +46,3 @@ +} + +const PrintLayout = new Lang.Class({ Yeah. Right, because sub-classes are defining this method as per their requirement. @@ +65,3 @@ + let mapSource = factory.create_cached_source(MapView.MapType.STREET); + let view = new Champlain.View({ width: width, + height: height }); we will be playing with view (modifying zoom-level, etc) to get the minimaps. If we use application MapView, then similar modifications will be visible in our application window . Won't that look bad? @@ +74,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + + if (view.state !== Champlain.State.DONE) { Maybe showing a message for Timeout / Something went wrong. Please try again @@ +90,3 @@ + x: x, + y: y }); + this.emit('render-surface'); okay. @@ +140,3 @@ + + _drawHeader: function(width, height, x, y, pageNum) { + let surface = new Cairo.ImageSurface (Cairo.Format.ARGB32, width, height); Sure. :) @@ +144,3 @@ + let layout = PangoCairo.create_layout(cr); + let query = Application.routeService.query; + let header = '%s %s %s %s'.format(_("From"), okay. @@ +147,3 @@ + query.points[0].place.name, + _("To"), + query.points[query.points.length -1].place.name); okay. @@ +158,3 @@ + this.surfaceObjects[pageNum].push({ surface: surface, + x: x, + y: y }); yeah, will correct that @@ +177,3 @@ + let renderedSurfaces = 0; + + for (let k = 0; k < this.surfaceObjects.length; k++) { Dint get this one.. ::: src/printOperation.js @@ +35,3 @@ + this._layout = PrintLayout.newFromRoute(Application.routeService.route); + + this._operation = new Gtk.PrintOperation(); Yes that has to be added further. @@ +60,3 @@ + + let cr = context.get_cairo_context(); + cr.scale(pageRatio.x, pageRatio.y); Hmm yes I am understanding a bit. When do we have to call Printlayout render method? @@ +68,3 @@ + + _runPrintOperation: function() { + let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); Yes can be added. Would be helpful to user. @@ +70,3 @@ + let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); + + if (result == Gtk.PrintOperationResult.ERROR) { okay @@ +71,3 @@ + + if (result == Gtk.PrintOperationResult.ERROR) { + log(self.operation.get_error()); Yes we can show some message, Maybe 'printing failed' to user in GUI. ::: src/route.js @@ +68,3 @@ }, + createBBox: function(coordinates) { Okay.
I tested this a bit and I only have two minor UI remarks: * The left margin is a lot bigger than the right margin on the printouts. * If the From and To string is really long, it gets cut off.
Created attachment 320256 [details] margin screenshot
Created attachment 320257 [details] long string screenshot
(In reply to Andreas Nilsson from comment #68) > I tested this a bit and I only have two minor UI remarks: > * The left margin is a lot bigger than the right margin on the printouts. > * If the From and To string is really long, it gets cut off. Thanks Andreas, the margin thing is due to the margin set of that of gtk print operation and can be resolved by setting its use full page property to true. Will do it. And regarding the header thing? ATU what will be better? Ellipising or shifting the rest of header to new lines?
Review of attachment 320028 [details] [review]: ::: src/printLayout.js @@ +177,3 @@ + let renderedSurfaces = 0; + + for (let k = 0; k < this.surfaceObjects.length; k++) { I mean, you can do "for (k in this.surfaceObjects)" ::: src/printOperation.js @@ +60,3 @@ + + let cr = context.get_cairo_context(); + cr.scale(pageRatio.x, pageRatio.y); Sorry, I think I have a typo in my comment. To make it clear, this is what I think you need to do in each signal handler: begin-print: Create layout instance (long or short) Set page information to the layout (like context.get_width() and context.get_height()) layout.render() set_n_pages() paginate: if (layout.renderFinished) return true // paginate signal emission stops, draw-page signal emission starts else return false // paginate is emitted again draw-page: Roughly what you are doing now (but you don't need to scale anymore)
Review of attachment 320028 [details] [review]: ::: src/printLayout.js @@ +177,3 @@ + let renderedSurfaces = 0; + + for (let k = 0; k < this.surfaceObjects.length; k++) { for (let k in this.surfaceObjects)
Review of attachment 320028 [details] [review]: ::: src/printLayout.js @@ +177,3 @@ + let renderedSurfaces = 0; + + for (let k = 0; k < this.surfaceObjects.length; k++) { or even this.surfaceObjects.reduce(function(previous, current) { return previous.length + current.length }); ? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce
Review of attachment 320028 [details] [review]: ::: src/longRouteLayout.js @@ +148,3 @@ + totalSurfaces += 1; + + this.numPages = this._pageNum + 1; Should I remove this._pageNum completely? In that case, i have to pass this.numPages - 1 as parameter in drawsurfaces calls. ::: src/printOperation.js @@ +60,3 @@ + + let cr = context.get_cairo_context(); + cr.scale(pageRatio.x, pageRatio.y); Okay now how we have to use this page size information in layout instance? we can keep the various ratios fixed like header/page or mapview/page, etc. Does that sound fine? And setting number of pages can be done only when rendering is completed because use of pages is dynamic while render call is being executed.
Review of attachment 320028 [details] [review]: ::: src/longRouteLayout.js @@ +148,3 @@ + totalSurfaces += 1; + + this.numPages = this._pageNum + 1; Sure
(In reply to amisha from comment #71) > (In reply to Andreas Nilsson from comment #68) > > I tested this a bit and I only have two minor UI remarks: > > * The left margin is a lot bigger than the right margin on the printouts. > > * If the From and To string is really long, it gets cut off. > > Thanks Andreas, the margin thing is due to the margin set of that of gtk > print operation and can be resolved by setting its use full page property to > true. Will do it. And regarding the header thing? ATU what will be better? > Ellipising or shifting the rest of header to new lines? Hi, My idea would be to ellipsize the name of the destinations, maybe at 25 chars? So the long one: "From 2 Norra Ågatan, Sweden To Friggagatan, Göteborgs och Bohuslän, Västra Götaland" Would be something like: "From 2 Norra Ågatan. Swed. To Friggagatan, GöteBorgs ..."
Or rather: "From 2 Norra Ågatan, Sweden To Friggagatan, Göteborgs ..."
And if the From-destination is to long that gets elippsized: "From ReallyReallyReallyReallyLon... To AShortOne"
(In reply to Jonas Danielsson from comment #78) > Or rather: > > "From 2 Norra Ågatan, Sweden To Friggagatan, Göteborgs ..." That sounds good to me.
Created attachment 320906 [details] [review] Add Print Route Feature A review is required regarding the functionality of keeping fixed ratio of various surfaces.Also its repurcussion on an important factor- whether the information per page should be kept same regardless of page size or not is to be decided.
Review of attachment 320906 [details] [review]: Thanks! As a general comment, the commit message must be a short first line, followed by a blank line, the description and the bugzilla link, take a look to this: https://wiki.gnome.org/Git/CommitMessages. Also, are you using git-bz to send these patches? that utility add the bugzilla link in the commit message for you and let you upload the patches much more easily. Please delete all the commented code when submitting it. ::: src/longPrintLayout.js @@ +24,3 @@ +const _MINIMAP_LOCATIONS_NUM = 5; + +/* All constants have units as pixels */ This comment is no longer valid since the constants are percentages. @@ +85,3 @@ + this._createNewPage(); + y = this._margin; + } Hmmm... If you haven't rendered anything yet, why do this check? @@ +89,3 @@ + y += dy; + + dy = _MAPVIEW.HEIGHT * this._pageHeight + _GAP * this._pageWidth; Why multiplying by this._pageWidth here? ::: src/mainWindow.js @@ +249,3 @@ _initHeaderbar: function() { + this._mapView.bind_property('routeVisible', + this._printRouteButton, 'visible', GObject.BindingFlags.DEFAULT); Please move this after the favoritesPopover/Button setup in this method, so we order these things by relevance. Also, move the two first method arguments ('routeVisible' and this._printRouteButton) in the first line of the method call and the other two to the second line. @@ +381,3 @@ + _printRouteActivate: function() { + if (this._mapView.routeVisible) { + let operation = new PrintOperation.PrintOperation(); Pass the MainWindow instance to the PrintOperation instance and use it as a parent window in Gtk.PrintOperation.run() ::: src/printLayout.js @@ +98,3 @@ + this.surfacesRendered++; + if (this.surfacesRendered === this._totalSurfaces) + this.emit('render-complete'); Move: this.surfacesRendered++; if (this.surfacesRendered === this._totalSurfaces) this.emit('render-complete'); to _addSurface Same in other places. @@ +182,3 @@ + + _addSurface: function(surface, x, y, pageNum) { + this.surfaceObjects[pageNum].push({ surface: surface, x: x, y: y }); As I can note in both print layouts, you always print in the current page (this.numPages - 1), so there is no need to have the pageNum argument in every method. ::: src/printOperation.js @@ +50,3 @@ + this._layout = PrintLayout.newFromRoute(Application.routeService.route, + context.get_page_setup().get_paper_width(Gtk.Unit.POINTS), + context.get_page_setup().get_paper_height(Gtk.Unit.POINTS)); Remember, get_width/get_height give you the size of the printable area, which is the only thing you want here. Don't calculate the margins manually, you start rendering in y = 0. Even more, I rather send the full print context object to the printlayout. @@ +53,3 @@ + this._layout.connect('render-complete', (function() { + this._layout.renderFinished = true; + }).bind(this)); You should set renderFinished in PrintLayout @@ +84,3 @@ + + if (result === Gtk.PrintOperationResult.ERROR) { + Utils.debug('Failed to print: %s'.format(this._operation.get_error())); Import Utils
Review of attachment 320906 [details] [review]: Awesome work! The code is looking good! We will get there I feel! ::: src/longPrintLayout.js @@ +43,3 @@ + HEIGHT: 0.15, + MAX_ZOOM_LEVEL: 18 +}; For all these const-objects... There is a bit to much CAPS for my liking here. If we look at placeStore and mapView, maybe a style of: const _Header = { const _Instruction = { const _MapView = { const _MiniMapView = { And also, the code gets harder to read below, I feel. Because width/height is not width and height. Something like: _MAPVIEW.WIDTH * this._pageWidth below is not multiplying two widths. It is multiplying a width with a scale, right? So maybe the name here is wrong. _MapView.SCALE_X * this._pageWidth would be more self-explanatory for me, what do you think? @@ +59,3 @@ + delete params.pageHeight; + + let totalSurfaces = 4 + this._route.turnPoints.length; This 4 is a bit magical. At least have a comment, something like: /* (Header + 3 maps) + instructions */ let totalSurfaces = 4 + this._route.turnPoints.length; @@ +60,3 @@ + + let totalSurfaces = 4 + this._route.turnPoints.length; + this.parent({totalSurfaces: totalSurfaces}); Some spaces here would be nice ({ numSurfaces: totalSurfaces }); @@ +67,3 @@ + + _initMargin: function() { + this._margin = (this._pageWidth - (_MAPVIEW.WIDTH * this._pageWidth)) / 2; I do not understand this at a glance. It might be because I avoid math whenever possible. Would a comment make sense? @@ +80,3 @@ + + /* Before rendering each surface , it is checked if it can be adjusted in current page, + other wise a new page is created */ This is my personal preference, ignore if you think I am mad or to nitty. For comments that span multiple lines I prefer the comment style of: /* * Before rendering each surface , it is checked if it can be adjusted in current page, * other wise a new page is created. */ @@ +107,3 @@ + instructions bound. Later on this can be made dynamic depending upon factors + like total number of instructions, complexity of neighbourhood areas, etc. */ + Same comment about long comments here :) And also an extra space. Nice comment by the way! @@ +109,3 @@ + + let nthStartLocation = Math.min(_MINIMAP_LOCATIONS_NUM, this._route.turnPoints.length); + let nStartLocations = this._createLocationArray(0, nthStartLocation); just startLocations I feel. @@ +134,3 @@ + + let firstEndLocation = Math.max(0, this._route.turnPoints.length - _MINIMAP_LOCATIONS_NUM); + let nEndLocations = this._createLocationArray(firstEndLocation, this._route.turnPoints.length); just endLocations ::: src/printLayout.js @@ +21,3 @@ +const Champlain = imports.gi.Champlain; +const Clutter = imports.gi.Clutter; +const Gdk = imports.gi.Gdk; Not used? @@ +37,3 @@ +const Utils = imports.utils; + +const _MIN_DISTANCE = 2000; What is the unit here? @@ +53,3 @@ + return new ShortPrintLayout.ShortPrintLayout({ route: route, + pageWidth: pageWidth, + pageHeight: pageHeight }); For multi line statements I prefer braces on if and else, ignore if you feel they are to ugly. @@ +72,3 @@ + this.surfaceObjects = []; + this.surfacesRendered = 0; + this.renderFinished = false; Do all these need to be public? Can't they be this._... ? @@ +77,3 @@ + render: function() { + }, + Do we have to have this render function? @@ +83,3 @@ + let view = new Champlain.View({ width: width, + height: height, + zoom_level: zoomLevel }); add the mapSource here as well, , map_source: mapSource }); @@ +89,3 @@ + + view.ensure_visible(this._route.createBBox(locations), false); + remove this space. @@ +94,3 @@ + if (view.state === Champlain.State.DONE) { + view.disconnect(notifyId); + let surface = view.to_surface(true); I think we want to check if (!surface) here and error out? How do we error out at this point? What will happen in UI? @@ +102,3 @@ + }).bind(this)); + } else { + let surface = view.to_surface(true); if (!surface) @@ +142,3 @@ + cr.paint(); + cr.setOperator(Cairo.Operator.OVER); + }).bind(this)); Maybe a comment about that this draw handler is just to remove background? @@ +162,3 @@ + let from = query.points[0].place.name; + if (from.length > 25) + from = from.substr(0, 22) + " ..."; We use " for translations only, so you can use + '...'; Then we can more easily find translations string quickly @@ +165,3 @@ + let to = query.points[query.points.length - 1].place.name; + if (to.length > 25) + to = to.substr(0, 22) + " ..."; No since we are doing this twice, maybe break it out to a function? let from = this._destinationName(0); let to = this._destinationName(query.points.length - 1]); @@ +166,3 @@ + if (to.length > 25) + to = to.substr(0, 22) + " ..."; + let header = _("From %s to %s").format(from, to); Since you add translations, this file needs to be added to po/POTFILES.in as well! Same for all other new files that add strings for translation! ::: src/printOperation.js @@ +35,3 @@ + this._operation = new Gtk.PrintOperation(); + this._operation.set_embed_page_setup(true); + this._operation.set_use_full_page(true); These can be set in the new call? new Gtk.PrintOperation({ embeded_page_setup: true, use_full_page: true }); ::: src/shortPrintLayout.js @@ +37,3 @@ + MAX_ZOOM_LEVEL: 18 +}; + Same comment about naming and format of these constants! @@ +52,3 @@ + delete params.pageHeight; + + let totalSurfaces = 2 + this._route.turnPoints.length; Same comment about a comment about the 2
Created attachment 320931 [details] Pdf printed by Maps So a comment here, you see that the instruction strings are sort of squished and the distance spans two lines. Can something be done about that? Also, on the second page, where there are no maps, why can't the instruction span all the page? Instead of a column? Can't we always make it so that an instruction can take the whole page if there is no map view next to it? Sort of _______ | | At blablabla turn left 23 m | | At blablabla turn left 23 m | | At blablabla turn left 23 m |_______| At blablabla turn left 23 m At blablabla turn left 23 m At blablabla turn left 23 m At blablabla turn left 23 m At blablabla turn left 23 m _______ | | At blablabla turn left 23 m | | At blablabla turn left 23 m | | At blablabla turn left 23 m |_______| At blablabla turn left 23 m And another thing. I belive we can set the default output name of the PDF? So we could provide a more meaningful name than "outdata" or whatever it is by default? We can leave this point as a gnome-love bug if you want to, it is a bit of a nit. Same with the formatting stuff above, we can try to fix it later if you feel it is complicated.
(In reply to Jonas Danielsson from comment #84) > Created attachment 320931 [details] > Pdf printed by Maps > > So a comment here, you see that the instruction strings are sort of squished > and the distance spans two lines. Can something be done about that? Changing the ratios(increasing space for instructions) will help. I will also thinking for ellipsing the instructions to fixed number of characters. > > Also, on the second page, where there are no maps, why can't the instruction > span all the page? Instead of a column? Can't we always make it so that an > instruction can take the whole page if there is no map view next to it? 1. It will be little tricky as instructions are printed altogether separately from mapview.As of now there is no way of knowing if a set of instructions are associated with a mapview or not. 2. Need to discuss with Andreas, from point of design. As per me, it will look fine and use paper efficiently. > > Sort of > _______ > | | At blablabla turn left 23 m > | | At blablabla turn left 23 m > | | At blablabla turn left 23 m > |_______| At blablabla turn left 23 m > > At blablabla turn left 23 m > At blablabla turn left 23 m > At blablabla turn left 23 m > At blablabla turn left 23 m > _______ > | | At blablabla turn left 23 m > | | At blablabla turn left 23 m > | | At blablabla turn left 23 m > |_______| At blablabla turn left 23 m > > > And another thing. I belive we can set the default output name of the PDF? > So we could provide a more meaningful name than "outdata" or whatever it is > by default? Yes we can set that. Maybe just "RouteInstructions" > > We can leave this point as a gnome-love bug if you want to, it is a bit of a > nit. Same with the formatting stuff above, we can try to fix it later if you > feel it is complicated. Changing file name can be left as a love bug. But formatting the above part willl require much code change i feel.
We can file a (In reply to amisha from comment #85) > (In reply to Jonas Danielsson from comment #84) > > Created attachment 320931 [details] > > Pdf printed by Maps > > > > So a comment here, you see that the instruction strings are sort of squished > > and the distance spans two lines. Can something be done about that? > Changing the ratios(increasing space for instructions) will help. I will > also thinking for ellipsing the instructions to fixed number of characters. > > > > Also, on the second page, where there are no maps, why can't the instruction > > span all the page? Instead of a column? Can't we always make it so that an > > instruction can take the whole page if there is no map view next to it? > > 1. It will be little tricky as instructions are printed altogether > separately from mapview.As of now there is no way of knowing if a set of > instructions are associated with a mapview or not. > 2. Need to discuss with Andreas, from point of design. As per me, it will > look fine and use paper efficiently. > > > > Sort of > > _______ > > | | At blablabla turn left 23 m > > | | At blablabla turn left 23 m > > | | At blablabla turn left 23 m > > |_______| At blablabla turn left 23 m > > > > At blablabla turn left 23 m > > At blablabla turn left 23 m > > At blablabla turn left 23 m > > At blablabla turn left 23 m > > _______ > > | | At blablabla turn left 23 m > > | | At blablabla turn left 23 m > > | | At blablabla turn left 23 m > > |_______| At blablabla turn left 23 m > > > > > > And another thing. I belive we can set the default output name of the PDF? > > So we could provide a more meaningful name than "outdata" or whatever it is > > by default? > Yes we can set that. Maybe just "RouteInstructions" > > > > We can leave this point as a gnome-love bug if you want to, it is a bit of a > > nit. Same with the formatting stuff above, we can try to fix it later if you > > feel it is complicated. > Changing file name can be left as a love bug. But formatting the above part > willl require much code change i feel. We can file a bug for it in either case. And try to fix it before 3.20 release. But maybe leave it as is for now?
(In reply to Jonas Danielsson from comment #86) > We can file a (In reply to amisha from comment #85) > > (In reply to Jonas Danielsson from comment #84) > > > Created attachment 320931 [details] > > > Pdf printed by Maps > > > > > > So a comment here, you see that the instruction strings are sort of squished > > > and the distance spans two lines. Can something be done about that? > > Changing the ratios(increasing space for instructions) will help. I will > > also thinking for ellipsing the instructions to fixed number of characters. > > > > > > Also, on the second page, where there are no maps, why can't the instruction > > > span all the page? Instead of a column? Can't we always make it so that an > > > instruction can take the whole page if there is no map view next to it? > > > > 1. It will be little tricky as instructions are printed altogether > > separately from mapview.As of now there is no way of knowing if a set of > > instructions are associated with a mapview or not. > > 2. Need to discuss with Andreas, from point of design. As per me, it will > > look fine and use paper efficiently. > > > > > > Sort of > > > _______ > > > | | At blablabla turn left 23 m > > > | | At blablabla turn left 23 m > > > | | At blablabla turn left 23 m > > > |_______| At blablabla turn left 23 m > > > > > > At blablabla turn left 23 m > > > At blablabla turn left 23 m > > > At blablabla turn left 23 m > > > At blablabla turn left 23 m > > > _______ > > > | | At blablabla turn left 23 m > > > | | At blablabla turn left 23 m > > > | | At blablabla turn left 23 m > > > |_______| At blablabla turn left 23 m > > > > > > > > > And another thing. I belive we can set the default output name of the PDF? > > > So we could provide a more meaningful name than "outdata" or whatever it is > > > by default? > > Yes we can set that. Maybe just "RouteInstructions" > > > > > > We can leave this point as a gnome-love bug if you want to, it is a bit of a > > > nit. Same with the formatting stuff above, we can try to fix it later if you > > > feel it is complicated. > > Changing file name can be left as a love bug. But formatting the above part > > willl require much code change i feel. > > We can file a bug for it in either case. And try to fix it before 3.20 > release. But maybe leave it as is for now? Yeah,Sure
Review of attachment 320906 [details] [review]: Looks great! Mostly style suggestions. Try to make sure you fit in 80 columns (78 is even better because then there's room for viewing diffs (which have "+ ", " ", or "- ") in 80 columns). http://stackoverflow.com/questions/25900954/80-characters-right-margin-line-in-sublime-text-3 I couldn't view the output though, so I'll probably have other comments on the next version. I'm very excited for this feature! ::: data/ui/main-window.ui @@ +104,3 @@ + <property name="tooltip-text" translatable="yes">Print Route</property> + <property name="action-name">win.print-route</property> + <property name="valign">center</property> why is center needed? When I change it to the default GTK_ALIGN_FILL with inspector the button looks exactly the same. This makes me feel like we can remove this line altogether. @@ +106,3 @@ + <property name="valign">center</property> + <style> + <class name="image-button"/> What do you think about adding the suggested-action style class here? http://i.imgur.com/sqq3KZV.png I like it because it brings more attention to a button that strangely hides and comes back. I wonder if we could even do an animation for the print icon like scale(0) just so it has something that catches people's eye when a route is displayed. (an example of css animations here https://git.gnome.org/browse/gtk+/diff/gtk/theme/Adwaita/gtk-contained.css?id=460aa64c58b6b12d49b132b7c4df0eadf0070e34) @@ +111,3 @@ + <object class="GtkImage" id="print-route-button-image"> + <property name="visible">True</property> + <property name="icon-size">1</property> Similarly, what does this do? ::: src/mainWindow.js @@ +210,3 @@ + }, + 'print-route': { + accels: ['<Primary>P'], Please add documentation about this to data/ui/help-overlay.ui @@ +249,3 @@ _initHeaderbar: function() { + this._mapView.bind_property('routeVisible', + this._printRouteButton, 'visible', GObject.BindingFlags.DEFAULT); Over 78 characters. this is more readable: this._mapView.bind_property('routeVisible', this._printRouteButton, 'visible', GObject.BindingFlags.DEFAULT); @@ +272,3 @@ favoritesPopover.rows > 0); this._placeEntry.sensitive = app.connected; + this._printRouteButton.sensitive = app.connected; I don't think this line is needed since if the button is visible we have routeVisible and thus the app is connected. @@ +380,3 @@ + _printRouteActivate: function() { + if (this._mapView.routeVisible) { Is this the best place for this check? ::: src/printLayout.js @@ +160,3 @@ + let layout = PangoCairo.create_layout(cr); + let query = Application.routeService.query; + let from = query.points[0].place.name; (gnome-maps:21910): Gjs-WARNING **: JS ERROR: TypeError: from is null PrintLayout<._drawHeader@resource:///org/gnome/Maps/js/printLayout.js:163 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 LongPrintLayout<.render@resource:///org/gnome/Maps/js/longPrintLayout.js:88 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 PrintOperation<._beginPrint@resource:///org/gnome/Maps/js/printOperation.js:56 http://i.imgur.com/jB5lCUJ.png I used the new right click menu to route to/from random coordinates. @@ +162,3 @@ + let from = query.points[0].place.name; + if (from.length > 25) + from = from.substr(0, 22) + " ..."; instead use the Unicode ellipses … @@ +165,3 @@ + let to = query.points[query.points.length - 1].place.name; + if (to.length > 25) + to = to.substr(0, 22) + " ..."; … ::: src/printOperation.js @@ +35,3 @@ + this._operation = new Gtk.PrintOperation(); + this._operation.set_embed_page_setup(true); + this._operation.set_use_full_page(true); "[when use_full_page is false] the origin is at the top left corner of the imageable area (i.e. inside the margins)." Why do we want true? @@ +38,3 @@ + this._operation.connect('begin-print', this._beginPrint.bind(this)); + this._operation.connect('draw-page', this._drawPage.bind(this)); + this._operation.connect('paginate', this._paginate.bind(this)); can you reorder these lines, and reorder the method definitions so they're in the order these signals are emitted? I.e. beginPrint, paginate, drawPage? It would help with readability of the code so you can more easily read top-to-bottom. @@ +50,3 @@ + this._layout = PrintLayout.newFromRoute(Application.routeService.route, + context.get_page_setup().get_paper_width(Gtk.Unit.POINTS), + context.get_page_setup().get_paper_height(Gtk.Unit.POINTS)); Too long. Instead: let w = context.get_page_setup().get_paper_width(Gtk.Unit.POINTS); let h = context.get_page_setup().get_paper_height(Gtk.Unit.POINTS); this._layout = PrintLayout.newFromRoute(Application.routeService.route, w, h); @@ +53,3 @@ + this._layout.connect('render-complete', (function() { + this._layout.renderFinished = true; + }).bind(this)); setting renderFinished should be done in PrintLayout, so this entire callback should be moved there. @@ +63,3 @@ + } else { + return false; + } This can be simplified to: if (this._layout.renderFinished) operation.set_n_pages(this._layout.numPages); return this._layout.renderFinished; @@ +76,3 @@ + + this._layout.surfaceObjects[page_num].forEach((function(surfaceObject) { + this._renderSurface(cr, surfaceObject.x, surfaceObject.y, surfaceObject.surface); Over 80. Also, renderSurface is simple enough and only used here, so we should probably inline it. Try: this._layout.surfaceObjects[page_num].forEach((function(so) { cr.setSourceSurface(so.surface, so.x, so.y); cr.paint(); @@ +81,3 @@ + + _runPrintOperation: function() { + let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); Over 80 let result = this._operation.run(Gtk.PrintOperationAction.PRINT_DIALOG, null); @@ +84,3 @@ + + if (result === Gtk.PrintOperationResult.ERROR) { + Utils.debug('Failed to print: %s'.format(this._operation.get_error())); Utils is undefined. Also this is over 80 characters. Try: let error = this._operation.get_error(); Utils.debug('Failed to print: %s'.format(error)); ::: src/shortPrintLayout.js @@ +25,3 @@ +const _GAP = 0.05; +const _HEADER = { + WIDTH: 0.9, I don't think most places in maps align constants on the right side of assignments. @@ +29,3 @@ +}; +const _INSTRUCTION = { + WIDTH: 0.9, here @@ +34,3 @@ +const _MAPVIEW = { + WIDTH: 0.9, + HEIGHT: 0.5, and these two @@ +50,3 @@ + + this._pageHeight = params.pageHeight; + delete params.pageHeight; You can make better use of inheritance by moving the above 6 statements out of ShortPrintLayout and LongPrintLayout and into PrintLayout. @@ +56,3 @@ + + this.numPages = 0; + this._initMargin(); and move these into PrintLayout @@ +61,3 @@ + _initMargin: function() { + this._margin = (this._pageWidth - (_MAPVIEW.WIDTH * this._pageWidth)) / 2; + }, And also this method into PrintLayout @@ +63,3 @@ + }, + + render: function() { And move this into PrintLayout but you have to separate out the shared vs unique code. In general I encourage you to go through Short, Long, and PrintLayout and try to consolidate all the shared code into the parent class. @@ +66,3 @@ + let x = 0; + let y = 0; + let dy = 0; see? no right-side alignment ;) @@ +96,3 @@ + this.numPages - 1); + y += dy; + This is where the Short and Long render() functions diverge, so move everything above into a PrintLayout.render(), and then either have that call renderDetail() which is implemented in the child classes, or maybe just have the child classes override render() and the first thing they do is call this.parent() to run the shared code. Probably the second way is cleaner since it involves fewer unique method names. (Example of this is in GeoJSONShapeLayer.load())
Review of attachment 320906 [details] [review]: ::: src/longPrintLayout.js @@ +24,3 @@ +const _MINIMAP_LOCATIONS_NUM = 5; + +/* All constants have units as pixels */ We should somewhere say that these are ratios I guess? and that is unit less. @@ +80,3 @@ + + /* Before rendering each surface , it is checked if it can be adjusted in current page, + other wise a new page is created */ /* * Long comments look better like * this I feel. */ Also: otherwise. @@ +81,3 @@ + /* Before rendering each surface , it is checked if it can be adjusted in current page, + other wise a new page is created */ + dy = (_HEADER.HEIGHT * this._pageHeight) + (_GAP * this._pageWidth); Please explain this with a comment @@ +106,3 @@ + /* Fixed number of locations are plotted on minimaps which requires a check on + instructions bound. Later on this can be made dynamic depending upon factors + like total number of instructions, complexity of neighbourhood areas, etc. */ Same formatting on this comment! Nice comment! @@ +109,3 @@ + + let nthStartLocation = Math.min(_MINIMAP_LOCATIONS_NUM, this._route.turnPoints.length); + let nStartLocations = this._createLocationArray(0, nthStartLocation); maybe just startLocations? @@ +134,3 @@ + + let firstEndLocation = Math.max(0, this._route.turnPoints.length - _MINIMAP_LOCATIONS_NUM); + let nEndLocations = this._createLocationArray(firstEndLocation, this._route.turnPoints.length); endLocations? ::: src/printLayout.js @@ +160,3 @@ + let layout = PangoCairo.create_layout(cr); + let query = Application.routeService.query; + let from = query.points[0].place.name; Yes. Amisha: You are making assumptions here that the first entry is the first place. But the entry might be empty. You should use query.filledPoints and not query.points. filledPoints is a filtered version of the points array where all the empty ones are removed.
Sorry for noise! An old draft was included! The below is the one I wanted to leave: > > ::: src/printLayout.js > @@ +160,3 @@ > + let layout = PangoCairo.create_layout(cr); > + let query = Application.routeService.query; > + let from = query.points[0].place.name; > > Yes. > > Amisha: You are making assumptions here that the first entry is the first > place. But the entry might be empty. > You should use query.filledPoints and not query.points. filledPoints is a > filtered version of the points array where all the empty ones are removed.
Created attachment 321035 [details] [review] Add Print Route Feature A review based on code refactoring, design improvement and additional functionality will be helpful. Thanks.
Created attachment 321054 [details] [review] Add Print Route Feature A review based on code refactoring, design improvement and additional functionality will be helpful. Thanks.
Review of attachment 321054 [details] [review]: Great Work moving stuff to the base class! ::: src/longPrintLayout.js @@ +35,3 @@ + SCALE_RIGHT_MARGIN: 0.03, + ZOOM_LEVEL: 18 +}; Would it be possible to explain where these ratios come from? @@ +77,3 @@ + + /* x-offset is increased temporarily for rendering instructions */ + let _x = this._offSetX; Maybe call this tmpX ? @@ +86,3 @@ + this._offSetY += dy; + }.bind(this)); + this._offSetX = _x; Couldn't we do this just for the instructions that are part of endLocations or startLocations? ::: src/printLayout.js @@ +142,3 @@ + height: height, + zoom_level: zoomLevel }); + view.set_map_source(mapSource); again: set this as part of the other properties, nothing is special about the map_source, property, right? ..., zoom_level: zoomLevel, map_source: mapSource }); @@ +149,3 @@ + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { + if (view.state === Champlain.State.DONE) { And what if we never get here? Or if it takes 30 seconds to get here? @@ +151,3 @@ + if (view.state === Champlain.State.DONE) { + view.disconnect(notifyId); + let surface = view.to_surface(true); Again, handle the !surface case. @@ +169,3 @@ + + return locationArray; + }, Remove the newlines in this function @@ +191,3 @@ + instructionWidget.height_request = height; + + /* Following handler paint the background of the entry to be transparent */ Nice comment! @@ +219,3 @@ + from = this._formatString(from); + let to = query.points[query.filledPoints.length - 1].place.name; + to = this._formatString(to); Can't we move the query calls to the formatString function as well? To try to get less code here and remnove the wall of let ... = ; from here? let from = this._formatName(0); let to = this._formatName(-1); Should be enough, instead of: let query = Application.routeService.query; let from = query.filledPoints[0].place.name; from = this._formatString(from); let to = query.points[query.filledPoints.length - 1].place.name; to = this._formatString(to); ::: src/printOperation.js @@ +32,3 @@ + _init: function(params) { + PrintLayout.SUPPORTED_TYPES.push(LongPrintLayout.LongPrintLayout); + PrintLayout.SUPPORTED_TYPES.push(ShortPrintLayout.ShortPrintLayout); This feels really odd and convoluted... Could this be: PrintLayout.short = ShortPrintLayout.ShortPrintLayout; PrintLayout.long = LongPrintLayout.LongPrintLayout; ? @@ +71,3 @@ + if (result === Gtk.PrintOperationResult.ERROR) { + let error = this._operation.get_error(); + Utils.debug('Failed to print: %s'.format(error)); So what more happens here? The user will not see the debug print. When does this happen and what is the UX/UI for it? How does this affect the user? Have you encountered this? ::: src/shortPrintLayout.js @@ +58,3 @@ + this._offSetY += dy; + }.bind(this)); + } So small! :D Great work!
Review of attachment 321054 [details] [review]: ::: src/longPrintLayout.js @@ +86,3 @@ + this._offSetY += dy; + }.bind(this)); + this._offSetX = _x; Something like (untested) for (index in this._route.turnPoints) { let turnPoint = this._route.turnPoints[index]; dy = this._instructionHeight + this._instructionBottomMargin; this._adjustPage(dy); if (index <= nthStartLocation || index >= nthEndLocation) this_offsetX = tmpX + this._miniMapViewWidth + this._miniMapViewRightMargin; else this._offsetX = tmpX; ... ... } And move the end calculations before?
Review of attachment 321054 [details] [review]: ::: src/longPrintLayout.js @@ +86,3 @@ + this._offSetY += dy; + }.bind(this)); + this._offSetX = _x; Actually tested: (indention may be off) /* * Fixed number of locations are plotted on minimaps which requires a * check on instructions bound. Later on this can be made dynamic * depending upon factors like total number of instructions, * complexity of neighbourhood areas, etc. */ let nthStartLocation = Math.min(_MINIMAP_LOCATIONS_NUM, locationsLength); let startLocations = this._createLocationArray(0, nthStartLocation); let firstEndLocation = Math.max(0, locationsLength - _MINIMAP_LOCATIONS_NUM); let endLocations = this._createLocationArray(firstEndLocation, locationsLength); this._drawMapView(this._miniMapViewWidth, this._miniMapViewHeight, this._miniMapViewZoomLevel, startLocations); /* x-offset is increased temporarily for rendering instructions */ let tmpX = this._offSetX; for (let index in this._route.turnPoints) { let turnPoint = this._route.turnPoints[index]; dy = this._instructionHeight + this._instructionBottomMargin; this._adjustPage(dy); let mapSize = this._miniMapViewWidth + this._miniMapViewRightMargin; if (index <= nthStartLocation || index >= firstEndLocation) this._offSetX = tmpX + mapSize; else this._offSetX = tmpX; this._drawInstruction(this._instructionWidth, this._instructionHeight, turnPoint); this._offSetY += dy; } this._offSetX = tmpX; this._offSetY = Math.max(0, this._offSetY - this._miniMapViewHeight); this._drawMapView(this._miniMapViewWidth, this._miniMapViewHeight, this._miniMapViewZoomLevel, endLocations); }
Review of attachment 321054 [details] [review]: Thanks Jonas for the review. ::: src/longPrintLayout.js @@ +35,3 @@ + SCALE_RIGHT_MARGIN: 0.03, + ZOOM_LEVEL: 18 +}; Maybe the comment can be like /* All following constants are ratios of different surface size to page size */ Because otherwise these number are purely arbitrary and are taken just to get a good print. @@ +77,3 @@ + + /* x-offset is increased temporarily for rendering instructions */ + let _x = this._offSetX; yeah. sure @@ +86,3 @@ + this._offSetY += dy; + }.bind(this)); + this._offSetX = _x; This is really great, Jonas. But one issue which can arise in this is, it is not assured that n locations which are plotted on minimaps will always have their respective instruction alongside them. There is no relation between minimap size, instruction size and _MINIMAP_LOCATIONS_NUM as of now. ::: src/printLayout.js @@ +142,3 @@ + height: height, + zoom_level: zoomLevel }); + view.set_map_source(mapSource); Yes i tried that, but doing it is giving results like this: imgur.com/e3teHVV I am still trying to find the reason behind. @@ +149,3 @@ + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { + if (view.state === Champlain.State.DONE) { Yes to be added. if timeout, we can emit a signal - timout and provide notification to user in notification bar . Maybe "Your interet connection is too slow" . I checked for mapView. I couldn't find something like timeout in that. @@ +169,3 @@ + + return locationArray; + }, okay @@ +191,3 @@ + instructionWidget.height_request = height; + + /* Following handler paint the background of the entry to be transparent */ Thanks. @@ +219,3 @@ + from = this._formatString(from); + let to = query.points[query.filledPoints.length - 1].place.name; + to = this._formatString(to); Yes nice one. Will refactor. and there is issue with using place.name . When user searches for route using context menu (Route from here and Route to here), place.name seems undefined. Maybe we need to getreverse geocode using place.location. What say? ::: src/printOperation.js @@ +32,3 @@ + _init: function(params) { + PrintLayout.SUPPORTED_TYPES.push(LongPrintLayout.LongPrintLayout); + PrintLayout.SUPPORTED_TYPES.push(ShortPrintLayout.ShortPrintLayout); Okay. @@ +71,3 @@ + if (result === Gtk.PrintOperationResult.ERROR) { + let error = this._operation.get_error(); + Utils.debug('Failed to print: %s'.format(error)); These are the kind of errors : https://developer.gnome.org/gtk3/stable/gtk3-High-level-Printing-API.html#GTK-PRINT-OPERATION-RESULT-ERROR:CAPS As a part of UI, we can notify users the error in the notification bar and the probably solution is to get GTK library updated. Maybe Alex can help us with this better. ::: src/shortPrintLayout.js @@ +58,3 @@ + this._offSetY += dy; + }.bind(this)); + } :)
Review of attachment 321054 [details] [review]: ::: src/longPrintLayout.js @@ +35,3 @@ + SCALE_RIGHT_MARGIN: 0.03, + ZOOM_LEVEL: 18 +}; to what page size? @@ +86,3 @@ + this._offSetY += dy; + }.bind(this)); + this._offSetX = _x; Yeah, you are right, we need to similar code but check that offSetY + dy is < miniMapViewHeight + offset_where_it_started_to_draw or something like that, yeah. Maybe just file a bug. ::: src/printLayout.js @@ +142,3 @@ + height: height, + zoom_level: zoomLevel }); + view.set_map_source(mapSource); Ok, some champlain bug mby, nevermind then! :) @@ +149,3 @@ + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { + if (view.state === Champlain.State.DONE) { When will this happen? When you press preview? Will the printdialog crash? Maybe similate by disabling our code that detectts if network goes down, and turn off internet after pressing print button? @@ +219,3 @@ + from = this._formatString(from); + let to = query.points[query.filledPoints.length - 1].place.name; + to = this._formatString(to); I see how you think but I do not think using reverse geocode is the way forward. We have no control of where that point will be, might be off, and when you are using routing from a lat/lon either by writing or right click the map, you are after a really exact coordinate. Instead we should handle this like we handle it in placeEntry: Something like: let name; let place = query.filledPoints[0].place; if (place.name) name = place.name else p.location.latitude + ', ' + p.location.longitude; ::: src/printOperation.js @@ +71,3 @@ + if (result === Gtk.PrintOperationResult.ERROR) { + let error = this._operation.get_error(); + Utils.debug('Failed to print: %s'.format(error)); I just don't understand what kind of errors that can be... and if it makes sense to tell the user about it, or if it will be obvious, maybe leave it for now
Review of attachment 321054 [details] [review]: Awesome, nicely done, we're almost there! ::: src/printLayout.js @@ +35,3 @@ + +/* Following constant has unit as meters */ +const _MIN_DISTANCE = 3000; Maybe _SHORT_LAYOUT_MAX_DISTANCE @@ +65,3 @@ + return new SUPPORTED_TYPES[1]({ route: route, + pageWidth: pageWidth, + pageHeight: pageHeight }); Why you are using SUPPORTED_TYPES again? it was ok before. Please remove SUPPORTED_TYPES. @@ +91,3 @@ + this._mapViewHeight = _MapView.SCALE_Y * this._pageHeight; + this._mapViewBottomMargin = _MapView.SCALE_BOTTOM_MARGIN * this._pageHeight; + this._mapViewZoomLevel = _MapView.ZOOM_LEVEL; I think these variables that makes scale calculations must be local variables of the "render" method (Same for Short/Long layouts) @@ +148,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { What about Utils.once(view, 'notify::state', ... @@ +149,3 @@ + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { + if (view.state === Champlain.State.DONE) { What if we add an Abort Dialog like Nautilus that only appears when the printing takes more than 3 seconds. I made something while I was playing to see if it'd work: http://pastebin.com/YHxDJFzz @@ +219,3 @@ + from = this._formatString(from); + let to = query.points[query.filledPoints.length - 1].place.name; + to = this._formatString(to); Name the method Jonas has mentioned as _formatQueryPlaceName @@ +224,3 @@ + + layout.set_text(header, -1); + layout.set_height(height); Pango.units_from_double(height) @@ +225,3 @@ + layout.set_text(header, -1); + layout.set_height(height); + layout.set_font_description(desc); Don't you like centered text for the header? Because I like centered text for the header! Don't you Andreas!? :D @@ +249,3 @@ + this.surfaceObjects[this.numPages - 1] = []; + this._offSetX = 0; + this._offSetY = 0; Ignore this if you don't like it, but, what about _cursorX and _cursorY? ::: src/printOperation.js @@ +46,3 @@ + let route = Application.routeService.route; + let width = context.get_page_setup().get_page_width(Gtk.Unit.POINTS); + let height = context.get_page_setup().get_page_height(Gtk.Unit.POINTS); What happened here? it doesn't work ok if you use get_width/get_height?
Review of attachment 321054 [details] [review]: 50 max chars on the first line, then a newline, then 72 chars max on subsequent lines of the commit message plz! https://wiki.gnome.org/Git/CommitMessages Great job! Several of my comments from the last review (https://bugzilla.gnome.org/review?bug=746790&attachment=320906) are still unaddressed though (in particular in data/ui/main-window.ui). ::: data/ui/help-overlay.ui @@ +31,3 @@ + </object> + </child> + <child> Great! It might be marginally cleaner to have this shortcut listed after the "Toggle route planner" one because conceptually it's dependent on the route planner being open. ::: src/printLayout.js @@ +211,3 @@ + let pageNum = this.numPages - 1; + let x = this._offSetX; + let y = this._offSetY; Can you create fewer local variables here? If it's possible try to use this._offsetX/Y directly and maybe also Application.routeService.query Too many local variables make things confusing unless they're absolutely needed. @@ +217,3 @@ + let query = Application.routeService.query; + let from = query.filledPoints[0].place.name; + from = this._formatString(from); doing this all on one line is cleaner let from = this._formatString(query.filledPoints[0].place.name); ::: src/printOperation.js @@ +52,3 @@ + + _paginate: function(operation, context) { + if (this._layout.renderFinished === true) this might evaluate to if (true === true), which can be simplified to if (true), so it can just be "if (this._layout.renderFinished)" ::: src/shortPrintLayout.js @@ +38,3 @@ + + /* (Header + map) + instructions */ + let totalSurfaces = 2 + this._route.turnPoints.length; It's better to have this commented, but even better would be a named constant that is defined in the place where we decide to have 1 header and 1 map. Sometimes I try to imagine if I were to change some assumed thing how many different places would I need to change something? Try to keep the number of places required to make a change very small. For example, if we decided to have a header, and two maps, and instructions, how can the code be structured so it's easy to do correctly?
Review of attachment 321054 [details] [review]: Thanks Jonas, Damian and Hashem for the reviews. :) ::: data/ui/help-overlay.ui @@ +31,3 @@ + </object> + </child> + <child> Yeah , right. ::: src/printLayout.js @@ +35,3 @@ + +/* Following constant has unit as meters */ +const _MIN_DISTANCE = 3000; Sure. @@ +65,3 @@ + return new SUPPORTED_TYPES[1]({ route: route, + pageWidth: pageWidth, + pageHeight: pageHeight }); Done as Jonas suggested. PrintLayout.short, PrintLayout.long @@ +91,3 @@ + this._mapViewHeight = _MapView.SCALE_Y * this._pageHeight; + this._mapViewBottomMargin = _MapView.SCALE_BOTTOM_MARGIN * this._pageHeight; + this._mapViewZoomLevel = _MapView.ZOOM_LEVEL; Sure @@ +148,3 @@ + view.ensure_visible(this._route.createBBox(locations), false); + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { Tried.Not working. @@ +149,3 @@ + if (view.state !== Champlain.State.DONE) { + let notifyId = view.connect('notify::state', (function() { + if (view.state === Champlain.State.DONE) { Added. Thanks @@ +211,3 @@ + let pageNum = this.numPages - 1; + let x = this._offSetX; + let y = this._offSetY; Yeah I understand. But if we use it directly, then there can be a lag between two values of this._offSet or some other variable.When this variable is required, it is possible that the original value is changed to some other value because rendering of other surfaces is also taking place at same time. @@ +217,3 @@ + let query = Application.routeService.query; + let from = query.filledPoints[0].place.name; + from = this._formatString(from); Changed it. @@ +224,3 @@ + + layout.set_text(header, -1); + layout.set_height(height); Okay. @@ +225,3 @@ + layout.set_text(header, -1); + layout.set_height(height); + layout.set_font_description(desc); Yeah Centered text might look good. @@ +249,3 @@ + this.surfaceObjects[this.numPages - 1] = []; + this._offSetX = 0; + this._offSetY = 0; Yeah that's actually better name. Thanks. ::: src/printOperation.js @@ +46,3 @@ + let route = Application.routeService.route; + let width = context.get_page_setup().get_page_width(Gtk.Unit.POINTS); + let height = context.get_page_setup().get_page_height(Gtk.Unit.POINTS); No function like get_width /get_width. get_paper_width/get_paper_height exist but that includes non-printable area too. get_page_width /get_page_height gives prinatble area only excluding the margin area. @@ +52,3 @@ + + _paginate: function(operation, context) { + if (this._layout.renderFinished === true) Sure @@ +71,3 @@ + if (result === Gtk.PrintOperationResult.ERROR) { + let error = this._operation.get_error(); + Utils.debug('Failed to print: %s'.format(error)); OKay.fine ::: src/shortPrintLayout.js @@ +38,3 @@ + + /* (Header + map) + instructions */ + let totalSurfaces = 2 + this._route.turnPoints.length; I think as of now, the code is such that it will require the minimum changes as seen from point of scalability. Suggestions are welcomed although :)
Review of attachment 320906 [details] [review]: ::: data/ui/main-window.ui @@ +104,3 @@ + <property name="tooltip-text" translatable="yes">Print Route</property> + <property name="action-name">win.print-route</property> + <property name="valign">center</property> That is because the height of image button is comparable to that of header bar. Try increasing the height of header bar and then use two options. GTK_ALIGN_FILL and GTK_ALIGN_CENTER. GTK_ALIGN_FILL Will increase the size of button to match that of parent(header bar) And GTK_ALIGN_CENTER will make it to exact center of header bar without changing its size. @@ +106,3 @@ + <property name="valign">center</property> + <style> + <class name="image-button"/> I feel this is good option. Let's check what Andreas has to say for the matter. @@ +111,3 @@ + <object class="GtkImage" id="print-route-button-image"> + <property name="visible">True</property> + <property name="icon-size">1</property> It is used to set absolute icon size of gtk image.Even if it's parent changes its size , the size of icon will remain the same as original one.
Created attachment 321261 [details] [review] Add Print Route Feature
Created attachment 321272 [details] [review] Add Print Route Feature
Review of attachment 321272 [details] [review]: Great work! This looks nice enough for me to push! I will do some local modifications, I hop you do not mind. Then I would like you to file some bugs, maybe mark them with keyword "newcomers" and "gnome-love"? Which should we file? * Add markers to map view? * Handle via points? * More dynamic split up of instructions? Thanks Amisha! * Smarter split up
Review of attachment 321272 [details] [review]: Pushed! https://git.gnome.org/browse/gnome-maps/commit/?id=ac88800dccbbc836b6e7a7bd6e5ee20fb225a997
Another bug maybe: * Styling of the print button
Review of attachment 321272 [details] [review]: Thanks everyone for the enormous help and reviews :) Yes, I will file the bugs as following: - Adding markers to map view - Minimaps for via points to be added, right? - Relating instructions set with minimaps, such that only those instructions which are indicated by minimap are to displayed along its side. - Overlapping of instructions is observed at some places when instructions take more than 2 lines. http://i.imgur.com/tEwckla.png . Another bug can be to ellipsize the instructions. - Styling of Print button.What say, Andreas?
(In reply to amisha from comment #108) > - Styling of Print button.What say, Andreas? I think the current styling of the button is good. How would you like to change it?
(In reply to Andreas Nilsson from comment #109) > (In reply to amisha from comment #108) > > > - Styling of Print button.What say, Andreas? > > I think the current styling of the button is good. How would you like to > change it? Hashem suggested some intersting ui. http://i.imgur.com/sqq3KZV.png as it it brings more attention to a button that strangely hides and comes back. And adding some animation sort of thing
(In reply to amisha from comment #110) > (In reply to Andreas Nilsson from comment #109) > > (In reply to amisha from comment #108) > > > > > - Styling of Print button.What say, Andreas? > > > > I think the current styling of the button is good. How would you like to > > change it? > > Hashem suggested some intersting ui. http://i.imgur.com/sqq3KZV.png as it it > brings more attention to a button that strangely hides and comes back. And > adding some animation sort of thing Ah, I see. File a new bug about it, and we can check with the other designers if that's the correct use of highlight. If not, then some kind of animation would probably help with noticing it.
(In reply to Andreas Nilsson from comment #111) > (In reply to amisha from comment #110) > > (In reply to Andreas Nilsson from comment #109) > > > (In reply to amisha from comment #108) > > > > > > > - Styling of Print button.What say, Andreas? > > > > > > I think the current styling of the button is good. How would you like to > > > change it? > > > > Hashem suggested some intersting ui. http://i.imgur.com/sqq3KZV.png as it it > > brings more attention to a button that strangely hides and comes back. And > > adding some animation sort of thing > > Ah, I see. > File a new bug about it, and we can check with the other designers if that's > the correct use of highlight. If not, then some kind of animation would > probably help with noticing it. Sure. Thanks.
There's a bug with dark theme and the printed text. When printing in dark theme the route text matches the system's dark theme and makes it unreadable. I tested this on Fedora 24 Alpha 7 as of today
Created attachment 326050 [details] Route info with dark theme enabled With system dark theme enabled, the route text follows suit and turns white
Currently we create a transparent background for the instructions and print the widget using cairo. In case of dark theme the widget fonts are white and get printed that way. To resolve it we need to change the style of the widget before converting it to cairo surface.
Yeah. That's right. Thanks for notifying it. Whenever we change any visuals of the application, printing does not give good results. We need to decide one option - either to try printing only in specific cases or the other option is setting standards for printing.Which will be used irrespective of the visuals of application. Andreas, Jonas what do you say?
(In reply to amisha from comment #116) > either to try printing only in specific cases or the other > option is setting standards for printing.Which will be used irrespective of > the visuals of application. Do you mean like hardcoding the direction symbols, regardless of icon theme, or hardcoding the colors? Hardcoding the colors seems very straight forward. Printer paper is always white, so going for black symbols and text feels sane.
Created attachment 326289 [details] [review] Change color of instructionEntry before printing set font color of intructionEntry to black manually to avoid printing in lighter font colors according to the theme
Review of attachment 326289 [details] [review]: ::: src/printLayout.js @@ +227,3 @@ instructionWidget.set_valign(Gtk.Align.START); instructionWidget.connect('damage-event', (function(widget) { + instructionEntry.get_style_context().add_class('print-layout'); Why is this done inside the damage event handler?
And this is a closed bug, could we plase open a new one for this issue? To make stuff clearer? Nayan could you attach your patch to a new bug?
Review of attachment 326289 [details] [review]: ::: src/printLayout.js @@ +227,3 @@ instructionWidget.set_valign(Gtk.Align.START); instructionWidget.connect('damage-event', (function(widget) { + instructionEntry.get_style_context().add_class('print-layout'); And the naming of the class could be better as well, something that reflects what it actually does?