GNOME Bugzilla – Bug 737775
Add buttons to MapBubbles
Last modified: 2014-12-12 09:04:02 UTC
I want to add buttons to MapBubbles. How about making the MapBubble have a template UI. With the icon, a content area and a button area. The button area will have the stanard buttons and the sub classes can opt in to the buttons they want. https://raw.github.com/gnome-design-team/gnome-mockups/020b66b292801ae281c54cb71b11d2eea8536edf/maps/v2/markers-and-bubbles.png This is the current mockup. Atm we only have the route button as a possible button to add.
Created attachment 287571 [details] [review] Add via entries by adding points to RouteQuery Refactor code so that adding a point to the route query will add an entry in the sidebar. This allows us to add via entries from other places than the sidebar module.
Created attachment 287572 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.buttons.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Review of attachment 287572 [details] [review]: ::: src/mapBubble.js @@ +59,3 @@ + this._icon = ui.bubbleIcon; + this._content = ui.bubbleContentArea; + this._buttonArea = ui.bubbleCustomButtonArea; No custom area in this version, unsure if ever needed. @@ +96,3 @@ + + get buttonArea() { + return this._customButtonArea; This is a leftover from when I had this.
Review of attachment 287571 [details] [review]: ::: src/sidebar.js @@ +136,3 @@ + return; + else + this._createViaRow(listbox, point, index); Could be written without the else.
Review of attachment 287571 [details] [review]: ::: src/routeQuery.js @@ +103,2 @@ removePoint: function(index) { + let points = this._points.splice(index, 1); What about removedPoints for the name of this variable? sounds more clear. @@ +107,2 @@ this.notify('points'); + this.emit('point-removed', point, index); I think we should not emit any signal if points.length === 0 (or point === null), since no points were removed. ::: src/sidebar.js @@ +92,3 @@ + let query = Application.routeService.query; + + query.addPoint(query.points.length - 1); What about accept -1 as parameter to insert a point to the last position as GtkListStore does? or do you prefer to keep the capabilities of Array.splice first parameter? @@ +141,3 @@ + query.connect('point-removed', (function(obj, point, index) { + let row = listbox.get_row_at_index(index - 1); + listbox.remove(row); If we do this, then take a look to the _initInstructionList method, where we connect the route::reset signal, we need to change the code that removes the via points for something like: for (let i = 1; i < query.points.length - 1; i++) query.removePoint(i); @@ +163,3 @@ + point, 'place', + GObject.BindingFlags.BIDIRECTIONAL); + ui.viaEntryGrid.add(entry); Hmmm... I don't like this part, it looks like we have repeated code. What about this? - In Sidebar::_init, add two initial points to the query - Don't add a new point in _initRouteEntry replace, but do "let point = Application.routeService.query.points[pointIndex]" - Call _initRouteEntry here as before (and you can skip the point argument in this method).
Review of attachment 287572 [details] [review]: This is pretty nice! ::: src/map-bubble.ui @@ +1,2 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- Generated with glade 3.18.3 --> Glad you have used Glade :) ::: src/mapBubble.js @@ +77,3 @@ + point.place = this._place; + } + }).bind(this)); I was thinking about a button with a menu associated to select between "Route from here", "Route to here" and "Add via point", the last item must be not visible if we don't have filled points. Can we discuss it with designers? @@ +79,3 @@ + }).bind(this)); + } + Should we hide the button container if a bubble decides to not show any buttons? @@ +84,3 @@ + + get image() { + return this._icon; Then you should name it this._image and ui.bubbleImage :D
Review of attachment 287572 [details] [review]: ::: src/map-bubble.ui @@ +42,3 @@ + <property name="margin-top">10</property> + <child> + <object class="GtkGrid" id="bubble-standard-button-area"> Since we are going to have multiple buttons in the near future and we surely want to use linked CSS class here, we must add the linked class to the container and convert it to a GtkBox to avoid visual glitches: https://bugzilla.gnome.org/show_bug.cgi?id=735839
Review of attachment 287571 [details] [review]: Thx for review! ::: src/routeQuery.js @@ +103,2 @@ removePoint: function(index) { + let points = this._points.splice(index, 1); Yeah, maybe or removedPoint, it will at most be one, but it will be in the form of an array so removedPoints could do. @@ +107,2 @@ this.notify('points'); + this.emit('point-removed', point, index); Yeah, fair point. ::: src/sidebar.js @@ +92,3 @@ + let query = Application.routeService.query; + + query.addPoint(query.points.length - 1); Yeah, sounds ok. @@ +141,3 @@ + query.connect('point-removed', (function(obj, point, index) { + let row = listbox.get_row_at_index(index - 1); + listbox.remove(row); Yeah, you are right, nice catch. And the code here should be row.destroy as well. Well the loop could stay as it is right? in _initInstructionList, and just omit the row.destroy call? @@ +163,3 @@ + point, 'place', + GObject.BindingFlags.BIDIRECTIONAL); + ui.viaEntryGrid.add(entry); Agreed, thought the same when I wrote it but couldn't think of a nice way. Yeah that could work if we do it before we connect the signals that listen to add/remove point signals.
Created attachment 287694 [details] [review] Add via entries by adding points to RouteQuery Refactor code so that adding a point to the route query will add an entry in the sidebar. This allows us to add via entries from other places than the sidebar module.
Created attachment 287695 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.buttons.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Review of attachment 287572 [details] [review]: ::: src/map-bubble.ui @@ +1,2 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- Generated with glade 3.18.3 --> Reluctantly :) @@ +42,3 @@ + <property name="margin-top">10</property> + <child> + <object class="GtkGrid" id="bubble-standard-button-area"> Yes! Great catch, you are right. ::: src/mapBubble.js @@ +77,3 @@ + point.place = this._place; + } + }).bind(this)); Yes, we should. Maybe a popover when we press the button. Should route from here and route to here always be visible? Even when stuff is already there? I guess it should. And the last only if from and to is filled? @@ +79,3 @@ + }).bind(this)); + } + Not sure. Will it really make a difference? I guess their might be extra margin, but maybe we want that anyway. Hmm. @@ +84,3 @@ + + get image() { + return this._icon; Fair enough :)
For me there is a bug with this, do you see it? I added a popover for when you press the route button, but it appears behind the mapBubble popover. It is visible if you fill in from and to, you see it above the bubble, why is this? Also if a mapBubble is shown and you search in the routing sidebar I do not see the search popover. Is it hidden behind the sidebar? What is this magic?
Created attachment 287810 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 287820 [details] [review] sidebar: Reveal sidebar when query is updated
Created attachment 287821 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Something like this? Made the route button a GtkMenuButton and set the popover property in the ui file. What do we think about this?
Created attachment 287826 [details] [review] mainWindow: Reveal sidebar when query is updated
Created attachment 287827 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Screencast: https://www.youtube.com/watch?v=kRQ72ZrIQkQ&feature=youtu.be
Created attachment 287831 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...); * Added translatable="yes"
(In reply to comment #19) > Screencast: > > https://www.youtube.com/watch?v=kRQ72ZrIQkQ&feature=youtu.be Looks cool. :)
(In reply to comment #16) > Something like this? Made the route button a GtkMenuButton and set the popover > property in the ui file. > > What do we think about this? Hmmm... I think in this case we don't have to use popover, but a classic menu. Since this button is already inside a popover, using another nested popover for the button looks like the route popover is not attached to the button. Also, what about a down arrow along side the route image? could that look good? Sorry I cannot try it, writing a comment is already too much distraction from my studies haha
Review of attachment 287831 [details] [review]: ::: src/map-bubble.ui @@ +52,3 @@ + <object class="GtkMenuButton" id="bubble-route-button"> + <property name="name">bubble-route-button</property> + <property name="popover">bubble-route-popover</property> I now I said that I prefer to not use popover, but why this? I think using use-popover and menu-model properties is a better approach.
(In reply to comment #22) > (In reply to comment #16) > > Something like this? Made the route button a GtkMenuButton and set the popover > > property in the ui file. > > > > What do we think about this? > > Hmmm... I think in this case we don't have to use popover, but a classic menu. > Since this button is already inside a popover, using another nested popover for > the button looks like the route popover is not attached to the button. > Ok! I seem to be doing this a lot, putting maps in maps and popovers in popovers :) > Also, what about a down arrow along side the route image? could that look good? > Sorry I cannot try it, writing a comment is already too much distraction from > my studies haha Hmm, I dunno, how do you mean exactly? Inside the button?
Review of attachment 287831 [details] [review]: ::: src/map-bubble.ui @@ +52,3 @@ + <object class="GtkMenuButton" id="bubble-route-button"> + <property name="name">bubble-route-button</property> + <property name="popover">bubble-route-popover</property> Well, when using menu-model I can only rely on actions, right? There is no "on click" or "selected" that I can see. And with actions I can't send the place a long, right?
Created attachment 287913 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 287915 [details] Screencast of route button with GtkMenu
What about putting two aligned buttons (three if "via" is needed)? It seems to me that there is enough space and it would be easier select the needed option.
(In reply to comment #28) > What about putting two aligned buttons (three if "via" is needed)? > > It seems to me that there is enough space and it would be easier select the > needed option. Yeah, I am not sure. That takes up a lot of place. We want at least two (three?) more buttons here. The bookmark/favorite button, the check-in button (if the user has an account) and the mockups have some kind of "share" button. And we might decide that we want more. We don't want to make this to cluttered. But I am not a designer by any means, so I am not sure.
Taking a step back, the most straight forward thing would be to have this route button trigger the sidebar I think. Mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png What do you all think?
(In reply to comment #30) > Taking a step back, the most straight forward thing would be to have this route > button trigger the sidebar I think. > > Mockup: > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > What do you all think? I think I prefer a workflow where the route button does not open a menu. And I think we should disregard what is currently in route-entries when the button is pressed. So I like this. It gets a bit icky when we need different behavior depending on the type of bubble, tho. Not sure about that.
Btw Damián: Maybe this patch should be split in two? One that adds the template and one that adds the route-button. That way we could commit the template and base your check-in patches on it, so that it finally can go in? :)
Created attachment 288637 [details] [review] Add via entries by adding points to RouteQuery Refactor code so that adding a point to the route query will add an entry in the sidebar. This allows us to add via entries from other places than the sidebar module.
Created attachment 288638 [details] [review] mainWindow: Reveal sidebar when query is updated
Created attachment 288639 [details] [review] queryPoint: Initialize place property to null
Created attachment 288640 [details] [review] routeQuery: notify on point added
Created attachment 288641 [details] [review] sidebar: Add route reverse button Add a reverse button next to the 'to' route entry. The button will be visible when there is no 'via' points and when at least one of 'to' or 'from' has a place attached to it. When pressed will switch 'to' and 'from'
Created attachment 288642 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 288643 [details] [review] mapBubble: Add route button
Created attachment 288649 [details] [review] mapBubble: Add route button
Screencast: http://youtu.be/NOYa2np3pes
Created attachment 288668 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 288669 [details] [review] mapBubble: Add route button
Created attachment 288670 [details] route reverse icon
Created attachment 288672 [details] [review] sidebar: Add route reverse button Add a reverse button next to the 'to' route entry. The button will be visible when there is no 'via' points and when at least one of 'to' or 'from' has a place attached to it. When pressed will switch 'to' and 'from'
Created attachment 288673 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 288674 [details] [review] mapBubble: Add route button
Created attachment 288675 [details] [review] mapBubble: Add route button
Created attachment 288676 [details] [review] mapBubble: Add route button
Thanks Andreas! New cast: http://youtu.be/75bzb4MZd2w
(In reply to comment #50) > Thanks Andreas! > > New cast: http://youtu.be/75bzb4MZd2w All seems really nice, though shouldn't hitting the 'routing' button on 'current location' update only the 'From' and not clear the route?
(In reply to comment #51) > (In reply to comment #50) > > Thanks Andreas! > > > > New cast: http://youtu.be/75bzb4MZd2w > > All seems really nice, though shouldn't hitting the 'routing' button on > 'current location' update only the 'From' and not clear the route? Not sure about that. The sidebar might be closed and have an old route inside of it, that was done some time ago. Then when we press route from, we update the from field and trigger a route search to the places that were previously in the to/via fields. I think I like the idea that pressing the route-button on a bubble is an intent to start a new route plan. But maybe the feeling of that is different depending on if the sidebar is open or not? So if the sidebar is closed we clear the query/route and if it is open we replace the from/to field? Andreas?
(In reply to comment #52) > Not sure about that. The sidebar might be closed and have an old route inside > of it, that was done some time ago. Then when we press route from, we update > the from field and trigger a route search to the places that were previously in > the to/via fields. > > I think I like the idea that pressing the route-button on a bubble is an intent > to start a new route plan. But maybe the feeling of that is different depending > on if the sidebar is open or not? So if the sidebar is closed we clear the > query/route and if it is open we replace the from/to field? > > Andreas? I went back and forth a bit on this yesterday. I think it would be most predictable and straight forward to always start a new route when pressing the route button in the bubble. The extra from/to logic would be a nice bonus in some situations, but unfortunately get in the way and add unpredictability in others.
Review of attachment 288673 [details] [review]: Despite the following, this looks nice. ::: src/mapBubble.js @@ +60,3 @@ + 'bubble-route-from-item', + 'bubble-route-to-item', + 'bubble-route-via-item']); button-route-* widgets must not be here.
(In reply to comment #32) > Btw Damián: Maybe this patch should be split in two? One that adds the template > and one that adds the route-button. That way we could commit the template and > base your check-in patches on it, so that it finally can go in? :) That'd be lovely :) . We are fine with that, there are not visible changes to the user by committing that patch.
(In reply to comment #30) > Taking a step back, the most straight forward thing would be to have this route > button trigger the sidebar I think. > > Mockup: > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > What do you all think? Grrrr... I don't know, I'm not convinced with this... it's not versatile as the menu approach and doesn't support via points (like a button Add destination or Add via point), also I don't like how the route button goes away when the route has via points.
(In reply to comment #56) > (In reply to comment #30) > > Taking a step back, the most straight forward thing would be to have this route > > button trigger the sidebar I think. > > > > Mockup: > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > > > What do you all think? > > Grrrr... I don't know, I'm not convinced with this... it's not versatile as the > menu approach and doesn't support via points (like a button Add destination or > Add via point), also I don't like how the route button goes away when the route > has via points. I agree with Andreas with keeping the routing functionality in the sidebar. And not spreading the logic out over multiple places. Also I'm having an hard time seeing the use case of adding a via point from a bubble. When is it useful? When trying to compose a route without using the sidebar at all? Searching for one place, going to it, pressing route, then searching for another, goining to it pressing route? Seems tedious and should be better done from the sidebar alone. Or you have the sidebar open, construct a route. Then search for a place and press route? It feels awkward to me. I think keeping the routing logic in the sidebar makes more sense. And having convenience button on the bubble that helps you get to the sidebar with some additional information.
Created attachment 289251 [details] [review] Add via entries by adding points to RouteQuery Refactor code so that adding a point to the route query will add an entry in the sidebar. This allows us to add via entries from other places than the sidebar module.
Created attachment 289252 [details] [review] mainWindow: Reveal sidebar when query is updated
Created attachment 289253 [details] [review] queryPoint: Initialize place property to null
Created attachment 289254 [details] [review] routeQuery: notify on point added
Created attachment 289255 [details] [review] sidebar: Add route reverse button Add a reverse button next to the 'to' route entry. The button will be visible when there is no 'via' points and when at least one of 'to' or 'from' has a place attached to it. When pressed will switch 'to' and 'from'
Created attachment 289256 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 289257 [details] [review] mapBubble: Add route button
(In reply to comment #57) > (In reply to comment #56) > > (In reply to comment #30) > > > Taking a step back, the most straight forward thing would be to have this route > > > button trigger the sidebar I think. > > > > > > Mockup: > > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > > > > > What do you all think? > > > > Grrrr... I don't know, I'm not convinced with this... it's not versatile as the > > menu approach and doesn't support via points (like a button Add destination or > > Add via point), also I don't like how the route button goes away when the route > > has via points. > > I agree with Andreas with keeping the routing functionality in the sidebar. And > not spreading the logic out over multiple places. Also I'm having an hard time > seeing the use case of adding a via point from a bubble. When is it useful? > > When trying to compose a route without using the sidebar at all? Searching for > one place, going to it, pressing route, then searching for another, goining to > it pressing route? Seems tedious and should be better done from the sidebar > alone. FWIW, I agree too with this.
(In reply to comment #56) > (In reply to comment #30) > > Taking a step back, the most straight forward thing would be to have this route > > button trigger the sidebar I think. > > > > Mockup: > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > > > What do you all think? > > Grrrr... I don't know, I'm not convinced with this... it's not versatile as the > menu approach and doesn't support via points (like a button Add destination or > Add via point), also I don't like how the route button goes away when the route > has via points. Btw, what do you mean by "I don't like how the route button goes away when the route has via points"? I don't think that should happen.
Created attachment 289264 [details] [review] mapBubble: Add route button
Created attachment 289265 [details] [review] routeQuery: Avoid multiple notifies in reset
Review of attachment 289264 [details] [review]: ::: src/mapBubble.js @@ +96,3 @@ + query.freeze_notify(); + query.reset(); + route.reset(); Perhaps we need a Application.routeService.reset()?
(In reply to comment #66) > (In reply to comment #56) > > (In reply to comment #30) > > > Taking a step back, the most straight forward thing would be to have this route > > > button trigger the sidebar I think. > > > > > > Mockup: > > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > > > > > What do you all think? > > > > Grrrr... I don't know, I'm not convinced with this... it's not versatile as the > > menu approach and doesn't support via points (like a button Add destination or > > Add via point), also I don't like how the route button goes away when the route > > has via points. > > Btw, what do you mean by "I don't like how the route button goes away when the > route has via points"? I don't think that should happen. Do you mean the route-reverse button? That it should instead reverse all the via-points as well?
(In reply to comment #66) > Btw, what do you mean by "I don't like how the route button goes away when the > route has via points"? I don't think that should happen. Sorry, I got confused, I thought the route button got insensitive when user adds a new via point (a combination of not reading the mockup text properly and got confused because the route button background color in the last mockup image :( ). (In reply to comment #57) > (In reply to comment #56) > > (In reply to comment #30) > > > Taking a step back, the most straight forward thing would be to have this route > > > button trigger the sidebar I think. > > > > > > Mockup: > > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/route-button.png > > > > > > What do you all think? > > > > Grrrr... I don't know, I'm not convinced with this... it's not versatile as the > > menu approach and doesn't support via points (like a button Add destination or > > Add via point), also I don't like how the route button goes away when the route > > has via points. > > I agree with Andreas with keeping the routing functionality in the sidebar. And > not spreading the logic out over multiple places. Also I'm having an hard time > seeing the use case of adding a via point from a bubble. When is it useful? > > When trying to compose a route without using the sidebar at all? Searching for > one place, going to it, pressing route, then searching for another, goining to > it pressing route? Seems tedious and should be better done from the sidebar > alone. > > Or you have the sidebar open, construct a route. Then search for a place and > press route? It feels awkward to me. I think keeping the routing logic in the > sidebar makes more sense. And having convenience button on the bubble that > helps you get to the sidebar with some additional information. There are many and increasingly ways to reach the situation in which Maps shows a bubble: current location, searching, "What's here?" button, and in the future friend's check-in and opening URIs. If there is no versatility in the route button, there are many possibilities that the user to fail to use the route button (I tried to use the route button by myself by setting the From and To using this button and failed), at some point she gives up and fallback on search the location directly on the sidebar. In my understanding, that's annoying, immediately I would think while staring the bubble "Why I need to manually write a place in the sidebar if I already found it?". You are mis-supposing which are the common or natural usage of this, when adding versatility to this doesn't clutter the UI so much (the menu is hidden behind a button), is not confusing (moreover, is more predictable from my POV) and there are not technical limitations.
Review of attachment 289256 [details] [review]: ::: src/mapBubble.js @@ +29,3 @@ +const Button = { + NONE: 0 +} Missed semicolon here? @@ +44,3 @@ delete params.mapView; + let buttonFlags = params.buttons || 0; What about "params.buttons || Button.NONE"? ::: src/turnPointBubble.js @@ +40,1 @@ ui.image.icon_name = turnPoint.iconName; this.image instead of ui.image? Also, I see you manually set the size of the icon in userLocationBubble.js, should we do the same thing here?
(In reply to comment #71) > > There are many and increasingly ways to reach the situation in which Maps shows > a bubble: current location, searching, "What's here?" button, and in the future > friend's check-in and opening URIs. > > If there is no versatility in the route button, there are many possibilities > that the user to fail to use the route button (I tried to use the route button > by myself by setting the From and To using this button and failed), at some > point she gives up and fallback on search the location directly on the sidebar. > In my understanding, that's annoying, immediately I would think while staring > the bubble "Why I need to manually write a place in the sidebar if I already > found it?". > I don't really see it. I search for something, I wonder how to get there. I press the route button. And the route-planner opens, with the location in the To field and the current location in the From field. And I am now in route planning mode. If I want to switch the places I press the route-reverse button. For the userlocation it will be added to the From field. So the button does something. It is an intent to start route-planning. I don't see the usecase where I search for something, press route button, then use something else to get another bubble and use that route button to continue planning. The sidebar for me is _the_ route planner. Pressing a route button on a bubble takes me there with information added. > You are mis-supposing which are the common or natural usage of this, when > adding versatility to this doesn't clutter the UI so much (the menu is hidden > behind a button), is not confusing (moreover, is more predictable from my POV) > and there are not technical limitations. I disagree. I think the sidebar should be the route planner. And the route buttons on the bubbles are nice ways to get to the route planner with a clear intent.
Review of attachment 289256 [details] [review]: Thanks for review! ::: src/mapBubble.js @@ +29,3 @@ +const Button = { + NONE: 0 +} Seems like it! @@ +44,3 @@ delete params.mapView; + let buttonFlags = params.buttons || 0; Sure, that's clearer. ::: src/turnPointBubble.js @@ +40,1 @@ ui.image.icon_name = turnPoint.iconName; Yeah should be this.image. I am not sure about the size, will investigate.
Review of attachment 289256 [details] [review]: ::: src/turnPointBubble.js @@ +40,1 @@ ui.image.icon_name = turnPoint.iconName; I don't think we need to. In this case we want to specify a certain size of the icon (find-location-symbolic) to 48 and in the turnPoint case we want to use the base size 32x32.
Created attachment 289544 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
Created attachment 289545 [details] [review] mapBubble: Add route button
(In reply to comment #73) > (In reply to comment #71) > > > > > There are many and increasingly ways to reach the situation in which Maps shows > > a bubble: current location, searching, "What's here?" button, and in the future > > friend's check-in and opening URIs. > > > > If there is no versatility in the route button, there are many possibilities > > that the user to fail to use the route button (I tried to use the route button > > by myself by setting the From and To using this button and failed), at some > > point she gives up and fallback on search the location directly on the sidebar. > > In my understanding, that's annoying, immediately I would think while staring > > the bubble "Why I need to manually write a place in the sidebar if I already > > found it?". > > > > I don't really see it. I search for something, I wonder how to get there. I > press the route button. And the route-planner opens, with the location in the > To field and the current location in the From field. And I am now in route > planning mode. If I want to switch the places I press the route-reverse button. > For the userlocation it will be added to the From field. > > So the button does something. It is an intent to start route-planning. I don't > see the usecase where I search for something, press route button, then use > something else to get another bubble and use that route button to continue > planning. The sidebar for me is _the_ route planner. Pressing a route button on > a bubble takes me there with information added. > > > You are mis-supposing which are the common or natural usage of this, when > > adding versatility to this doesn't clutter the UI so much (the menu is hidden > > behind a button), is not confusing (moreover, is more predictable from my POV) > > and there are not technical limitations. > > I disagree. I think the sidebar should be the route planner. And the route > buttons on the bubbles are nice ways to get to the route planner with a clear > intent. Also, I experienced with adding the patch that converts the geoclue location to a place with a name of "Current location". I made sure to add it to the From field when pressing the route button. And the effect becomes nice. You get a route calculated from your current location to the bubble you press on. I can maybe do a cast of it tomorrow. We should try to merge in some improvements to current location handling.
Some comments: – Pressing the route button should trigger a route search immediately IMO (if it isn't pressed on the current location bubble). – The current location should be written out as "Current location" with gray text instead of a coordinate. I think this is important. Other than that I think this looks good from a design POV (I'd really like Andreas' view also though). I'll try to look at the code tomorrow. Great work Jonas, can't wait for this to land!
(In reply to comment #79) > Some comments: > – Pressing the route button should trigger a route search immediately IMO (if > it isn't pressed on the current location bubble). That is: if I press the route button on a bubble for Onsala what should happen is: 1) clear the old route if any 2) put Current Location in the From-field and Onsala in the To-field 3) run the route search Hope that makes sense. Btw. I realize that doing this might not be /always/ what you want, but probably most of the time and you'd be able to change the from field easily enough when the sidebar is expanded.
(In reply to comment #80) > (In reply to comment #79) > > Some comments: > > – Pressing the route button should trigger a route search immediately IMO (if > > it isn't pressed on the current location bubble). > > That is: if I press the route button on a bubble for Onsala what should happen > is: > 1) clear the old route if any > 2) put Current Location in the From-field and Onsala in the To-field > 3) run the route search > > Hope that makes sense. Yeah I did it up like this earlier today and I like that approach. But it needs some love from else where. I need to push the changes to make the geoclue location into a place. And name it correctly ("current location, or so"). And I would like to spend some time on how we handle current location in the entries. Howto complete against it? (add it to placestore?) (add it as an action?) Do we get to type current location and press enter? Stuff like that. So either we wait for that, or we commit this now, or we just fix the geoclue location to place and go with that and fix up the other stuff later. Thanks! > > Btw. I realize that doing this might not be /always/ what you want, but > probably most of the time and you'd be able to change the from field easily > enough when the sidebar is expanded.
(In reply to comment #73) > (In reply to comment #71) > > > > > There are many and increasingly ways to reach the situation in which Maps shows > > a bubble: current location, searching, "What's here?" button, and in the future > > friend's check-in and opening URIs. > > > > If there is no versatility in the route button, there are many possibilities > > that the user to fail to use the route button (I tried to use the route button > > by myself by setting the From and To using this button and failed), at some > > point she gives up and fallback on search the location directly on the sidebar. > > In my understanding, that's annoying, immediately I would think while staring > > the bubble "Why I need to manually write a place in the sidebar if I already > > found it?". > > > > I don't really see it. I search for something, I wonder how to get there. I > press the route button. And the route-planner opens, with the location in the > To field and the current location in the From field. And I am now in route > planning mode. If I want to switch the places I press the route-reverse button. > For the userlocation it will be added to the From field. > > So the button does something. It is an intent to start route-planning. I don't > see the usecase where I search for something, press route button, then use > something else to get another bubble and use that route button to continue > planning. The sidebar for me is _the_ route planner. Pressing a route button on > a bubble takes me there with information added. > > > You are mis-supposing which are the common or natural usage of this, when > > adding versatility to this doesn't clutter the UI so much (the menu is hidden > > behind a button), is not confusing (moreover, is more predictable from my POV) > > and there are not technical limitations. > > I disagree. I think the sidebar should be the route planner. And the route > buttons on the bubbles are nice ways to get to the route planner with a clear > intent. - The current location is not always the starting point of the route (I can tell you that from my normal usage of a maps application) - The current location is frequently not accurate enough to be useful in a frequent kind of planning which is, for example, directions to go to different points of the same city or town. - What if I have two clickables or identifiable markers and I want to know the route from one to the other one. - The current state of the search in Maps doesn't allow to search exact street address, so Maps cannot use plan a route for that case. A way to achieve this kind of planning (two or more street address which none is the current user location), is by using the "What is here?" feature from Maps and using the route button. But this doesn't work we the current approach. - Why the reverse button is needed? From what I can see, it doesn't serve to any purpose to the route button since the route button unconditionally clears the route. Aren't we implementing drag and drop for that anyway? OTOH I don't know if the reverse button is correctly positioned, normally it should be in the middle of the height of the From and To entry. To be clear, I'm not criticizing your work, which is great :) . I just don't agree on how it works, it should be more powerful, as a user trying this feature right now, I cannot find it useful at all :(
(In reply to comment #82) > (In reply to comment #73) > > (In reply to comment #71) > > > > > > > > There are many and increasingly ways to reach the situation in which Maps shows > > > a bubble: current location, searching, "What's here?" button, and in the future > > > friend's check-in and opening URIs. > > > > > > If there is no versatility in the route button, there are many possibilities > > > that the user to fail to use the route button (I tried to use the route button > > > by myself by setting the From and To using this button and failed), at some > > > point she gives up and fallback on search the location directly on the sidebar. > > > In my understanding, that's annoying, immediately I would think while staring > > > the bubble "Why I need to manually write a place in the sidebar if I already > > > found it?". > > > > > > > I don't really see it. I search for something, I wonder how to get there. I > > press the route button. And the route-planner opens, with the location in the > > To field and the current location in the From field. And I am now in route > > planning mode. If I want to switch the places I press the route-reverse button. > > For the userlocation it will be added to the From field. > > > > So the button does something. It is an intent to start route-planning. I don't > > see the usecase where I search for something, press route button, then use > > something else to get another bubble and use that route button to continue > > planning. The sidebar for me is _the_ route planner. Pressing a route button on > > a bubble takes me there with information added. > > > > > You are mis-supposing which are the common or natural usage of this, when > > > adding versatility to this doesn't clutter the UI so much (the menu is hidden > > > behind a button), is not confusing (moreover, is more predictable from my POV) > > > and there are not technical limitations. > > > > I disagree. I think the sidebar should be the route planner. And the route > > buttons on the bubbles are nice ways to get to the route planner with a clear > > intent. > > - The current location is not always the starting point of the route (I can > tell you that from my normal usage of a maps application) Not always but typically, it is. > - The current location is frequently not accurate enough to be useful in a > frequent kind of planning which is, for example, directions to go to different > points of the same city or town. How do you mean? We have wifi-geolocation in geoclue now and location accuracy should be at 300m now days. If its not, you gotta start contributing to mozilla location service using mozstumbler app. :) If you check their map, you'll find that they have a lot of the European and US cities covered quite nicely. There is also some developing countries, e.g India that seem to be well covered already. So I don't think this 'frequently not accurate enough to be useful' is correct here.
Anyway, we shouldn't make design design decisions based on limitations in existing technology that can be easily fixed.
(In reply to comment #83) > > - The current location is frequently not accurate enough to be useful in a > > frequent kind of planning which is, for example, directions to go to different > > points of the same city or town. > > How do you mean? We have wifi-geolocation in geoclue now and location accuracy > should be at 300m now days. If its not, you gotta start contributing to mozilla > location service using mozstumbler app. :) > > If you check their map, you'll find that they have a lot of the European and US > cities covered quite nicely. There is also some developing countries, e.g India > that seem to be well covered already. > > So I don't think this 'frequently not accurate enough to be useful' is correct > here. Tell me that nobody uses wired Internet setup and I'll know you are lying :) . IP based geolocation is really inaccurate. OTOH, I don't know how well WiFi based geolocation works in my country since I generally use GPS.
(In reply to comment #85) > (In reply to comment #83) > > > - The current location is frequently not accurate enough to be useful in a > > > frequent kind of planning which is, for example, directions to go to different > > > points of the same city or town. > > > > How do you mean? We have wifi-geolocation in geoclue now and location accuracy > > should be at 300m now days. If its not, you gotta start contributing to mozilla > > location service using mozstumbler app. :) > > > > If you check their map, you'll find that they have a lot of the European and US > > cities covered quite nicely. There is also some developing countries, e.g India > > that seem to be well covered already. > > > > So I don't think this 'frequently not accurate enough to be useful' is correct > > here. > > Tell me that nobody uses wired Internet setup and I'll know you are lying :) No, I'm not saying that at all but what I can certainly say for sure is that most people use laptops and other portable devices more often than desktops. With laptop, you typically have wifi and if you don't turn it off (i doubt most people bother to turn it off), geoclue can make use of it. > OTOH, I don't know how well WiFi based geolocation works in my country since I > generally use GPS. I'm also planning to add support for standalone bluetooth GPS so people can use their phones as GPS device (there is apps that can transform your phone into a bluetooth GPS) so that should help us getting more accurate location for even more people out there. Having said that, desktops usually don't have bluetooth (do they?) so not helping that case much but OTOH desktops should really die in this age anyway. :)
Created attachment 289644 [details] [review] mapBubble: Add route button
Created attachment 289713 [details] [review] Add button template to MapBubble This adds a template ui to MapBubble with an icon, a content area and a button area. The button area contains standard buttons, which bubble sub classes will opt-in to using the buttons parameter: params.buttons = MapBubble.button.ROUTE | ... ; this.parent(params); Bubble sub classes add their own content by: this.image.[property] = ...; this.content.add(...);
So, should we push this? Would like some reviews and ACKS/NACKS on the patches in this, there has been a lot of focus on the route button. Zeeshan/Mattias/Damian could you help me out? Andreas, are you ok with it? Right now it is as your mockup and also sets the From field to current location on bubbles that are not current location. We could push this without the route-reverse button if we want. And wait and see if we want DnD or route-reverse or both.
And if this UX turns out to be horrible broken, we have time to revisit in 3.15 cycle, would like it for 3.15.2 at least.
Attachment 289251 [details] pushed as 2725e2d - Add via entries by adding points to RouteQuery Attachment 289252 [details] pushed as 87c87cd - mainWindow: Reveal sidebar when query is updated Attachment 289253 [details] pushed as 4d9dfd3 - queryPoint: Initialize place property to null Attachment 289254 [details] pushed as 4fab5b1 - routeQuery: notify on point added Attachment 289265 [details] pushed as 0ccbecd - routeQuery: Avoid multiple notifies in reset Attachment 289644 [details] pushed as 88b0a73 - mapBubble: Add route button Attachment 289713 [details] pushed as 572fdf8 - Add button template to MapBubble
Just tested this and it works great. Props!