GNOME Bugzilla – Bug 761636
Add support for editing phone on OSM objects
Last modified: 2016-02-11 21:51:55 UTC
Since phone number is one of the additional fields of contact information we plan on showing with the new place popover design (which can show an expanded area with extra details) it makes sense to allow editing this field for OSM objects.
Created attachment 320541 [details] [review] osmEdit: Add support for editing phone number Adds the ability to edit phone numbers. Also reformat URIs using the tel: scheme by stripping off the leading tel: and trailing parameters (following a ”;”), which could be useful when copying links i.e. from a browser.
Review of attachment 320541 [details] [review]: Thanks! Same question here, could we add it to bubble as well?
(In reply to Jonas Danielsson from comment #2) > Review of attachment 320541 [details] [review] [review]: > > Thanks! > > Same question here, could we add it to bubble as well? Absolutely, perhaps with a clickable tel: URI. I dunno if we actually support that URI scheme… When I try to run: xdg-open tel:+4670<my mobile number> I get an error that the location is not supported.
(In reply to Marcus Lundblad from comment #3) > (In reply to Jonas Danielsson from comment #2) > > Review of attachment 320541 [details] [review] [review] [review]: > > > > Thanks! > > > > Same question here, could we add it to bubble as well? > > Absolutely, perhaps with a clickable tel: URI. I dunno if we actually > support that URI scheme… When I try to run: > xdg-open tel:+4670<my mobile number> > I get an error that the location is not supported. Maybe you can look at my commit in polari that checks if we support the URI and in that case URIfy it? https://git.gnome.org/browse/polari/commit/?id=651c71fed5413dc5a06712277fba97e54cfc3552
Created attachment 320759 [details] [review] osmEdit: Add support for editing phone number Adds the ability to edit phone numbers. Also reformat URIs using the tel: scheme by stripping off the leading tel: and trailing parameters (following a ”;”), which could be useful when copying links i.e. from a browser. Also add phone number to the information shown in the expanded area in the place bubble.
Review of attachment 320759 [details] [review]: Thanks! Maybe "Add support for phone in map bubbles" ? ::: src/osmEditDialog.js @@ +64,3 @@ + strip off the leading tel: protocol string and trailing parameters, + following a ; + otherwise return the string unmodified */ /* * For multi line comments I prefer * this style of comments */ @@ +65,3 @@ + following a ; + otherwise return the string unmodified */ +let _osmPhoneRewriteFunc = function(text) { Why this way? And not _osmPhoneRewriteFunc: function(text) { } ? @@ +66,3 @@ + otherwise return the string unmodified */ +let _osmPhoneRewriteFunc = function(text) { + let afterTel = text.startsWith('tel:') ? text.replace('tel:', '') : text; Can we use https://developer.gnome.org/glib/stable/glib-URI-Functions.html ? ::: src/placeBubble.js @@ +148,3 @@ + let schemes = Utils.getURISchemes(); + + if (schemes.indexOf('tel') === -1) { Maybe we can instead have: if (Utils.uriSupported('tel:') { ? I do not know if we will have a need of the full list anywhere. @@ +154,3 @@ + expandedContent.push({ label: _("Phone"), + linkText: place.phone, + linkUrl: 'tel:' + place.phone }); I think I might prefer 'tel:%s'.format(place.phone) ::: src/utils.js @@ +365,3 @@ } + +function getURISchemes() { uriSupported(scheme)
Created attachment 320828 [details] [review] osmEdit: Add support for editing phone number Adds the ability to edit phone numbers. Also reformat URIs using the tel: scheme by stripping off the leading tel: and trailing parameters (following a ”;”), which could be useful when copying links i.e. from a browser. Also add phone number to the information shown in the expanded area in the place bubble.
Review of attachment 320759 [details] [review]: ::: src/osmEditDialog.js @@ +64,3 @@ + strip off the leading tel: protocol string and trailing parameters, + following a ; + otherwise return the string unmodified */ Sure, that looks nicer! @@ +65,3 @@ + following a ; + otherwise return the string unmodified */ +let _osmPhoneRewriteFunc = function(text) { Actually, these functions where defined outside of the OSMEditDialog class (as well as the edit field map definitions). Maybe they should be moved in there? @@ +66,3 @@ + otherwise return the string unmodified */ +let _osmPhoneRewriteFunc = function(text) { + let afterTel = text.startsWith('tel:') ? text.replace('tel:', '') : text; There seems to be a _parse_scheme function, unfortunatly not one to extract the "middle part". Though, I rewrote the code to check the result from that function instead of doing indexOf === -1. I guess it's a bit more clear anyway… ::: src/placeBubble.js @@ +148,3 @@ + let schemes = Utils.getURISchemes(); + + if (schemes.indexOf('tel') === -1) { Sure @@ +154,3 @@ + expandedContent.push({ label: _("Phone"), + linkText: place.phone, + linkUrl: 'tel:' + place.phone }); Sure ::: src/utils.js @@ +365,3 @@ } + +function getURISchemes() { Sure
(In reply to Jonas Danielsson from comment #6) > Review of attachment 320759 [details] [review] [review]: > > Thanks! > > Maybe "Add support for phone in map bubbles" ? I think the commit message mentioned adding phone to the info in the place bubble. Or did I misunderstand? > > ::: src/osmEditDialog.js > @@ +64,3 @@ > + strip off the leading tel: protocol string and trailing parameters, > + following a ; > + otherwise return the string unmodified */ > > /* > * For multi line comments I prefer > * this style of comments > */ > > @@ +65,3 @@ > + following a ; > + otherwise return the string unmodified */ > +let _osmPhoneRewriteFunc = function(text) { > > Why this way? > > And not _osmPhoneRewriteFunc: function(text) { > > } > > ? > > @@ +66,3 @@ > + otherwise return the string unmodified */ > +let _osmPhoneRewriteFunc = function(text) { > + let afterTel = text.startsWith('tel:') ? text.replace('tel:', '') : > text; > > Can we use https://developer.gnome.org/glib/stable/glib-URI-Functions.html ? > > ::: src/placeBubble.js > @@ +148,3 @@ > + let schemes = Utils.getURISchemes(); > + > + if (schemes.indexOf('tel') === -1) { > > Maybe we can instead have: if (Utils.uriSupported('tel:') { ? > I do not know if we will have a need of the full list anywhere. > > @@ +154,3 @@ > + expandedContent.push({ label: _("Phone"), > + linkText: place.phone, > + linkUrl: 'tel:' + place.phone }); > > I think I might prefer 'tel:%s'.format(place.phone) > > ::: src/utils.js > @@ +365,3 @@ > } > + > +function getURISchemes() { > > uriSupported(scheme)
I applied this patch to commit 9cb01e6f49 and for a location with a known phone number I did not see the phone number in the place bubble. However the phone number is editable in the OSM edit dialog. Before I saw this patch I added virtually identical code to simply display the phone number in the place bubble and had the same issue, so it's at least good to know someone more familiar with the project wrote exactly what I did.
To see it somone would have to tag a place with phone in OpenStreetMap. It may be rare. You can use this query to find some:http://overpass-turbo.eu/?template=key&key=phone
Created attachment 320850 [details] Screehshot of phone number Marcus: What is up with the weird padding around website?
(In reply to Gatlin Johnson from comment #10) > I applied this patch to commit 9cb01e6f49 and for a location with a known > phone number I did not see the phone number in the place bubble. However the > phone number is editable in the OSM edit dialog. > > Before I saw this patch I added virtually identical code to simply display > the phone number in the place bubble and had the same issue, so it's at > least good to know someone more familiar with the project wrote exactly what > I did. Hi Gatlin! Thanks for trying this out! :-) Since I guess you had already searched for this place, it was probably already stored in the local place store. In this case, if that search was done before applying the patch, it would have been saved in the place store without the phone attribute. You could try removing the place store file (~/.local/share/maps-places.json) if you don't have something in there that you would like to keep (such as searched routes). Or if you add phone to an existing object, it should update the data also if it was already stored.
(In reply to Jonas Danielsson from comment #12) > Created attachment 320850 [details] > Screehshot of phone number > > Marcus: What is up with the weird padding around website? Yeah, that looks weird. I suppose it might have changed when I made it use a GtkLinkButton for the link case. What's strange though, is that it still sets .halign = Gtk.Align.START (the same as when using a GtkLabel). Maybe it needs .hadjust for some reason. I'll try and experiment with the inspector tonight.
Review of attachment 320828 [details] [review]: Thanks! ::: src/osmEditDialog.js @@ +68,3 @@ + let scheme = GLib.uri_parse_scheme(text); + + if (scheme === 'tel') { scheme is not used again right? Maybe just have this as if (GLib.uri_parse_scheme() === 'tel') { ... } ? @@ +72,3 @@ + let beforeParams = afterTel.split(';')[0]; + + return beforeParams; return afterTel.split(';')[0]; ? @@ +74,3 @@ + return beforeParams; + } else + return text; Nit I prefer brackets when "main if" has brackets: else { } Up to you tho ::: src/placeBubble.js @@ +151,3 @@ + linkUrl: 'tel:%s'.format(place.phone) }); + } else { + expandedContent.push({ label: _("Phone"), Both these should be Phone: ::: src/utils.js @@ +380,3 @@ + }); + }); + return schemes.indexOf(scheme) != -1; I guess a small optimization would be to use: for (let i in apps) { ... } And return true if (type.replace(prefix, '') === scheme) And false if we finish the loop? Then the schemes array is not needed?
(In reply to Marcus Lundblad from comment #13) > Hi Gatlin! > Thanks for trying this out! :-) > Since I guess you had already searched for this place, it was probably > already stored in the local place store. > In this case, if that search was done before applying the patch, it would > have been saved in the place store without the phone attribute. > You could try removing the place store file > (~/.local/share/maps-places.json) if you don't have something in there that > you would like to keep (such as searched routes). > Or if you add phone to an existing object, it should update the data also if > it was already stored. Thank you so much, that was exactly it. And now I know that much more about this application works.
(In reply to Marcus Lundblad from comment #13) > Or if you add phone to an existing object, it should update the data also if > it was already stored. The place has the phone tag (verified by visiting openstreetmap.org) and was already in my list of searched places before applying the patch (because it's a pizza shop I like and that's an important saved place). However, applying the patch and then searching for it again didn't update the store. Just to clarify: should it have? Because maybe that's something I can look into? I speak only for myself but if more data are added to places after I visit them the first time I would like for my local data to be updated without deleting the places store.
(In reply to Gatlin Johnson from comment #17) > (In reply to Marcus Lundblad from comment #13) > > > Or if you add phone to an existing object, it should update the data also if > > it was already stored. > > The place has the phone tag (verified by visiting openstreetmap.org) and was > already in my list of searched places before applying the patch (because > it's a pizza shop I like and that's an important saved place). However, > applying the patch and then searching for it again didn't update the store. > Just to clarify: should it have? Because maybe that's something I can look > into? I speak only for myself but if more data are added to places after I > visit them the first time I would like for my local data to be updated > without deleting the places store. Hi! We check if the object already exists in our store, if it does we do not call out to overpass again. It is to avoid the loading time really. Overpass is slow. But, we also check of a place is "stale", in placeBubble.js: https://git.gnome.org/browse/gnome-maps/tree/src/placeBubble.js#n77 The function it calls is in placeStore.js: https://git.gnome.org/browse/gnome-maps/tree/src/placeStore.js#n286 And the current threshold is 7 days: https://git.gnome.org/browse/gnome-maps/tree/src/placeStore.js#n35
(In reply to Jonas Danielsson from comment #18) > > Hi! > > We check if the object already exists in our store, if it does we do not > call out to overpass again. It is to avoid the loading time really. Overpass > is slow. > > But, we also check of a place is "stale", in placeBubble.js: > https://git.gnome.org/browse/gnome-maps/tree/src/placeBubble.js#n77 > > The function it calls is in placeStore.js: > https://git.gnome.org/browse/gnome-maps/tree/src/placeStore.js#n286 > > And the current threshold is 7 days: > https://git.gnome.org/browse/gnome-maps/tree/src/placeStore.js#n35 This is an alarmingly friendly project. I hope I have not been asking questions already documented; if not maybe my ignorance can benefit other newcomers. Thank you!
Review of attachment 320828 [details] [review]: ::: src/placeBubble.js @@ +155,3 @@ + } + } + Also I think I want wiki and website together and nothing between them, make sense?
Created attachment 320908 [details] [review] osmEdit: Add support for editing phone number Adds the ability to edit phone numbers. Also reformat URIs using the tel: scheme by stripping off the leading tel: and trailing parameters (following a ”;”), which could be useful when copying links i.e. from a browser. Also add phone number to the information shown in the expanded area in the place bubble.
Review of attachment 320908 [details] [review]: Lgtm!
Review of attachment 320828 [details] [review]: ::: src/osmEditDialog.js @@ +68,3 @@ + let scheme = GLib.uri_parse_scheme(text); + + if (scheme === 'tel') { Sure @@ +72,3 @@ + let beforeParams = afterTel.split(';')[0]; + + return beforeParams; Sure, makes it more compact and nice :) @@ +74,3 @@ + return beforeParams; + } else + return text; Yeah, I think I agree :) ::: src/placeBubble.js @@ +151,3 @@ + linkUrl: 'tel:%s'.format(place.phone) }); + } else { + expandedContent.push({ label: _("Phone"), Sure @@ +155,3 @@ + } + } + Yeah, that makes sense, probably looks a bit neater. ::: src/utils.js @@ +380,3 @@ + }); + }); + return schemes.indexOf(scheme) != -1; Yeah. I rewrote it into a nested loop over the apps and schemes and return early on match and fallback return of false at the end.
(In reply to Marcus Lundblad from comment #23) > Review of attachment 320828 [details] [review] [review]: > > ::: src/osmEditDialog.js > @@ +68,3 @@ > + let scheme = GLib.uri_parse_scheme(text); > + > + if (scheme === 'tel') { > > Sure > > @@ +72,3 @@ > + let beforeParams = afterTel.split(';')[0]; > + > + return beforeParams; > > Sure, makes it more compact and nice :) > > @@ +74,3 @@ > + return beforeParams; > + } else > + return text; > > Yeah, I think I agree :) > > ::: src/placeBubble.js > @@ +151,3 @@ > + linkUrl: > 'tel:%s'.format(place.phone) }); > + } else { > + expandedContent.push({ label: _("Phone"), > > Sure > > @@ +155,3 @@ > + } > + } > + > > Yeah, that makes sense, probably looks a bit neater. > > ::: src/utils.js > @@ +380,3 @@ > + }); > + }); > + return schemes.indexOf(scheme) != -1; > > Yeah. > I rewrote it into a nested loop over the apps and schemes and return early > on match and fallback return of false at the end. I also dropped the tooltip property when adding the extended content for website. It still gets a tooltip, btw, and it should work out nice with that proposed patch to go back to using a GtkLabel with an <a/> with a title attribute.
Created attachment 320909 [details] [review] osmEdit: Add support for editing phone number Adds the ability to edit phone numbers. Also reformat URIs using the tel: scheme by stripping off the leading tel: and trailing parameters (following a ”;”), which could be useful when copying links i.e. from a browser. Also add phone number to the information shown in the expanded area in the place bubble.
Attachment 320909 [details] pushed as a044440 - osmEdit: Add support for editing phone number