GNOME Bugzilla – Bug 726628
Edit openstreetmap through Maps
Last modified: 2016-01-30 12:20:23 UTC
One thing that makes Maps stand out is the use of OpenStreetMap which is a crowd sourced wiki style approach to mapping. It would be cool if one could use Maps to edit and add things to OpenStreetMap. I thought maybe we could use this bug to discuss whether we want to do this. And if we do, how should we expose it to users? Would it be possible to only show editing controls if we can find out the user has an OpenStreetMap account? Should we always have editing controls but in a way that is not in your face? How about the context menu, could that have an "Edit this view!" item?
Can't say anything about design but I really like this idea. This would also make us the first OSM editor that works out of the box w/o requiring java. :) All editors I've tried so far are either mainly targetted for editing small parts of the map (some of them even don't all you any editing until you zoom enough) or they are very difficult to use. So if we can provide an easy and intuitive way to edit places, that would help us a lot.
One thing to decide that will have a big impact on the level of work needed for this is if it's 'good enough' to send the user to OpenStreetMaps Id editor when we want to edit. Or if we want to implement the OSM API and do it natively. And in that case should that be a library (is there any already?) or be implemented in Maps?
(In reply to comment #2) > One thing to decide that will have a big impact on the level of work needed for > this is if it's 'good enough' to send the user to OpenStreetMaps Id editor when > we want to edit. We can start with that I guess. > Or if we want to implement the OSM API and do it natively. Yeah, I'd really like to provide editing in Maps itself for the reasons I mentioned in comment#1. > And > in that case should that be a library (is there any already?) or be implemented > in Maps? Yeah, I think a library would be better. The less the code in Maps, the better; I've always said. :) Don't think such a library exists. Maybe this belongs in champlain?
My thought on this is that we have an edit-link (that looks and behaves like an <a href=""> in the browser next to the copyright notice. Something like how this would look in html: <a href="https://www.openstreetmap.org/edit#map=10/57.6960/11.9521>Edit this</a> | (c) <a href="http://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors Look in the right corner at the map at http://www.leafletjs.com, and switch out the Leaflet-link for the "Edit this"-link above. The Edit link would go to the osm.org/edit with the center coordinate set and the zoom level to whatever zoomlevel currently is set in Maps (the example above is for Gothenburg at Z = 10). What do you think?
I think the link could be always shown btw and just open the link in the default browser.
(In reply to comment #3) > > > Or if we want to implement the OSM API and do it natively. > > Yeah, I'd really like to provide editing in Maps itself for the reasons I > mentioned in comment#1. Yeah I agree with this as well. > > > And > > in that case should that be a library (is there any already?) or be implemented > > in Maps? > > Yeah, I think a library would be better. The less the code in Maps, the better; > I've always said. :) Don't think such a library exists. Maybe this belongs in > champlain? Agreed. I am not sure it belongs in Champlain though. Champlain being a Map widget I think it would make more sense that Champlain used the library to maybe tie it in with the map-rendering. Possibly. Maybe geocode is a better fit? It already deals with OSM API in some ways. Or maybe it should be a new small library that geocode/champlain/maps could use.
(In reply to comment #4) > My thought on this is that we have an edit-link (that looks and behaves like an > <a href=""> in the browser next to the copyright notice. > > Something like how this would look in html: > > <a href="https://www.openstreetmap.org/edit#map=10/57.6960/11.9521>Edit > this</a> | (c) <a > href="http://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors > > Look in the right corner at the map at http://www.leafletjs.com, and switch out > the Leaflet-link for the "Edit this"-link above. > > The Edit link would go to the osm.org/edit with the center coordinate set and > the zoom level to whatever zoomlevel currently is set in Maps (the example > above is for Gothenburg at Z = 10). > > What do you think? I think it maybe could make sense as a start. But I like the idea of native editing in Maps. Especially if could be implemented to not be a distraction for those not interesting in collaborative editing. I'm not sure I want to do it Clutter though. Is there a nice GTK+ widget hat could be overlaid to achieve this?
(In reply to comment #6) > (In reply to comment #3) > > > > > > Or if we want to implement the OSM API and do it natively. > > > > Yeah, I'd really like to provide editing in Maps itself for the reasons I > > mentioned in comment#1. > > Yeah I agree with this as well. > > > > > > And > > > in that case should that be a library (is there any already?) or be implemented > > > in Maps? > > > > Yeah, I think a library would be better. The less the code in Maps, the better; > > I've always said. :) Don't think such a library exists. Maybe this belongs in > > champlain? > > Agreed. I am not sure it belongs in Champlain though. Champlain being a Map > widget I think it would make more sense that Champlain used the library to > maybe tie it in with the map-rendering. Possibly. > > Maybe geocode is a better fit? It already deals with OSM API in some ways. Or > maybe it should be a new small library that geocode/champlain/maps could use. geocode isn't any better fit than champlain, its supposed to be a library for geocoding/decoding only.
What kind of osm editing do you have in mind here btw. What should the user be able to edit, and how would she/he start the editing process?
(In reply to comment #9) > What kind of osm editing do you have in mind here btw. What should the user be > able to edit, and how would she/he start the editing process? I think we can take inspiration from existing OSM editors for this. E.g We could implement overlays and views similar to that on ID editor. There could be a button somewhere (maybe a bit hidden) that you hit to enable editing view. In case this idea doesn't fly, maybe we can just provide a way to open the current view in ID editor (in browser).
(In reply to comment #9) > What kind of osm editing do you have in mind here btw. What should the user be > able to edit, and how would she/he start the editing process? It would have to be design driven. And be implemented in a way that does not steal focus away from the basic functionality of the app. Maybe start small and look into allowing editing of already existing map features then the interface doesn't need to be intrusive.
So my thinking: Be able to edit/add information that we currently display in our map bubbles. The information we get from geocode/overpass apis. Maybe this needs us to add a OpenStreetMap account to GNOME online accounts? To get the authentication? And if we detect that in maps, we add an Edit button to the map bubble somewhere. How about that? And we would need to spread the word that people should help out bettering open map data by getting an account.
I started playing with implementing an OSM edit API using GJS here: https://github.com/mlundblad/gnome-maps-osm-edit-poc Currently the repo only contains a stub launcher script. I started outlining some basic classes mapping the OSM objects (tag, node, et.c.) and will add things incrementally as I go...
(In reply to comment #13) > I started playing with implementing an OSM edit API using GJS here: > https://github.com/mlundblad/gnome-maps-osm-edit-poc > > Currently the repo only contains a stub launcher script. I started outlining > some basic classes mapping the OSM objects (tag, node, et.c.) and will add > things incrementally as I go... You could also request a GNOME git account and then transition to a wip/edit-map-bubbles branch on git.gnome.org if you'd like.
I now have a branch working with rudimentary editing. Currently only the name field is editable. For authentication you need to set the environment variables OSM_USERNAME and OSM_PASSWORD to a corresponding user in the OSM development DB (available at http://api06.dev.openstreetmap.org). Also, since the object IDs in the development DB is not mapped to the regular DB and we get objects from there when searching, I added code to override the object actually edited when clicking "Edit" to a mock object in the development DB. This is controlled by the environment variables OSM_MOCK_TYPE and OSM_MOCK_ID (to set the object type and ID of a pre-created mock object). I have currently added a node that can used for testing by setting: OSM_MOCK_TYPE=node OSM_MOCK_ID=4298258663 There is currently no UI feedback on success or errors (and currently the editing dialog is not dismissed automatically), but there is on the other hand fairly plenty debug printouts... :-) Theoretically it should be possible to use the live API by changing the USE_TEST_API const in src/osmConnection.js to false (and then the mock overriding should not be performed), but I have not tested this and it is probably a bit to early to do run live just yet... The code is available in the wip/osm-edit branch in this repo: git@github.com:mlundblad/gnome-maps.git
Created attachment 301090 [details] [review] osmShim: Add OSM XML to Json shim library using libxml2.
I thought it might be time to get some of the code up for some first shot reviewing. So the first patch is the C shim library used for producing the Json output for the JS OSM API used by the OSM editing functionallity, given the raw OSM XML data.
Review of attachment 301090 [details] [review]: Thanks! Awesome that you have started this. The code looks nice. I have a few style comments below. Also, wouldn't it be nicer if this was a proper GObject? And it returned MapsOsmWay-, MapsOsmRelation- and MapsOsmNode objects for each of these functions? So we do not first parse xml to json and then json to javascript? Or will that be to large of a burden? And maybe we could get a bit in the commit log that explains how this is supposed to be used and why it is needed? Thanks again! ::: lib/Makefile.am @@ +6,3 @@ +libgnome_maps_headers_private = maps-contact-store.h maps-contact.h maps.h maps-osm.h +libgnome_maps_sources = maps-contact-store.c maps-contact.c maps-osm.c Not your fault initially I guess, but maybe we could list them same as the other stuff, with separators at col 72? To have a uniform feel in this file? ::: lib/maps-osm.c @@ +14,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA We do not need to keep track of the post address of the FSF, this should read: * You should have received a copy of the GNU General Public License along * with GNOME Maps; if not, see <http://www.gnu.org/licenses/> Same in maps-osm.h @@ +29,3 @@ +} + +void maps_osm_finalize (void) The style in the other lib c files is: void maps_osm_init (void) { ... } Same for all functions. @@ +34,3 @@ +} + +static xmlDocPtr _read_xml_doc (const gchar *content, guint length) No need for the _ prefix the static keyword is enough to denote the functions as private to this file. @@ +51,3 @@ +{ + const xmlAttr *cur_attr; + gchar *key; We use 'char *' in the maps-contact-* files, so perhaps we should here as well.
Created attachment 301161 [details] [review] osmShim: Add OSM XML to Json shim library using libxml2. Shim library routines used to parse OSM XML objects into Json representation usable by JS. Subsequent patches will change these to return proper GObjects.
I have now pushed the current state of the implementation to the wip/osm-edit branch in git.gnome.org. As before, you need to set a few environment variables for running this. By default, it will use the OSM testing API, located at http://api06.dev.openstreetmap.org/ Using that link it should be possible to create a test account (this is not the same as your regular OSM account). Since the data (nodes, ways, relations) in the test database does not correspond to the objects in the live database (different IDs), and the data we get back from nominatim and overpass when searching and displaying an object can not be used directly for editing I have added a way to hardcode a test object to be used from the test database for the editing by setting a couple of environment variables. This was you could basically search for anything as usual and when you press the "Edit" button, it will load the hard-coded test object from the test database and editing will operate on that. The environment variables that should be set are as follows: OSM_USERNAME=<username in the test database> OSM_PASSWORD=<password in the test database> OSM_MOCK_TYPE=node|way|relation OSM_MOCK_ID=<id of an object in the test database> So, the OSM_MOCK_TYPE would be set to either node, way, or relation depending on what object in the test database would be operating on and OSM_MOCK_ID to the ID of that object. I have created a test object of type node in the test database that could be used to play with: OSM_MOCK_TYPE=node OSM_MOCK_ID=4298258663 Currently, it is only possible to edit the name of the object. The implementation is currently using in-place editing in the popover (and yes, the UI elements aren't particularily well-aligned right now :-) ). We are, in addition to the information we currently show in the popover, probably going to want to have an edit field to enter a comment (this is set on the OSM changeset and is currently not required, but recommended by the OSM API). Furthermore, we have been discussing having a way to add simple nodes to the map (POIs). One thought might be to use a slightly different UI for that and show a menu with a fixed selection of POI types (such as "amenity", "shop", possible a few others) and for each type have fixed set of "subtypes" (for example for "amenity" having "restaurant", "pub", "pharmacy" and so on). Since the tag values in OSM are free-text, it will probably not really be feasable to allow editing those on existing nodes (f.ex. i18n would be rather hard). Explantions of the common OSM tags can be found on: http://wiki.openstreetmap.org/wiki/Map_Features
Thanks! For this and your work! Andreas and me discussed this a bit this weekend. We didn't reach any definitive conclusions, but got somewhere. I think Andreas means to draw some mockups. But some guidence in words. I think we want the edit side of the bubble to resemble that of gnome-contacts. With the name of the fields as placeholders in the text-entries. And an add-new-item combo box down below. How the edit-fields should look I leave to Andreas. I am not sure we want to be able to edit the address-part. So that would leave: add/edit: name, wiki, wheelchair, opening hours and population(?). The adding of password is tricky. Maybe we want to have this in a (new) gear menu? Sort of "Add OpenStreetMap account" ? And should we then only allow username/pw option? I guess anything else would be to cumbersome? Or could we borrow something from GOA there? That is already done and importable? And maybe we would want a splash screen after the features land, urging people to add an account. Hope we can straight these things out :)
Thanks! That sounds reasonable! I was also thinking about the adresses, it might complicate things a bit too much. So I think we could start off with the simple things (at least as a start). Maybe the gear menu could be shared with facilities for downloading offline vector data when that comes? Yeah, I'm not sure how much work OATH would be to implement. It's safe to say that "straight" username/password is easier, since that is what's implemented right now, and is very straight-forward to do with libsoup. Yeah, now it's mostly the hard parts remaining, a good UI! :-)
(In reply to Jonas Danielsson from comment #21) > Thanks! For this and your work! > > Andreas and me discussed this a bit this weekend. We didn't reach any > definitive conclusions, but got somewhere. > > I think Andreas means to draw some mockups. But some guidence in words. I > think we want the edit side of the bubble to resemble that of gnome-contacts. > With the name of the fields as placeholders in the text-entries. And an > add-new-item combo box down below. > > How the edit-fields should look I leave to Andreas. I am not sure we want to > be able to edit the address-part. So that would leave: add/edit: name, wiki, > wheelchair, opening hours and population(?). > > The adding of password is tricky. Maybe we want to have this in a (new) gear > menu? Sort of "Add OpenStreetMap account" ? And should we then only allow > username/pw option? I guess anything else would be to cumbersome? Or could > we borrow something from GOA there? That is already done and importable? I feel this is very similar to the case of broker accounts in Boxes. We have been thinking of doing that management through GOA but issue is that its very specific to Boxes and its unlikely any other app would benefit from this. I think editing of "maps" is very similar in that regard. Jimmac had a tentative mockup for that: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/newbox-assistant-broker.png
Why is the easiest option, embedding the ID editor as a Webview in Maps, not considered? It would be doable in a day and provide an almost complete editing interface -- with advanced search for tags etc. pp. Folks wouldn't have to leave the Gnome Maps application and credentials could be stored in the App/Keychain. There are literally thousands of hours that have already been invested into making the editor, and making it lean and newbie-friendly. I think any editor on top of Gnome Maps would compete in that exact same space as ID editor, since Gnome Apps want to stay simple so competing with JOSM and the likes is unreasonable anyways. I am editing OSM as a hobby and I think that a proper solution is a lot of work, and is very hard to do it simple. That's why I think that using the ID editor as interface is the way to go :)
(In reply to Wolf Vollprecht from comment #24) > Why is the easiest option, embedding the ID editor as a Webview in Maps, not > considered? It would be doable in a day and provide an almost complete > editing interface -- with advanced search for tags etc. pp. Folks wouldn't > have to leave the Gnome Maps application and credentials could be stored in > the App/Keychain. If it's really that easy, maybe you can provide a patch that does that? Even a rough patch to show how feasible it is would give us an idea of the result. > There are literally thousands of hours that have already been invested into > making the editor, and making it lean and newbie-friendly. > > I think any editor on top of Gnome Maps would compete in that exact same > space as ID editor, since Gnome Apps want to stay simple so competing with > JOSM and the likes is unreasonable anyways. > > I am editing OSM as a hobby and I think that a proper solution is a lot of > work, and is very hard to do it simple. That's why I think that using the ID > editor as interface is the way to go :) I wonder how integrated this would look.
Here you go: Both links same video, just different encoding, hope one of them works in FF/Chrome. https://polybox.ethz.ch/public.php?service=files&t=c920b31bec33dadf8901b87ed00359e9 https://polybox.ethz.ch/public.php?service=files&t=bccd8c331982d6d9201a1728fe9dc9f1 The code is, dirty to say the least: let p = this; let style; let eb = this._editButton.connect('clicked', function() { let f = Gio.file_new_for_path('/home/wolfv/Programs/gnome-maps/style.css'); f.load_contents_async(null, function(f, res) { try { style = f.load_contents_finish(res)[1]; print(style) this.cm_manager = new Webkit.UserContentManager() this._user_stylesheet = new Webkit.UserStyleSheet(style.toString(), Webkit.UserContentInjectedFrames.ALL_FRAMES, Webkit.UserStyleLevel.USER, null, null); this.cm_manager.add_style_sheet(this._user_stylesheet) p.editor = Webkit.WebView.new_with_user_content_manager(this.cm_manager) p.editor.load_uri('https://www.openstreetmap.org/edit?editor=id#map=18/47.36478/8.56264') p.editor.set_property('hexpand', true) p._overlay.remove(p._mapView) p._overlay.add(p.editor); p.editor.show() } catch (e) { log("*** ERROR: " + e.message); return; } }); }); I also had to add an edit button to the UI (as you see in the video) and load imports.gi.WebKit2 and Gio. The user-style css looks like this and basically just hides the OSM header: header display: none; #content top: 0 !important; * font-family: 'Cantarell', sans-serif !important; What's not done: Not scrolled to current view (don't know where to get the params from champlain) No cookies / auth mechanism (should be doable but might not be trivial. Easiest would be to add cookies to the webview and let that handle the login process, ie. you could also login to OSM with Google/facebook whatnot). Apart from that it should *just work*. I think it doesn't look too shabby. What could be improved: - Exchange messages with GTK UI to integrate zoom button, the line/area/point/save button - try to remove some more styles from ID editor to make it look a tiny bit more integrated The positive things: - It's a finished editor thats already proven with a minimal amount of extra work that needs to be put in - More or less feature complete OSM editor, strong community - looks modern Negative: - Not 100% GTKy, web-based, loading times - Currently a hack around the osm.org interface -- consider hosting the ID editor somewhere gnome / or even local Anyhow, my 2 cents. - Wolf
These 6 lines make the GTK UI interact with the Javascript resulting in this: https://polybox.ethz.ch/public.php?service=files&t=8c5f454e7ba906b38f038a31d3d7076b this._addPointButton.connect('clicked', function () { p.editor.run_javascript( "document.querySelector('iframe') .contentDocument.querySelector('.add-button') .click()", null, function() {} ) }) It would totally possible to implement all ID actions like this (and hide the buttons through CSS display: none). That could potentially look tightly integrated ;)
Created attachment 313694 [details] [review] osmEdit: Add C shim functions to parse and serialize OSM objects and changesets.
Created attachment 313695 [details] [review] osmEdit: Add OSM edit support. High-level JS implementation for communication with the OSM server. Dialog for editing OSM data and utility functions used for editing.
Created attachment 313696 [details] [review] osmEdit: Hook up edit button in the place bubble. Add the Edit button (this should later be reworked to invoke the account setup if the user has not yet added an OSM account).
Review of attachment 313694 [details] [review]: Thanks! The commit message needs some love, try follow https://wiki.gnome.org/Git/CommitMessages when possible. I haven't looked in detail yet, sorry! ::: lib/Makefile.am @@ +27,3 @@ + maps-osm-object.c \ + maps-osm-way.c \ + maps-osm-relation.c Are these '\' aligned at 72 chars with the rest? ::: lib/maps-osm-node.c @@ +78,3 @@ + case PROP_LAT: + g_value_set_double (value, + node->priv->lat); Why is this line, and the PROP_LON one broken? Does it really go > 80 chars? @@ +149,3 @@ + 0.0, + G_PARAM_READWRITE); + g_object_class_install_property (node_class, PROP_LAT, pspec); Maybe we can be a bit less lazy and use 'longitude' and 'latitude' as property names? @@ +201,3 @@ +{ + g_object_set (node, "lat", lat); +} Why g_object_[set|get] here? Why not just access the priv->lat, priv->lon? And are these functions used at all? Or can we just access the properties directly from gjs? ::: lib/maps-osm-object.c @@ +48,3 @@ + MapsOSMObject *osm_object = MAPS_OSMOBJECT (object); + MapsOSMObjectPrivate *priv = + maps_osm_object_get_instance_private (osm_object); You could just go osm_object->priv = maps_osm_object_get_instance_private (osm_object); once in maps_osm_object_init. And have a priv pointer on the osm_object object. See for instance maps-contact.c @@ +90,3 @@ + case PROP_VERSION: + g_value_set_uint (value, + priv->version); Why break these lines? @@ +122,3 @@ +maps_osm_object_get_xml_attributes (const MapsOSMObject *object) +{ + return g_hash_table_new (g_str_hash, g_str_equal); This can't be null? @@ +243,3 @@ + g_object_set (G_OBJECT (object), "changeset", changeset); +} + Again, why use g_object_[get|set] here, and not the priv pointer? And again: are they needed or can we access properties directly? @@ +268,3 @@ + g_hash_table_remove (priv->tags, key); +} + Should we have some g_return_if's around here as guards? @@ +277,3 @@ + + /* skip tag if it has an empty placeholder value */ + if (val) { For some reason we use GNU style in the lib/ dir so braces on new line. ::: lib/maps-osm-relation.c @@ +100,3 @@ + const GList *members = relation->priv->members; + + if (members) { Here and everywhere, curly brace on new line ::: lib/maps-osm.c @@ +43,3 @@ + + if (!doc) { + g_error ("Failed to parse to XML document"); Should use a GError? @@ +211,3 @@ + */ +MapsOSMNode * +maps_osm_parse_node (const char *content, guint length) Should we take an GError here? So we can propagate errors up? @@ +422,3 @@ + */ +MapsOSMRelation * +maps_osm_parse_relation (const char *content, guint length) GError?
Review of attachment 313695 [details] [review]: Thanks! Sorry for drive by shallow review. ::: data/ui/map-bubble.ui @@ -111,0 +111,6 @@ + <child> + <object class="GtkButton" id="bubble-edit-button"> + <property name="name">bubble-edit-button"</property> ... 3 more ... Should it really? I thought in the mockups that it should always be there? But prompt you to create an account if you haven't one. ::: data/ui/osm-edit-dialog.ui @@ +45,3 @@ + <property name="orientation">vertical</property> + <property name="margin">20</property> + Remove these empty lines @@ +56,3 @@ + </child> + <child> + <object class="GtkBox"> Can we use a Grid instead of a box, here and elsewhere? Maybe we can avoid extra packing tags? ::: src/osmConnection.js @@ +72,3 @@ + cancellable.connect((function() { + this._session.cancel_message(request, Soup.STATUS_CANCELLED); + }).bind(this)); What are we connecting to here exactly? Is this missing 'cancelled'? @@ +80,3 @@ + } + + Utils.debug ('data received: ' + message.response_body.data); Remove this later on @@ +87,3 @@ + + let object = this._parseXML(type, message.response_body); + Remove this empty line @@ +93,3 @@ + callback(true, + message.status_code, + object, type); Do not split this, or if you do, use braces for multi-line statements in if/else @@ +128,3 @@ + default: + GLib.error('unknown OSM type: ' + type); + } I guess the alternative would be to have Maps.osm_parse(type, body.data, body.length) here? And have some kind of factory pattern? Not sure if one is better than the other tho. ::: src/osmEdit.js @@ +28,3 @@ +const Utils = imports.utils; + +const OSMEditManager = new Lang.Class({ I have sort of an aversion against "manager" Can't this be called just "OsmEdit" ? @@ +43,3 @@ + get osmObject() { + return this._osmObject; + }, Can this be just get object() { ... } ? @@ +46,3 @@ + + showEditDialog: function(parentWindow, place) { + let dialog = new OSMEditDialog.OSMEditDialog( { transient_for: parentWindow, Extra space before { @@ +104,3 @@ + callback(false, status); + } + }.bind(this)); This is horrible to read, can this be done better? Maybe by assigning the function to a variable and use that? @@ +120,3 @@ + }.bind(this)); + }, + Same here. @@ +137,3 @@ + callback(false, status); + }.bind(this)); + }, And here. ::: src/osmEditDialog.js @@ +55,3 @@ + + // if the entered text is a Wikipedia link, + // substitute it with the OSM-formatted Wikipedia article tag I prefer /* style comments */ at least for multi line comments, see for instance mapWalker.js @@ +74,3 @@ + type: EditFieldType.INTEGER}, + {name: _("Wheelchair access"), tag: 'wheelchair', + type: EditFieldType.YES_NO_LIMITED_DESIGNATED}]; Other consts we have have been CAPS why is this one different? @@ +102,3 @@ + this._cancellable.connect((function() { + this.response(Response.CANCELLED); + }).bind(this)); Does this work? Doesn't it need to be this._cancellable.connect('cancelled', (function ... ? @@ +154,3 @@ + }, + + _fetchOSMObjectCB: function(success, status, osmObject, osmType) { What is the difference beween _onFunctions and _functionCB? @@ +185,3 @@ + messageDialog.run(); + messageDialog.destroy(); + this.response(Response.ERROR); Can we use in-app notification instead or is Dialog better ux here? @@ +252,3 @@ + this._addOSMEditDeleteButton(tag); + + this._nextRow++; Isnät this functionally currentRow? @@ +319,3 @@ + let rewriteFunc = fieldSpec.rewriteFunc; + + if (value == null) { Should this be === ?
Review of attachment 313696 [details] [review]: Thanks! ::: src/placeBubble.js @@ +142,3 @@ + }, + + // clear the view widgets to be able to re-polute an updated place re-polute? re-populate? @@ +177,3 @@ + default: + break; + I think initEditButton belong in mapBubble.js not here.
Review of attachment 313694 [details] [review]: Maybe the commit message could describe the relation between all these GObjects as well? ::: lib/maps-osm-changeset.c @@ +21,3 @@ + +G_DEFINE_TYPE (MapsOSMChangeset, maps_osm_changeset, + MAPS_TYPE_OSMOBJECT) What exactly is an OSM object? It makes sense to me to list way/node/relation, but changeset? Why does this need to ineherit osmobject? @@ +40,3 @@ +maps_osm_changeset_init (MapsOSMChangeset *changeset) +{ +} That this is empty is a red flag for me ::: lib/maps-osm-object.c @@ +269,3 @@ +} + +void static? @@ +287,3 @@ +} + +void static? @@ +298,3 @@ +} + +xmlDocPtr static? @@ +321,3 @@ + id = priv->id; + version = priv->version; + changeset = priv->changeset; Is this needed? is it to pass 80 chars?
(In reply to Jonas Danielsson from comment #31) > > @@ +268,3 @@ > + g_hash_table_remove (priv->tags, key); > +} > + > > Should we have some g_return_if's around here as guards? > Where you thinking about protecting against NULL for "key" in these functions (get_/set_/delete_tag)?
Created attachment 314856 [details] [review] osmEdit: Hook up edit button in the place bubble Add the Edit button (this should later be reworked to invoke the account setup if the user has not yet added an OSM account).
Created attachment 314857 [details] [review] osmEdit: Add OSM edit support High-level JS implementation for communication with the OSM server. Dialog for editing OSM data and utility functions used for editing.
Created attachment 314858 [details] [review] osmEdit: Add OSM shim library The following GObject classes are defined: MapsOSMObject: An abstract base class representing objects in the OpenStreetMap database. Has a function to serialize objects to their XML representation and abstract functions that implementation classes define to add object-specific XML tags. MapsOSMNode: Represents an object of type ”node” in OSM. Inherits from MapsOSMObject. MapsOSMWay: Represents an object of type ”way” in OSM. Inherits from MapsOSMObject. MapsOSMRelation: Represents an object of type ”relation” in OSM. Inherits from MapsOSMObject. MapsOSMChangeset: Represents a changeset in OSM. Has a function to serialize the changeset (with comment and client identifier) to the XML representation. maps-osm.[c|h]: Contains parsing functions to read OSM objects from the raw XML input as downloaded from the OSM database. Also contains utility functions to initialize and destruct the libxml2 parser.
(In reply to Jonas Danielsson from comment #31) > Review of attachment 313694 [details] [review] [review]: > > Thanks! > > The commit message needs some love, try follow > https://wiki.gnome.org/Git/CommitMessages when possible. > I haven't looked in detail yet, sorry! > Updated commit message, trying to explain the object hierarchies. > ::: lib/Makefile.am > @@ +27,3 @@ > + maps-osm-object.c \ > + maps-osm-way.c \ > + maps-osm-relation.c > > Are these '\' aligned at 72 chars with the rest? Fixed > > ::: lib/maps-osm-node.c > @@ +78,3 @@ > + case PROP_LAT: > + g_value_set_double (value, > + node->priv->lat); > > Why is this line, and the PROP_LON one broken? Does it really go > 80 chars? > Fixed > @@ +149,3 @@ > + 0.0, > + G_PARAM_READWRITE); > + g_object_class_install_property (node_class, PROP_LAT, pspec); > > Maybe we can be a bit less lazy and use 'longitude' and 'latitude' as > property names? > Sure, these followed the internal OSM attributes, but it is probably more clear to spell it out. So I changed. > @@ +201,3 @@ > +{ > + g_object_set (node, "lat", lat); > +} > > Why g_object_[set|get] here? Why not just access the priv->lat, priv->lon? > And are these functions used at all? Or can we just access the properties > directly from gjs? Good point. I removed these, as they are not used at all from JS (just the serializing to XML, inside the C code). > > ::: lib/maps-osm-object.c > @@ +48,3 @@ > + MapsOSMObject *osm_object = MAPS_OSMOBJECT (object); > + MapsOSMObjectPrivate *priv = > + maps_osm_object_get_instance_private (osm_object); > > You could just go osm_object->priv = maps_osm_object_get_instance_private > (osm_object); once in maps_osm_object_init. And have a priv pointer on the > osm_object object. See for instance maps-contact.c > Appearantly, you don't get a priv pointer when using the type declaration macros for an abstract derivable class (as the MapsOSMObject is). > @@ +90,3 @@ > + case PROP_VERSION: > + g_value_set_uint (value, > + priv->version); > > Why break these lines? > Fixed. > @@ +122,3 @@ > +maps_osm_object_get_xml_attributes (const MapsOSMObject *object) > +{ > + return g_hash_table_new (g_str_hash, g_str_equal); > > This can't be null? > I changed this to return NULL and have the XML-generating code take that into consideration. > @@ +243,3 @@ > + g_object_set (G_OBJECT (object), "changeset", changeset); > +} > + > > Again, why use g_object_[get|set] here, and not the priv pointer? And again: > are they needed or can we access properties directly? > > @@ +268,3 @@ > + g_hash_table_remove (priv->tags, key); > +} > + > > Should we have some g_return_if's around here as guards? > Good point! Added guards here. > @@ +277,3 @@ > + > + /* skip tag if it has an empty placeholder value */ > + if (val) { > > For some reason we use GNU style in the lib/ dir so braces on new line. > Fixed. > ::: lib/maps-osm-relation.c > @@ +100,3 @@ > + const GList *members = relation->priv->members; > + > + if (members) { > > Here and everywhere, curly brace on new line > > ::: lib/maps-osm.c > @@ +43,3 @@ > + > + if (!doc) { > + g_error ("Failed to parse to XML document"); > > Should use a GError? > Yes, I refactored the code to use and set a GError in all cases returning NULL, also the JS code handles this in a try-catch. > @@ +211,3 @@ > + */ > +MapsOSMNode * > +maps_osm_parse_node (const char *content, guint length) > > Should we take an GError here? So we can propagate errors up? > > @@ +422,3 @@ > + */ > +MapsOSMRelation * > +maps_osm_parse_relation (const char *content, guint length) > > GError?
(In reply to Jonas Danielsson from comment #32) > Review of attachment 313695 [details] [review] [review]: > > Thanks! > > Sorry for drive by shallow review. > > ::: data/ui/map-bubble.ui > @@ -111,0 +111,6 @@ > + <child> > + <object class="GtkButton" id="bubble-edit-button"> > + <property name="name">bubble-edit-button"</property> > ... 3 more ... > > Should it really? I thought in the mockups that it should always be there? > But prompt you to create an account if you haven't one. Yes, however for the time being this button is placed in the bottom toolbar section (defined in mapBubble) but enabled in placeBubble. I plan to rewrite that once credential management is in place. And have the edit button be defined in placeBubble, and will then be always visible and trigger account setup if needed. So, I thought to change that behavior in a later commit once I have account setup working. > > ::: data/ui/osm-edit-dialog.ui > @@ +45,3 @@ > + <property name="orientation">vertical</property> > + <property name="margin">20</property> > + > > Remove these empty lines > Fixed > @@ +56,3 @@ > + </child> > + <child> > + <object class="GtkBox"> > > Can we use a Grid instead of a box, here and elsewhere? Maybe we can avoid > extra packing tags? > Sure, makes sence. Changed that. > ::: src/osmConnection.js > @@ +72,3 @@ > + cancellable.connect((function() { > + this._session.cancel_message(request, Soup.STATUS_CANCELLED); > + }).bind(this)); > > What are we connecting to here exactly? Is this missing 'cancelled'? > GCancellable has a convienience function g_cancellable_connnect() which connects to the "cancelled" signal. > @@ +80,3 @@ > + } > + > + Utils.debug ('data received: ' + message.response_body.data); > > Remove this later on > > @@ +87,3 @@ > + > + let object = this._parseXML(type, message.response_body); > + > > Remove this empty line Fixed > > @@ +93,3 @@ > + callback(true, > + message.status_code, > + object, type); > > Do not split this, or if you do, use braces for multi-line statements in > if/else Fixed > > @@ +128,3 @@ > + default: > + GLib.error('unknown OSM type: ' + type); > + } > > I guess the alternative would be to have Maps.osm_parse(type, body.data, > body.length) here? And have some kind of factory pattern? Not sure if one is > better than the other tho. > I changed the parsing code to look the actual XML element to determine the type and return a MapsOSMObject * > ::: src/osmEdit.js > @@ +28,3 @@ > +const Utils = imports.utils; > + > +const OSMEditManager = new Lang.Class({ > > I have sort of an aversion against "manager" Can't this be called just > "OsmEdit" ? Sure, changed it to just OSMEdit. > > @@ +43,3 @@ > + get osmObject() { > + return this._osmObject; > + }, > > Can this be just get object() { ... } ? > Fixed. > @@ +46,3 @@ > + > + showEditDialog: function(parentWindow, place) { > + let dialog = new OSMEditDialog.OSMEditDialog( { transient_for: > parentWindow, > > Extra space before { Fixed. > > @@ +104,3 @@ > + callback(false, status); > + } > + }.bind(this)); > > This is horrible to read, can this be done better? Maybe by assigning the > function to a variable and use that? I tried making it more readable by breaking out the function body into separate functions. > > @@ +120,3 @@ > + }.bind(this)); > + }, > + > > Same here. > > @@ +137,3 @@ > + callback(false, status); > + }.bind(this)); > + }, > > And here. > > ::: src/osmEditDialog.js > @@ +55,3 @@ > + > + // if the entered text is a Wikipedia link, > + // substitute it with the OSM-formatted Wikipedia article tag > > I prefer /* style comments */ at least for multi line comments, see for > instance mapWalker.js Sure. > > @@ +74,3 @@ > + type: EditFieldType.INTEGER}, > + {name: _("Wheelchair access"), tag: 'wheelchair', > + type: EditFieldType.YES_NO_LIMITED_DESIGNATED}]; > > Other consts we have have been CAPS why is this one different? > Fixed. > @@ +102,3 @@ > + this._cancellable.connect((function() { > + this.response(Response.CANCELLED); > + }).bind(this)); > > Does this work? Doesn't it need to be this._cancellable.connect('cancelled', > (function ... ? > > @@ +154,3 @@ > + }, > + > + _fetchOSMObjectCB: function(success, status, osmObject, osmType) { > > What is the difference beween _onFunctions and _functionCB? > Changed the *CB functions to _on* for consistancy. > @@ +185,3 @@ > + messageDialog.run(); > + messageDialog.destroy(); > + this.response(Response.ERROR); > > Can we use in-app notification instead or is Dialog better ux here? > Waiting for design feedback for this, I think. > @@ +252,3 @@ > + this._addOSMEditDeleteButton(tag); > + > + this._nextRow++; > > Isnät this functionally currentRow? Yeah, that makes sense, changed (and added a comment when initially setting it. > > @@ +319,3 @@ > + let rewriteFunc = fieldSpec.rewriteFunc; > + > + if (value == null) { > > Should this be === ? Fixed.
(In reply to Jonas Danielsson from comment #34) > Review of attachment 313694 [details] [review] [review]: > > Maybe the commit message could describe the relation between all these > GObjects as well? Yes, I changed to commit message body to include a description of the objects and the parse code. > > ::: lib/maps-osm-changeset.c > @@ +21,3 @@ > + > +G_DEFINE_TYPE (MapsOSMChangeset, maps_osm_changeset, > + MAPS_TYPE_OSMOBJECT) > > What exactly is an OSM object? It makes sense to me to list > way/node/relation, but changeset? Why does this need to ineherit osmobject? > Yes. I changed MapsOSMChangeset to inherit directly from GObject instead of piggy-backing on MapsOSMObject's setting of some common XML attributes to make it more clean. > @@ +40,3 @@ > +maps_osm_changeset_init (MapsOSMChangeset *changeset) > +{ > +} > > That this is empty is a red flag for me > > ::: lib/maps-osm-object.c > @@ +269,3 @@ > +} > + > +void > > static? > Set all internal functions as static. > @@ +287,3 @@ > +} > + > +void > > static? > > @@ +298,3 @@ > +} > + > +xmlDocPtr > > static? > > @@ +321,3 @@ > + id = priv->id; > + version = priv->version; > + changeset = priv->changeset; > > Is this needed? is it to pass 80 chars? Changed to use priv-> directly when used.
Created attachment 314943 [details] [review] osmEdit: Add OSM shim library The following GObject classes are defined: MapsOSMObject: An abstract base class representing objects in the OpenStreetMap database. Has a function to serialize objects to their XML representation and abstract functions that implementation classes define to add object-specific XML tags. MapsOSMNode: Represents an object of type ”node” in OSM. Inherits from MapsOSMObject. MapsOSMWay: Represents an object of type ”way” in OSM. Inherits from MapsOSMObject. MapsOSMRelation: Represents an object of type ”relation” in OSM. Inherits from MapsOSMObject. MapsOSMChangeset: Represents a changeset in OSM. Has a function to serialize the changeset (with comment and client identifier) to the XML representation. maps-osm.[c|h]: Contains parsing functions to read OSM objects from the raw XML input as downloaded from the OSM database. Also contains utility functions to initialize and destruct the libxml2 parser.
Created attachment 314944 [details] [review] osmEdit: Add OSM edit support High-level JS implementation for communication with the OSM server. Dialog for editing OSM data and utility functions used for editing.
Created attachment 314945 [details] [review] osmEdit: Hook up edit button in the place bubble Add the Edit button (this should later be reworked to invoke the account setup if the user has not yet added an OSM account).
Fixed all remaining tab indentations (hopefully didn't miss any…)
Created attachment 315033 [details] [review] osmEdit: Add OSM edit support High-level JS implementation for communication with the OSM server. Dialog for editing OSM data and utility functions used for editing.
I removed some duplicate code in the last patch, due to being able to use the osmTypeToString function in util.js introduced in master for the new Open in browser functionallity.
Created attachment 316815 [details] [review] osmEdit: Add OSM shim library The following GObject classes are defined: MapsOSMObject: An abstract base class representing objects in the OpenStreetMap database. Has a function to serialize objects to their XML representation and abstract functions that implementation classes define to add object-specific XML tags. MapsOSMNode: Represents an object of type ”node” in OSM. Inherits from MapsOSMObject. MapsOSMWay: Represents an object of type ”way” in OSM. Inherits from MapsOSMObject. MapsOSMRelation: Represents an object of type ”relation” in OSM. Inherits from MapsOSMObject. MapsOSMChangeset: Represents a changeset in OSM. Has a function to serialize the changeset (with comment and client identifier) to the XML representation. maps-osm.[c|h]: Contains parsing functions to read OSM objects from the raw XML input as downloaded from the OSM database. Also contains utility functions to initialize and destruct the libxml2 parser.
Fixed a g_return_val_if_failed not returning a value
Created attachment 316816 [details] [review] osmEdit: Implement OAuth sign in support
Created attachment 316817 [details] [review] osmEdit: Implement OSM account dialog
Created attachment 316818 [details] [review] osmEdit: Add specialized OAuthProxyCall subclass Add OAuthProxyCall class supporting setting the request content.
Created attachment 316819 [details] [review] osmEdit: Implement OAuth-authorized calls for updating
Created attachment 316820 [details] [review] osmEdit: Remove stop-gap solutions for password authentication
Created attachment 316821 [details] [review] osmEdit: Move edit button to PlaceBubble and adjust more to the mockup
Created attachment 316822 [details] [review] osmEdit: Invoke the account dialog if trying to edit while not signed in
Created attachment 316823 [details] [review] osmEdit: Invoke the account dialog if trying to edit while not signed in
Thanks this is awesome! Trying it out a bit, got this, with MAPS_DEBUG=1 "Gjs-Message: JS LOG: DEBUG: data received: <?xml version="1.0" encoding="UTF-8"?> <osm version="0.6" generator="CGImap 0.4.0 (8691 thorn-02.openstreetmap.org)" copyright="OpenStreetMap and contributors" attribution="http://www.openstreetmap.org/copyright" license="http://opendatacommons.org/licenses/odbl/1-0/"> <node id="577312628" visible="true" version="9" changeset="31395325" timestamp="2015-05-23T11:30:36Z" user="kisaa" uid="68982" lat="55.5923075" lon="13.0101007"> <tag k="addr:city" v="Malmö"/> <tag k="addr:country" v="SE"/> <tag k="addr:housenumber" v="16"/> <tag k="addr:street" v="Kristianstadsgatan"/> <tag k="amenity" v="cafe"/> <tag k="cuisine" v="vegan"/> <tag k="internet_access" v="wlan"/> <tag k="name" v="Café Glassfabriken"/> <tag k="note" v="Hundar välkomna"/> <tag k="opening_hours" v="Tu-Su 11:00-20:00"/> <tag k="smoking" v="no"/> <tag k="wheelchair" v="limited"/> </node> </osm> Gjs-Message: JS LOG: DEBUG: about open changeset: <?xml version="1.0"?> <osm><changeset><tag k="comment" v="My block"/><tag k="created_by" v="gnome-maps 3.19.2"/></changeset></osm> Gjs-Message: JS LOG: DEBUG: status: 200 Gjs-Message: JS LOG: DEBUG: opened changeset: 35792537, (org.gnome.Maps:4072): Gjs-WARNING **: JS ERROR: Error: Don't know how to convert JavaScript object to GType guint64 OSMConnection<.uploadObject@resource:///org/gnome/Maps/js/osmConnection.js:159 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 OSMEdit<._uploadObject@resource:///org/gnome/Maps/js/osmEdit.js:113 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 OSMEdit<._onChangesetOpened@resource:///org/gnome/Maps/js/osmEdit.js:85 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 OSMEdit<._openChangeset/<@resource:///org/gnome/Maps/js/osmEdit.js:95 OSMConnection<._onChangesetOpened@resource:///org/gnome/Maps/js/osmConnection.js:155 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 OSMConnection<._doOpenChangeset/<@resource:///org/gnome/Maps/js/osmConnection.js:139 OSMEdit<.showEditDialog@resource:///org/gnome/Maps/js/osmEdit.js:50 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 PlaceBubble<._onEditClicked@resource:///org/gnome/Maps/js/placeBubble.js:175 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 main@resource:///org/gnome/Maps/js/main.js:47 run@resource:///org/gnome/gjs/modules/package.js:192 start@resource:///org/gnome/gjs/modules/package.js:176 @/home/jonas/jhbuild/install/bin/gnome-maps:5" Also seems when going back from the Comment section the button in the headerbar of the dialog does not update?
Log in worked tho! And I see the token on openstreetmap.org! You rock!
Created attachment 316854 [details] [review] osmEdit: Reset label of ”Next“ button when going back to editing
Created attachment 316855 [details] [review] osmEdit: Show spinner when uploading to OSM
(In reply to Jonas Danielsson from comment #58) > Thanks this is awesome! > > Trying it out a bit, got this, with MAPS_DEBUG=1 > > "Gjs-Message: JS LOG: DEBUG: data received: <?xml version="1.0" > encoding="UTF-8"?> > <osm version="0.6" generator="CGImap 0.4.0 (8691 > thorn-02.openstreetmap.org)" copyright="OpenStreetMap and contributors" > attribution="http://www.openstreetmap.org/copyright" > license="http://opendatacommons.org/licenses/odbl/1-0/"> > <node id="577312628" visible="true" version="9" changeset="31395325" > timestamp="2015-05-23T11:30:36Z" user="kisaa" uid="68982" lat="55.5923075" > lon="13.0101007"> > <tag k="addr:city" v="Malmö"/> > <tag k="addr:country" v="SE"/> > <tag k="addr:housenumber" v="16"/> > <tag k="addr:street" v="Kristianstadsgatan"/> > <tag k="amenity" v="cafe"/> > <tag k="cuisine" v="vegan"/> > <tag k="internet_access" v="wlan"/> > <tag k="name" v="Café Glassfabriken"/> > <tag k="note" v="Hundar välkomna"/> > <tag k="opening_hours" v="Tu-Su 11:00-20:00"/> > <tag k="smoking" v="no"/> > <tag k="wheelchair" v="limited"/> > </node> > </osm> > > Gjs-Message: JS LOG: DEBUG: about open changeset: > <?xml version="1.0"?> > <osm><changeset><tag k="comment" v="My block"/><tag k="created_by" > v="gnome-maps 3.19.2"/></changeset></osm> > > > Gjs-Message: JS LOG: DEBUG: status: 200 > Gjs-Message: JS LOG: DEBUG: opened changeset: 35792537, > > (org.gnome.Maps:4072): Gjs-WARNING **: JS ERROR: Error: Don't know how to > convert JavaScript object to GType guint64 > OSMConnection<.uploadObject@resource:///org/gnome/Maps/js/osmConnection.js: > 159 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > OSMEdit<._uploadObject@resource:///org/gnome/Maps/js/osmEdit.js:113 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > OSMEdit<._onChangesetOpened@resource:///org/gnome/Maps/js/osmEdit.js:85 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > OSMEdit<._openChangeset/<@resource:///org/gnome/Maps/js/osmEdit.js:95 > OSMConnection<._onChangesetOpened@resource:///org/gnome/Maps/js/ > osmConnection.js:155 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > OSMConnection<._doOpenChangeset/<@resource:///org/gnome/Maps/js/ > osmConnection.js:139 > OSMEdit<.showEditDialog@resource:///org/gnome/Maps/js/osmEdit.js:50 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > PlaceBubble<._onEditClicked@resource:///org/gnome/Maps/js/placeBubble.js:175 > wrapper@resource:///org/gnome/gjs/modules/lang.js:178 > main@resource:///org/gnome/Maps/js/main.js:47 > run@resource:///org/gnome/gjs/modules/package.js:192 > start@resource:///org/gnome/gjs/modules/package.js:176 > @/home/jonas/jhbuild/install/bin/gnome-maps:5" > I couldn't reproduce this :-( So, I guess we'll need some more debugging here… > > Also seems when going back from the Comment section the button in the > headerbar of the dialog does not update? Yeah, we forgot to reset the label of the button back. Fixed in a new patch.
I felt that the UI felt a bit stuck when clicking the “Upload“ button, since the view remained showing the comment page. So, I attached a patch that turns on the ”loading“ spinner when starting uploading.
Created attachment 316856 [details] [review] osmEdit: Show spinner when uploading to OSM Show the “working spinner” when uploading. Also disable the navigational buttons, as there is no way to go back.
(In reply to Marcus Lundblad from comment #64) > Created attachment 316856 [details] [review] [review] > osmEdit: Show spinner when uploading to OSM > > Show the “working spinner” when uploading. > Also disable the navigational buttons, as there is no way to go back. Also folded in de-sensitizing of the ”Upload“ and back button when starting the upload into this patch.
Review of attachment 314945 [details] [review]: Thanks! I think the initing of the edit-button should be in the mapBubble. You specify in the params to the mapBubble super class that you want edit functionality. It should be added there.
Review of attachment 316816 [details] [review]: ::: configure.ac @@ +39,3 @@ gtk+-3.0 >= $GTK_MIN_VERSION geoclue-2.0 >= $GEOCLUE_MIN_VERSION + webkit2gtk-4.0 Is this explicitly needed? Do we need to check for libsecret/librest?
Review of attachment 315033 [details] [review]: The default action of pressing enter in the password field should be to sign-in I feel. ::: data/ui/map-bubble.ui @@ +114,3 @@ + <property name="label" translatable="yes">Edit</property> + <!-- TODO: this button should be invisible by default + when we handle OSM accounts --> Is this still true?
Created attachment 316902 [details] [review] osmEdit: Implement OAuth sign in support
(In reply to Jonas Danielsson from comment #67) > Review of attachment 316816 [details] [review] [review]: > > ::: configure.ac > @@ +39,3 @@ > gtk+-3.0 >= $GTK_MIN_VERSION > geoclue-2.0 >= $GEOCLUE_MIN_VERSION > + webkit2gtk-4.0 > > Is this explicitly needed? Do we need to check for libsecret/librest? Right, that was unneeded. I think I added that when trying to figure out why the GtkBuilder .ui files didn't "pick up" the WebView widget.
(In reply to Jonas Danielsson from comment #68) > Review of attachment 315033 [details] [review] [review]: > > The default action of pressing enter in the password field should be to > sign-in I feel. > > ::: data/ui/map-bubble.ui > @@ +114,3 @@ > + <property name="label" translatable="yes">Edit</property> > + <!-- TODO: this button should be invisible by default > + when we handle OSM accounts --> > > Is this still true? This is nullified by a later patch, moving the edit button to the place bubble (and always visible), as per the mockups.
Created attachment 316903 [details] [review] osmEdit: Use JS parseInt() instead of GLib.ascii_strtoull() It seems some versions of GJS fails to pass along variables parsed with GLib.ascii_strtoull() to GObjects properties of type guint64.
Created attachment 316905 [details] [review] osmEdit: Let the password and verification code entries handle enter If the user presses enter in the password entry and the verification code entry when setting up an accound, automatically proceed with the actions if needed data has been supplied.
(In reply to Jonas Danielsson from comment #59) > Log in worked tho! And I see the token on openstreetmap.org! > > You rock! Thanks! :-)
Created attachment 317168 [details] [review] osmEdit: Add specialized OAuthProxyCall subclass Add OAuthProxyCall class supporting setting the request content.
Created attachment 317169 [details] [review] osmEdit: Add OSM edit and account support High-level JS implementation for communication with the OSM server. Implementing the OAuth 1.0a protocol to enroll user credentials and store credentials in the system keyring using libsecret. Dialog for setting up OSM account. Dialog for editing OSM data and utility functions used for editing.
Created attachment 317170 [details] [review] osmEdit: Add OSM edit and account support High-level JS implementation for communication with the OSM server. Implementing the OAuth 1.0a protocol to enroll user credentials and store credentials in the system keyring using libsecret. Dialog for setting up OSM account. Dialog for editing OSM data and utility functions used for editing.
I re-arranged the patch set. Now, there is tree patches. First patch contains the C shim for parsing and serializing OSM objects using libxml2 (this is not updated since before). Second patch implements the custom OAuthProxyCall subclass implementing serialization of HTTP request data in PUT and DELETE REST calls. Third patch is now the high-level OSM communication code and UI, so that the churn from the earlier stop-gap code should be gone (and also the left-over debug calls).
Created attachment 317184 [details] [review] osmEdit: Add OSM edit and account support High-level JS implementation for communication with the OSM server. Implementing the OAuth 1.0a protocol to enroll user credentials and store credentials in the system keyring using libsecret. Dialog for setting up OSM account. Dialog for editing OSM data and utility functions used for editing.
Review of attachment 316815 [details] [review]: Thanks! The code here looks clean. But I want to take a closer look at it, because it is so much :) And at some point (this could be after first merge) I want to run valgrind on is. The osm-object children: node, way, relation - the reason they need to be in C is ... ? (maybe they have to, I haven't read it careful enough yet)
Review of attachment 317168 [details] [review]: Looks fine! Thanks!
(In reply to Jonas Danielsson from comment #80) > Review of attachment 316815 [details] [review] [review]: > > Thanks! > > The code here looks clean. But I want to take a closer look at it, because > it is so much :) > And at some point (this could be after first merge) I want to run valgrind > on is. > Yeah, it had crossed my mind as well :-) > The osm-object children: node, way, relation - the reason they need to be in > C is ... ? (maybe they have to, I haven't read it careful enough yet) They use xmlNodePtr from libxml2 (actually, when I think about it, OSMNode doesn't actually implement the "fill custom XML" interface, OSMWay and OSMRelation implements this to add the <nd/> and <member/> subnodes describing the way nodes and relation memebers, respectively).
Attachment 316815 [details] pushed as 60471b8 - osmEdit: Add OSM shim library Attachment 317168 [details] pushed as 1754186 - osmEdit: Add specialized OAuthProxyCall subclass Attachment 317184 [details] pushed with modifications as ce39c3c - osmEdit: Add OSM edit and account support