GNOME Bugzilla – Bug 762303
Add markers to map view when rendering the print layout
Last modified: 2016-02-25 19:17:47 UTC
As shown in mockup : https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/print-map.png, start, via and end point markers are to be added to mapview while rendering the print layout.
Created attachment 322212 [details] [review] mapMarker: Enable use without mapView
Created attachment 322213 [details] [review] printLayout: Add markers to route
Created attachment 322214 [details] [review] instructionRow: Add property for colored icons
Created attachment 322215 [details] [review] printLayout: Use red marker icons in instrutionRow
Created attachment 322216 [details] Cast of markers
Review of attachment 322214 [details] [review]: Thanks Jonas. This looks really great. Just a query, when we are adding via points markers too on the mapview and instruction row, i feel like we should add minimaps for via points too. Because there can be a requirement by user to move around in that complex area. ::: src/instructionRow.js @@ +46,3 @@ + if (!this._iconColor) { + this._directionImage.icon_name = this.turnPoint.iconName; + } else { Can't this be like if (this._iconColor) { I mean just to make it look little straight forward.
Review of attachment 322214 [details] [review]: Thanks for review! Yes I agree we want via point miniMaps as well, do you think we should add this in this bug as well? ::: src/instructionRow.js @@ +46,3 @@ + if (!this._iconColor) { + this._directionImage.icon_name = this.turnPoint.iconName; + } else { You mean switch the cases? Sure.
Review of attachment 322215 [details] [review]: ::: src/printLayout.js @@ +208,3 @@ visible: true, + turnPoint: turnPoint, + iconColor: turnPoint.isStop() I think the property should be colorIcon , because this name tells that it is a bool value,and iconColor feels like refering to color of icon.What say?
Review of attachment 322215 [details] [review]: ::: src/printLayout.js @@ +208,3 @@ visible: true, + turnPoint: turnPoint, + iconColor: turnPoint.isStop() Yeah you are right... maybe hasColor?
Review of attachment 322215 [details] [review]: ::: src/printLayout.js @@ +208,3 @@ visible: true, + turnPoint: turnPoint, + iconColor: turnPoint.isStop() Yeah, hasColor sounds good.
Review of attachment 322214 [details] [review]: I think we should handle that one in separate ticket. I will report one. But before that, would like to work on smarter split of instructions i.e. associating minimaps with instructions.
Created attachment 322263 [details] [review] mapMarker: Enable use without mapView
Created attachment 322264 [details] [review] printLayout: Add markers to route
Created attachment 322265 [details] [review] instructionRow: Add property for colored icons
Created attachment 322266 [details] [review] printLayout: Use red marker icons in instrutionRow
Created attachment 322267 [details] [review] longPrintLayout: Add via points
Created attachment 322268 [details] Cast of markers with via points
Review of attachment 322267 [details] [review]: ::: src/longPrintLayout.js @@ +95,3 @@ + let viaPoints = this._createTurnPointArray(firstViaPoint, + ++index); + this._cursorX = tmpX; How about adding few instructions before the via point and few ones after the via point. Just a thought.
Created attachment 322279 [details] [review] longPrintLayout: Add via points
Review of attachment 322263 [details] [review]: ::: src/mapMarker.js @@ +60,3 @@ this.connect('notify::size', this._translateMarkerPosition.bind(this)); + if (this._mapView) { + this._view = this._mapView.view; Setting this._view in here would appear to break _positionBubble() when this._view is undefined.
Review of attachment 322265 [details] [review]: Small suggestion ::: src/instructionRow.js @@ +44,3 @@ this._instructionLabel.label = this.turnPoint.instruction; + + if (this._hasColor) { I have no idea how this works. Could you add a comment here?
Review of attachment 322266 [details] [review]: Looks good
Review of attachment 322264 [details] [review]: Looks good. ::: src/printLayout.js @@ +125,3 @@ dy = mapViewHeight + mapViewMargin; this._adjustPage(dy); + let turnPointsLength = this._route.turnPoints.length; I wish you did a rename in a separate commit, but this is fine.
Review of attachment 322263 [details] [review]: Small issue maybe?
Review of attachment 322279 [details] [review]: OK ::: src/longPrintLayout.js @@ +81,3 @@ /* x-cursor is increased temporarily for rendering instructions */ let tmpX = this._cursorX; + for (let index = 0; index < this._route.turnPoints.length; index++) { "i" instead of "index" would be more common. @@ +94,3 @@ + let tmpY = this._cursorY; + first = Math.max(0, index + 1 - (_NUM_MINIMAPS / 2)); + last = Math.min(index + 1 + (_NUM_MINIMAPS / 2), pointsLength); Where is this math coming from? Are we printing 2 turns on either side of a 'via' point? More comments please. @@ +106,3 @@ this._cursorX = tmpX; + first = Math.max(0, pointsLength - _NUM_MINIMAPS); so this prints the last 5 turns? I dont' know enough to understand what's happening. More comments would help me.
Review of attachment 322263 [details] [review]: ::: src/mapMarker.js @@ +60,3 @@ this.connect('notify::size', this._translateMarkerPosition.bind(this)); + if (this._mapView) { + this._view = this._mapView.view; How does this manifest? this._view will never be undefined unless we are doing print.
Review of attachment 322264 [details] [review]: Thanks for review! ::: src/printLayout.js @@ +125,3 @@ dy = mapViewHeight + mapViewMargin; this._adjustPage(dy); + let turnPointsLength = this._route.turnPoints.length; But it is here I turn the array of coordinates to an array of turnPoints, it is part of the change I feel, since we know need turnpoints to get the iconName
Review of attachment 322265 [details] [review]: Thanks! ::: src/instructionRow.js @@ +44,3 @@ this._instructionLabel.label = this.turnPoint.instruction; + + if (this._hasColor) { Yes, good idea! Spoiler alert, it has todo with the icon having -symbolic in its name!
Review of attachment 322279 [details] [review]: Thanks! ::: src/longPrintLayout.js @@ +81,3 @@ /* x-cursor is increased temporarily for rendering instructions */ let tmpX = this._cursorX; + for (let index = 0; index < this._route.turnPoints.length; index++) { yes, but since we use it in calculations and not just as an array index I felt I wanted a proper name, but can rename. @@ +94,3 @@ + let tmpY = this._cursorY; + first = Math.max(0, index + 1 - (_NUM_MINIMAPS / 2)); + last = Math.min(index + 1 + (_NUM_MINIMAPS / 2), pointsLength); Yes, it was Amishas suggestions from previous review, that for start points we show the NUM_MINIMAP from start, for END we show the NUM_MINIMAP leading towards end. And for via we show NUM_MINIMAP/2 before and after via. @@ +106,3 @@ this._cursorX = tmpX; + first = Math.max(0, pointsLength - _NUM_MINIMAPS); Yes, exactly. Will provide a comment above all this to explain.
Created attachment 322338 [details] [review] mapMarker: Enable use without mapView
Created attachment 322339 [details] [review] printLayout: Add markers to route
Created attachment 322340 [details] [review] instructionRow: Add property for colored icons
Created attachment 322341 [details] [review] printLayout: Use red marker icons in instrutionRow
Created attachment 322342 [details] [review] longPrintLayout: Add via points
Review of attachment 322342 [details] [review]: Looks good. But this adds whitespace errors (at end of line). View with git show --check.
Review of attachment 322264 [details] [review]: ::: src/printLayout.js @@ +125,3 @@ dy = mapViewHeight + mapViewMargin; this._adjustPage(dy); + let turnPointsLength = this._route.turnPoints.length; OK
Review of attachment 322263 [details] [review]: OK ::: src/mapMarker.js @@ +60,3 @@ this.connect('notify::size', this._translateMarkerPosition.bind(this)); + if (this._mapView) { + this._view = this._mapView.view; I suppose it won't, it just seems like something that could easily be broken or overlooked in a refactor. It's fine for now though thanks.
Review of attachment 322338 [details] [review]: OK
Review of attachment 322339 [details] [review]: Nice!
Review of attachment 322340 [details] [review]: Nice!
Review of attachment 322341 [details] [review]: Nice!
Attachment 322338 [details] pushed as 62604db - mapMarker: Enable use without mapView Attachment 322339 [details] pushed as b8dfaab - printLayout: Add markers to route Attachment 322340 [details] pushed as 2dc4d8e - instructionRow: Add property for colored icons Attachment 322341 [details] pushed as 0e70d4a - printLayout: Use red marker icons in instrutionRow Attachment 322342 [details] pushed as 0273c83 - longPrintLayout: Add via points