GNOME Bugzilla – Bug 761327
Add support for adding OSM objects
Last modified: 2016-02-12 07:44:48 UTC
In the branch wip/mlundblad/osm-add-location I have additions for allowing creation of new locations in OSM. It also allows setting "POI type" of existing objects. I intend to soon attach these patches on this bug after possible some cleanup, hopefully making it in time for 3.20.
Created attachment 320144 [details] [review] osmEdit: Add GJS script to extract POI definitions from the iD OSM editor
Created attachment 320145 [details] [review] osmEdit: Add generated POI type data
Created attachment 320146 [details] [review] Introduce an abstract base class for search result popovers
Created attachment 320147 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types.
Created attachment 320148 [details] [review] osmEdit: Add a context menu item for adding locations Launches the OSM edit dialog in add new location mode. Offer to zoom in if the zoom level is not one the two innermost ones.
Created attachment 320149 [details] [review] osmEdit: Add generated POI type data
Review of attachment 320148 [details] [review]: Drive by review, sorry! ::: src/osmEdit.js @@ +64,3 @@ + let response = dialog.run(); + dialog.destroy(); + latitude: latitude, Is there a way to avoid using dialog.run() ? This was the cause of the focus/pan/bug we saw with the export dialog. Would it be really tricky to move to: dialog.connect('response', ... ); dialog.show_all() flow? ::: src/osmUtils.js @@ +26,3 @@ +/* minimum zoom level at which to offer adding a location */ +const MIN_ADD_LOCATION_ZOOM_LEVEL = 16; Does this constant not belong in osmEdit? ::: src/zoomInNotification.js @@ +42,3 @@ + + ui.zoomInButton.connect('clicked', this._onZoomIn.bind(this)); + No need for newlines @@ +47,3 @@ + + _onZoomIn: function() { + /* zoom in to the inner-most level */ The code is pretty self-explanitory I feel, no need for this one.
Review of attachment 320146 [details] [review]: Could the commit message explain more about why this is needed? And while we are doing this, maybe search-popup/searchPopup can become search-popover.ui and searchPopover.js? and propagatingSearchPopover is a terrible name :) can we do better?
Review of attachment 320144 [details] [review]: Who is supposed to run this and why and when?
Review of attachment 320149 [details] [review]: Some more info in commit message would be nice here.
Review of attachment 320147 [details] [review]: ::: src/osmEditDialog.js @@ +153,3 @@ + + if (this._addLocation) { + this._headerBar.title = _("Add Location"); We set this above as well... right? Is the two different blocks needed? ::: src/osmTypeSearchPopover.js @@ +29,3 @@ +const OSMTypeSearchPopover = new Lang.Class({ + Name: 'OSMTypeSearchPopover', + Extends: PropagatingSearchPopover.PropagatingSearchPopover, Do we have anything that does not extend from this proppagatingSearchPopover?
Maybe the abstract class could be called SearchPopover and the current SearchPopup (for the place entry) be called PlaceSearchPopover? Together with OSMTypeSearchPopover.
Created attachment 320224 [details] [review] osmEdit: Add generated POI type data Add the .json POI type defintions extracted by the extractPoiTypesFromId.js script. This file is manually updated when new translations and/or corrections are available from the iD editor.
(In reply to Marcus Lundblad from comment #13) > Created attachment 320224 [details] [review] [review] > osmEdit: Add generated POI type data > > Add the .json POI type defintions extracted by the > extractPoiTypesFromId.js script. > This file is manually updated when new translations and/or corrections > are available from the iD editor. Also set compressed="true" in the GResource specification, this saves ≈100 kB on the generated GResource definition.
Created attachment 320233 [details] [review] osmEdit: Add GJS script to extract POI definitions from the iD OSM editor This creates an updated definition of POI types (with translations) from a checkout of the iD editor's code. This script could be run (and the output copied to data/osm-types.json) if POI presets have been added and/or updated or if new or updated translations are available in a new version of iD.
Created attachment 320237 [details] [review] osmEdit: Add a context menu item for adding locations Launches the OSM edit dialog in add new location mode. Offer to zoom in if the zoom level is not one the two innermost ones. Also port the OSM account and edit/create dialogs to not use dialog.run().
Review of attachment 320233 [details] [review]: I'd like to have something like this above your text in the README: ----8<---8<------8< SCRIPTS ======= extractPoiTypesFromID.js ------------------------ Extracts the POI types that iD editor uses to JSON for us to consume. ----8<---8<------8< Other than that it looks good, except for some style nits. ::: scripts/extractPoiTypesFromID.js @@ +44,3 @@ + let object = JSON.parse(buffer); + let tags = object.tags; + let name = object.name; let [tags, name] = JSON.parse(buffer); @@ +52,3 @@ + title['C'] = name; + + OUTPUT[key + '/' + value] = {'title': title}; OUTPUT[key + '/' + value] = {'title': {'C': name}}; @@ +57,3 @@ + +function processType(type, basePath) { + let dirPath = basePath + '/' + PRESETS_PATH + '/' + type; I generally like this approach more: > let dirPath = [basePath, PRESETS_PATH, type].join('/'); And when repeating the delimiter I think it's much better. @@ +72,3 @@ + continue; + + parseJson(dirPath, file.get_name()); Skip the continue if not needed: if (file.get_name().endsWith('.json')) parseJson(dirPath, file.get_name()); @@ +103,3 @@ + } + } + } Instead of all these nested ifs, I'd do: for (let type in OUTPUT) { let name; try { name = object.presets.presets[type].name; } catch (ex) { continue; } OUTPUT[type].title[lang] = name; } @@ +123,3 @@ + continue; + + processLocale(dirPath, file.get_name()); if (file.get_name().endsWith('.json')) processLocale(dirPath, file.get_name()); (like previously above)
Review of attachment 320224 [details] [review]: LGTM
I'll try to get time to look at the last commit tomorrow, but since I'm ill I can't promise anything. Maybe Jonas will get in before me. :) (Also I've only /looked/ at the code, I haven't tested it yet).
Created attachment 320296 [details] [review] osmEdit: Add GJS script to extract POI definitions from the iD OSM editor This creates an updated definition of POI types (with translations) from a checkout of the iD editor's code. This script could be run (and the output copied to data/osm-types.json) if POI presets have been added and/or updated or if new or updated translations are available in a new version of iD.
(In reply to Mattias Bengtsson from comment #17) > Review of attachment 320233 [details] [review] [review]: Thanks for the review! > > I'd like to have something like this above your text in the README: > > ----8<---8<------8< > > SCRIPTS > ======= > > extractPoiTypesFromID.js > ------------------------ > > Extracts the POI types that iD editor uses to JSON for us to consume. > > ----8<---8<------8< Good idea! Added something like that in the new patch. > > Other than that it looks good, except for some style nits. > > ::: scripts/extractPoiTypesFromID.js > @@ +44,3 @@ > + let object = JSON.parse(buffer); > + let tags = object.tags; > + let name = object.name; > > let [tags, name] = JSON.parse(buffer); I didn't actually get that working… tags and name comes out "undefined" I tried this in a GJS shell: gjs> let [foo, bar] = JSON.parse('{"foo": "bar", "bar": "foo"}'); typein:7: strict warning: reference to undefined property JSON.parse(...)[0] Dunno if I'm missing something… > > @@ +52,3 @@ > + title['C'] = name; > + > + OUTPUT[key + '/' + value] = {'title': title}; > > OUTPUT[key + '/' + value] = {'title': {'C': name}}; Good idea, that's nicer! > > @@ +57,3 @@ > + > +function processType(type, basePath) { > + let dirPath = basePath + '/' + PRESETS_PATH + '/' + type; > > I generally like this approach more: > > > let dirPath = [basePath, PRESETS_PATH, type].join('/'); > > And when repeating the delimiter I think it's much better. Indeed, fixed. > > @@ +72,3 @@ > + continue; > + > + parseJson(dirPath, file.get_name()); > > Skip the continue if not needed: > Sure, makes sense :) > if (file.get_name().endsWith('.json')) > parseJson(dirPath, file.get_name()); > > @@ +103,3 @@ > + } > + } > + } > > Instead of all these nested ifs, I'd do: > > for (let type in OUTPUT) { > let name; > > try { > name = object.presets.presets[type].name; > } catch (ex) { > continue; > } > > OUTPUT[type].title[lang] = name; > } Good that trims it down a bit, too! > > @@ +123,3 @@ > + continue; > + > + processLocale(dirPath, file.get_name()); > > if (file.get_name().endsWith('.json')) > processLocale(dirPath, file.get_name()); > > (like previously above) Sure
Please comment on reviews inside Splinter (by pressing the review button). That makes it easier for me. :)
Review of attachment 320233 [details] [review]: ::: scripts/extractPoiTypesFromID.js @@ +44,3 @@ + let object = JSON.parse(buffer); + let tags = object.tags; + let name = object.name; That was a typo, I mean: {tags, name} = JSON.parse(buffer);
Review of attachment 320296 [details] [review]: Just two small things. ::: scripts/extractPoiTypesFromID.js @@ +35,3 @@ +const LOCALES_PATH = 'dist/locales'; +const PRESET_TYPES = [ 'amenity', 'leisure', 'office', 'place', 'shop', + 'tourism' ]; Style nit: just put a newline after all new list elements instead. :) @@ +44,3 @@ + let object = JSON.parse(buffer); + let tags = object.tags; + let name = object.name; Yeah, so instead: > let {tags, name} = JSON.parse(buffer);
Created attachment 320390 [details] [review] osmEdit: Add GJS script to extract POI definitions from the iD OSM editor This creates an updated definition of POI types (with translations) from a checkout of the iD editor's code. This script could be run (and the output copied to data/osm-types.json) if POI presets have been added and/or updated or if new or updated translations are available in a new version of iD.
Review of attachment 320296 [details] [review]: ::: scripts/extractPoiTypesFromID.js @@ +35,3 @@ +const LOCALES_PATH = 'dist/locales'; +const PRESET_TYPES = [ 'amenity', 'leisure', 'office', 'place', 'shop', + 'tourism' ]; Sure, makes sense @@ +44,3 @@ + let object = JSON.parse(buffer); + let tags = object.tags; + let name = object.name; Sure! :)
Created attachment 320391 [details] [review] Introduce an abstract base class for search result popovers The base class for search popover enables propagating typing events to an associated search entry. This allows the user to continue typing when the search result popover is activated (while still being able to navigate the search results in the popover and selecting a row with enter). This code was earlier part of the place search popover (in the main header bar). Also rename the place search popover from SearchPopup to PlacePopover to be consistent with the search result popover for OSM POI results (in a later patch).
Review of attachment 320146 [details] [review]: Added a more thourough description of the base class. Also changed the name to SearchPopover and renamed the old SearchPopup to PlacePopover.
Review of attachment 320148 [details] [review]: ::: src/osmEdit.js @@ +64,3 @@ + let response = dialog.run(); + dialog.destroy(); + return response; Changed this to connect to the 'response' signal of the dialogs. ::: src/osmUtils.js @@ +26,3 @@ +/* minimum zoom level at which to offer adding a location */ +const MIN_ADD_LOCATION_ZOOM_LEVEL = 16; Good point! Moved this. ::: src/zoomInNotification.js @@ +42,3 @@ + + ui.zoomInButton.connect('clicked', this._onZoomIn.bind(this)); + Fixed. @@ +47,3 @@ + + _onZoomIn: function() { + /* zoom in to the inner-most level */ Removed this.
Created attachment 320394 [details] [review] Introduce an abstract base class for search result popovers The base class for search popover enables propagating typing events to an associated search entry. This allows the user to continue typing when the search result popover is activated (while still being able to navigate the search results in the popover and selecting a row with enter). This code was earlier part of the place search popover (in the main header bar). Also rename the place search popover from SearchPopup to PlacePopover to be consistent with the search result popover for OSM POI results (in a later patch).
Created attachment 320396 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types. Also enables editing the POI type of existing objects in more simple cases (known tag values with no combined tags).
Review of attachment 320147 [details] [review]: ::: src/osmEditDialog.js @@ +153,3 @@ + + if (this._addLocation) { + this._headerBar.title = _("Add Location"); Right, that must have been a left-over from some earlier code churn. I merged the two _addLocation blocks (and got rid of the extranous title setting).
Review of attachment 320237 [details] [review]: Looks neat! ::: data/ui/context-menu.ui @@ +34,3 @@ + <object class="GtkMenuItem" id="addOSMLocationItem"> + <property name="name">add-osm-location-item</property> + <property name="label" translatable="yes">Add Location</property> Is Add Location clear enough? Are We Using This Style Of Capitalizing? Should We? Honest question. Designer Input Needed. Does the hig say? ::: data/ui/zoom-in-notification.ui @@ +10,3 @@ + <property name="can_focus">False</property> + <property name="margin-end">30</property> + <property name="label" translatable="yes">You need to be zoomed-in to edit!</property> Should it be edit? Or Add? ::: src/contextMenu.js @@ +159,3 @@ + }, + + if (response === OSMAccountDialog.Response.SIGNED_IN) What does the "do" add to this name? :) @@ +167,3 @@ + new ZoomInNotification.ZoomInNotification({longitude: this._longitude, + latitude: this._latitude, + return; Add some white space here ({ long ... view }); @@ +180,3 @@ + dialog.destroy(); + if (response === OSMEditDialog.Response.UPLOADED) { + _doAddOSMLocation: function() { This comment is reduntant
Review of attachment 320390 [details] [review]: Looks nice! ::: Makefile.am @@ +1,3 @@ ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS} +SUBDIRS = lib src data po scripts oh new dir! shiny! ::: scripts/Makefile.am @@ +1,2 @@ +noinst_DATA = \ + extractPoiTypesFromID.js\ give this some space, we could even give these \ the ol' aligna at 72 char treatment like other Makefiles
Review of attachment 320394 [details] [review]: Thanks!
Review of attachment 320237 [details] [review]: ::: src/osmEdit.js @@ +57,3 @@ }, + createEditNewDialog: function(parentWindow, latitude, longitude) { This one is used in src/applications.js as well.
Did not manage to test this, I got: cannot register existing type 'WebKitWebView' seen it?
Created attachment 320829 [details] [review] osmEdit: Add GJS script to extract POI definitions from the iD OSM editor This creates an updated definition of POI types (with translations) from a checkout of the iD editor's code. This script could be run (and the output copied to data/osm-types.json) if POI presets have been added and/or updated or if new or updated translations are available in a new version of iD.
Review of attachment 320390 [details] [review]: ::: Makefile.am @@ +1,3 @@ ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS} +SUBDIRS = lib src data po scripts Yeah thanks, I shamefully copied the approach from cantarell :-) ::: scripts/Makefile.am @@ +1,2 @@ +noinst_DATA = \ + extractPoiTypesFromID.js\ Good point!
Created attachment 320830 [details] [review] osmEdit: Add a context menu item for adding locations Launches the OSM edit dialog in add new location mode. Offer to zoom in if the zoom level is not one the two innermost ones. Also port the OSM account and edit/create dialogs to not use dialog.run().
Review of attachment 320237 [details] [review]: ::: src/contextMenu.js @@ +159,3 @@ + }, + + _doAddOSMLocation: function() { Yeah, I dropped the do prefix, might have been a bit redundant :) @@ +167,3 @@ + new ZoomInNotification.ZoomInNotification({longitude: this._longitude, + latitude: this._latitude, + view: this._mapView.view}); Sure @@ +180,3 @@ + dialog.destroy(); + if (response === OSMEditDialog.Response.UPLOADED) { + /* show a notification on success */ Sure
Review of attachment 320237 [details] [review]: ::: src/osmEdit.js @@ +57,3 @@ }, + createEditNewDialog: function(parentWindow, latitude, longitude) { Good catch! Fixed this!
(In reply to Jonas Danielsson from comment #33) > Review of attachment 320237 [details] [review] [review]: > > Looks neat! Thanks! > > ::: data/ui/context-menu.ui > @@ +34,3 @@ > + <object class="GtkMenuItem" id="addOSMLocationItem"> > + <property name="name">add-osm-location-item</property> > + <property name="label" translatable="yes">Add Location</property> > > Is Add Location clear enough? > > Are We Using This Style Of Capitalizing? Should We? Honest question. > > Designer Input Needed. Does the hig say? > > ::: data/ui/zoom-in-notification.ui > @@ +10,3 @@ > + <property name="can_focus">False</property> > + <property name="margin-end">30</property> > + <property name="label" translatable="yes">You need to be zoomed-in > to edit!</property> > > Should it be edit? Or Add? I didn't update any of strings yet, I guess we might want to get some designer opinions. "Zoom in to add" sounds a bit odd to me, dunno. Maybe "… to add a location!" > > ::: src/contextMenu.js > @@ +159,3 @@ > + }, > + > + if (response === OSMAccountDialog.Response.SIGNED_IN) > > What does the "do" add to this name? :) > > @@ +167,3 @@ > + new ZoomInNotification.ZoomInNotification({longitude: > this._longitude, > + latitude: > this._latitude, > + return; > > Add some white space here ({ long ... view }); > > @@ +180,3 @@ > + dialog.destroy(); > + if (response === OSMEditDialog.Response.UPLOADED) { > + _doAddOSMLocation: function() { > > This comment is reduntant
Review of attachment 320396 [details] [review]: Looks nice! ::: src/osmEditDialog.js @@ +122,3 @@ + /* It seems it's not possible to use a custom widget implemented in GJS + from within a widget template */ It should be possible if you do ensure_type? Check application.js. Maybe we should have an array of types there and ensure them in init. const _ensuredTypes = [Webkit2.WebView, OsmTypeSearchEntry.OsmTypeSearchEntry]; ... _ensuredTypes.forEach(function(type) { GObject.ensure_type(type); }); Would this work? @@ +147,3 @@ + + if (this._addLocation) { + this._headerBar.title = _("Add Location"); How about 'Add to OpenStreetMap' to make no doubt on what is going on? @@ +155,3 @@ + Maps.OSMNode.new(0, 0, 0, this._longitude, this._latitude); + /* set a placeholder name tag to always get a name entry for new + locations */ Comment style. @@ +167,3 @@ + + /* store original title of the dialog to be able to restore it when + coming back from type selection */ comment style @@ +224,3 @@ + + /* enable the Next button, so that it's possible to just change the type + of an object without changing anything else */ Comment style in whole file and also maybe read the code again and see if any comment is reduntant ::: src/osmTypePopover.js @@ +33,3 @@ + Signals : { + /* signal emitted when selecting a type, indicates OSM key and value + and display title */ Comment style @@ +38,3 @@ + GObject.TYPE_STRING ] } + }, + InternalChildren: ['list'], Do internalchildren after template ::: src/osmTypeSearchEntry.js @@ +62,3 @@ + /* Note: Not sure if searching already on one character might be a bit + too much, but unsure about languages such as Chinese and Japanese + using ideographs. */ Comment style ::: src/osmTypes.js @@ +45,3 @@ + value for search purposes, given a type mapping value, + also cache the title to avoid re-iterating the language map every time, + and store the lower-case normalized title in the current locale */ Comment style in this file
Review of attachment 320829 [details] [review]: Neat!
Review of attachment 320830 [details] [review]: Neat! ::: data/ui/context-menu.ui @@ +34,3 @@ + <object class="GtkMenuItem" id="addOSMLocationItem"> + <property name="name">add-osm-location-item</property> + <property name="label" translatable="yes">Add Location</property> I think I want "Add to OpenStreetMaps" it is a _context_menu so should be clear from context what we want to add. And we want to make sure people get that we are adding to a global thingie ::: data/ui/zoom-in-notification.ui @@ +10,3 @@ + <property name="can_focus">False</property> + <property name="margin-end">30</property> + <property name="label" translatable="yes">You need to be zoomed-in to edit!</property> I want designer input on this. Maybe this should say "Zoom in to add location! [ Ok ]' instead? ::: po/POTFILES.in @@ +22,3 @@ [type: gettext/glade]data/ui/social-place-more-results-row.ui [type: gettext/glade]data/ui/user-location-bubble.ui +[type: gettext/glade]data/ui/zoom-in-notification.ui magic that you remember! I never do! ::: src/application.js @@ +166,1 @@ }, dialog.connect('response', dialog.destroy.bind(dialog)); ? ::: src/contextMenu.js @@ +162,3 @@ + let osmEdit = Application.osmEdit; + /* if the user is not sufficently zoomed-in, show a notification + offering to zoom in */ Remove or format better ::: src/placeBubble.js @@ +237,3 @@ + }, + + _doEdit: function() { doEdit? or _edit?
Tried it out a bit now and I love the ui and getting the types! Wish we had opening hours support!
Created attachment 320911 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types. Also enables editing the POI type of existing objects in more simple cases (known tag values with no combined tags).
Review of attachment 320396 [details] [review]: ::: src/osmEditDialog.js @@ +122,3 @@ + /* It seems it's not possible to use a custom widget implemented in GJS + from within a widget template */ I tried this, still couldn't get it working, seems to get a segfault somewhere when initing the GJS object when parsing the GtkBuilder template. I still added this construct. Maybe this could be further debugged. Also kept the commented-out definition in the .ui for that widget, so it should be easy to try it again. @@ +147,3 @@ + + if (this._addLocation) { + this._headerBar.title = _("Add Location"); Sure, that looks great! I also changed the title used when in edit mode (defined as the "default" title in the template) to "Edit on OpenStreetMap" @@ +155,3 @@ + Maps.OSMNode.new(0, 0, 0, this._longitude, this._latitude); + /* set a placeholder name tag to always get a name entry for new + locations */ Sure @@ +167,3 @@ + + /* store original title of the dialog to be able to restore it when + coming back from type selection */ Sure @@ +224,3 @@ + + /* enable the Next button, so that it's possible to just change the type + of an object without changing anything else */ Yeah, I removed some I thought are probably redundant. ::: src/osmTypePopover.js @@ +33,3 @@ + Signals : { + /* signal emitted when selecting a type, indicates OSM key and value + and display title */ Sure @@ +38,3 @@ + GObject.TYPE_STRING ] } + }, + InternalChildren: ['list'], Sure ::: src/osmTypeSearchEntry.js @@ +62,3 @@ + /* Note: Not sure if searching already on one character might be a bit + too much, but unsure about languages such as Chinese and Japanese + using ideographs. */ Sure ::: src/osmTypes.js @@ +45,3 @@ + value for search purposes, given a type mapping value, + also cache the title to avoid re-iterating the language map every time, + and store the lower-case normalized title in the current locale */ Sure
Created attachment 320913 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types. Also enables editing the POI type of existing objects in more simple cases (known tag values with no combined tags).
Created attachment 320914 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types. Also enables editing the POI type of existing objects in more simple cases (known tag values with no combined tags).
Created attachment 320915 [details] [review] osmEdit: Add a context menu item for adding locations Launches the OSM edit dialog in add new location mode. Offer to zoom in if the zoom level is not one the two innermost ones. Also port the OSM account and edit/create dialogs to not use dialog.run().
Review of attachment 320830 [details] [review]: ::: data/ui/context-menu.ui @@ +34,3 @@ + <object class="GtkMenuItem" id="addOSMLocationItem"> + <property name="name">add-osm-location-item</property> + <property name="label" translatable="yes">Add Location</property> Sure, that makes sense and clarifies that stuff will be saven on OpenStreetMap ::: data/ui/zoom-in-notification.ui @@ +10,3 @@ + <property name="can_focus">False</property> + <property name="margin-end">30</property> + <property name="label" translatable="yes">You need to be zoomed-in to edit!</property> Yeah. Changed it in the mean time (also did a rename of the notification button to "okButton" to stay consistent. ::: po/POTFILES.in @@ +22,3 @@ [type: gettext/glade]data/ui/social-place-more-results-row.ui [type: gettext/glade]data/ui/user-location-bubble.ui +[type: gettext/glade]data/ui/zoom-in-notification.ui Thanks! :-) ::: src/application.js @@ +166,1 @@ }, Sure! :) ::: src/contextMenu.js @@ +162,3 @@ + let osmEdit = Application.osmEdit; + /* if the user is not sufficently zoomed-in, show a notification + offering to zoom in */ Yeah, I just removed this, the code is probably pretty self-explanatory. ::: src/placeBubble.js @@ +237,3 @@ + }, + + _doEdit: function() { _edit I think (changed to that scheme elsewhere too) ::: src/zoomInNotification.js @@ +30,3 @@ + Name: 'ZoomInNotification', + Extends: Notification.Notification, + InternalChildren: ['zoomInButton'], I also removed this, since this is not (yet) a widget template…
(In reply to Jonas Danielsson from comment #47) > Tried it out a bit now and I love the ui and getting the types! Thanks! Yeah, it feels pretty fluid to search for types this way! > > Wish we had opening hours support! Yeah, that would be awesome! I had planned to add a "prior arts" wiki page with some screenshots from the Josm opening hours plugin to use as some starting point for thinking about some design for that, it's quite tricky, though and would probably handle a small subset. For some reason though, I don't seem to be able to edit wiki pages anymore (it used to work a while back), will probably need to check with the sysadmin team.
Created attachment 320932 [details] [review] osmEdit: Add support for creating new locations in the edit dialog Adds the ability to edit newly created locations in the dialog. Also adds a module to handle OSM tag key/value mapping to translated POI types and a list of recently used POI types. Also enables editing the POI type of existing objects in more simple cases (known tag values with no combined tags).
Quick comment, after applying: $ git diff --check HEAD~2 src/osmTypeListRow.js:52: new blank line at EOF. src/osmTypePopover.js:68: trailing whitespace. + src/osmTypePopover.js:68: new blank line at EOF. src/osmTypes.js:169: new blank line at EOF. src/zoomInNotification.js:54: new blank line at EOF.
Review of attachment 320915 [details] [review]: Looks fine! Commit after fixing whitespace damages!
Review of attachment 320932 [details] [review]: lgtm! Awesome work Marcus! Fixup white space damage and push! ::: data/ui/osm-edit-dialog.ui @@ +278,3 @@ <property name="can-focus">False</property> <property name="show-close-button">False</property> + <property name="title" translatable="yes">Edit on OpenStreetMap</property> Yes!
Attachment 320224 [details] pushed as 92dec43 - osmEdit: Add generated POI type data Attachment 320394 [details] pushed as 00aad3d - Introduce an abstract base class for search result popovers Attachment 320829 [details] pushed as 2d9988e - osmEdit: Add GJS script to extract POI definitions from the iD OSM editor Attachment 320915 [details] pushed as 51bb8e4 - osmEdit: Add a context menu item for adding locations Attachment 320932 [details] pushed as c4c25a8 - osmEdit: Add support for creating new locations in the edit dialog
(In reply to Jonas Danielsson from comment #58) > Review of attachment 320932 [details] [review] [review]: > > lgtm! > > Awesome work Marcus! Thanks! Yeah, it's turning out quite nice :) > Fixup white space damage and push! > > ::: data/ui/osm-edit-dialog.ui > @@ +278,3 @@ > <property name="can-focus">False</property> > <property name="show-close-button">False</property> > + <property name="title" translatable="yes">Edit on > OpenStreetMap</property> > > Yes!