GNOME Bugzilla – Bug 762591
Add support for editing address of OSM objects
Last modified: 2016-02-26 12:14:46 UTC
In the branch "wip/jonasdn/osm-edit-address" there is currently a patch to allow adding/editing street addresses. It currently supports editing street, house number, postal code, and city (the place name usually following the postal code in addresses). It introduces a few new translatable strings, so a string announcement will be nessesary.
Created attachment 322221 [details] [review] Adds the ability to add and edit OSM address tags
Review of attachment 322221 [details] [review]: ::: data/ui/osm-edit-address.ui @@ +6,3 @@ + <property name="can_focus">False</property> + <style> + <class name="linked"/> I thought linked was only to be used with GtkBox? https://wiki.gnome.org/HowDoI/Buttons ::: src/osmEditDialog.js @@ +107,3 @@ + name: _("Address"), + tag: 'addr', + subtags: ['addr:street', 'addr:housenumber', 'addr:postcode', 'addr:city'], I guess we want to document subtags above? @@ +110,3 @@ + type: EditFieldType.ADDRESS + }, + remove this. @@ +523,3 @@ }, + _addOSMEditDeleteButton: function(tag, rowsToDelete) { Maybe this should take fieldSpec instead of tag, then the logic below would look more like in other places... if (fieldSpec.subtags) .... @@ +545,3 @@ + /* if we should delete the following row (for a widget spanning + two rows) delete the next row (which now has moved up one + row) */ /* * if we * * */
Created attachment 322232 [details] [review] osmEditDialog: Add ability to add and edit addresses Supports editing street, house number, postal code, and city.
Review of attachment 322221 [details] [review]: ::: data/ui/osm-edit-address.ui @@ +6,3 @@ + <property name="can_focus">False</property> + <style> + <class name="linked"/> Seems to work, but maybe it's different with GtkEntries (as children)… ::: src/osmEditDialog.js @@ +107,3 @@ + name: _("Address"), + tag: 'addr', + subtags: ['addr:street', 'addr:housenumber', 'addr:postcode', 'addr:city'], Sure @@ +110,3 @@ + type: EditFieldType.ADDRESS + }, + Sure @@ +523,3 @@ }, + _addOSMEditDeleteButton: function(tag, rowsToDelete) { Sounds reasonable @@ +545,3 @@ + /* if we should delete the following row (for a widget spanning + two rows) delete the next row (which now has moved up one + row) */ Sure
Review of attachment 322232 [details] [review]: Looks good otherwise, I think. ::: src/osmEditDialog.js @@ +97,3 @@ * (for TEXT and INTEGER fields) + * subtags: Used by a complex composite OSM tag. + * rows: Number of rows neded if != 1. Maybe we should clarify that this (currently) applies to type "ADDRESS"? @@ +553,3 @@ + this._editorGrid.remove_row(row); + this._currentRow--; + } Maybe we should generalize this and remove all extra rows, should be possible to do something like: for (let i = 0; i < rows; i++) { ... } here, and remove the "this._editorGrid.remove_row(row)" and "this._currentRow--" statements outside of the for-loop Should be equivalent, AFICT Also, this should cover use in case we would some day want 3-row-spanning address (or other) composites. @@ +684,3 @@ + this._editorGrid.attach(addr, 1, this._currentRow, 1, 2); + this._addOSMEditDeleteButton(fieldSpec); + this._currentRow += 2; Maybe this could be rewritten like let rows = fieldSpec.rows || 1; ... attach(..., 1, rows); ... this._currentRow += rows In the same spirit as the suggestion above.
Comment on attachment 322232 [details] [review] osmEditDialog: Add ability to add and edit addresses Two things I saw in this and wonder about in this patch: >* rows: Number of rows neded if != 1. "neded" looks like it should be "needed". > if (fieldSpec.subtstags) What is subtstags? Inside that condition is fieldSpec.subtags.forEach((function(key), so I start to wonder if the extra "ts" is a typo and it should be subtags like in the other lines?
(In reply to Anders Jonsson from comment #6) > Comment on attachment 322232 [details] [review] [review] > osmEditDialog: Add ability to add and edit addresses > > > Two things I saw in this and wonder about in this patch: > > >* rows: Number of rows neded if != 1. > > "neded" looks like it should be "needed". > > > > if (fieldSpec.subtstags) > > What is subtstags? Inside that condition is > fieldSpec.subtags.forEach((function(key), so I start to wonder if the extra > "ts" is a typo and it should be subtags like in the other lines? You are right! Thanks for this!
Thanks Anders! You really have a pair of sharp eyes! :-) That typo would mean that clicking the delete button for an address entry would have removed the input entries, but silently kept the OSM tags' values.
Created attachment 322343 [details] [review] osmEditDialog: Add ability to add and edit addresses Supports editing street, house number, postal code, and city.
Review of attachment 322343 [details] [review]: LGTM
Review of attachment 322343 [details] [review]: We need exception for this
Created attachment 322406 [details] Cast of edit address
Comment on attachment 322343 [details] [review] osmEditDialog: Add ability to add and edit addresses Nice, just one more typo (trivial and harmless this time): >* rows: Number of rows needed if != 1 (Curently only for ADDRESS) Should be spelled "Currently".
Review of attachment 322343 [details] [review]: Pushed after freeze break approval