GNOME Bugzilla – Bug 740356
Add 'add-location' action
Last modified: 2014-11-20 06:18:07 UTC
Add an action to add a location to the world clock.
Created attachment 290976 [details] [review] Add add_location action Add an action to add a location to world. The parameter is a serialized GWeather.Location which is unfortunate but for the moment might be the best. This is needed to allow other applications, such as Maps, to share locations with us.
Created attachment 290980 [details] [review] Add add_location action Add an action to add a location to world. The parameter is a serialized GWeather.Location which is unfortunate but for the moment might be the best. This is needed to allow other applications, such as Maps, to share locations with us.
Created attachment 290982 [details] [review] Add add_location action Add an action to add a location to world. The parameter is a serialized GWeather.Location which is unfortunate but for the moment might be the best. This is needed to allow other applications, such as Maps, to share locations with us.
Review of attachment 290982 [details] [review]: ::: src/world.vala @@ +395,3 @@ + locations.append (item); + content_view.add_item (item); + save (); Should there be checks to see if it is already added?
Review of attachment 290982 [details] [review]: Cool! that was fast... The patch looks good to commit if you tested it and with the minor nitpick below amended. ::: src/application.vala @@ -102,2 +103,4 @@ } + public void on_add_location_activate (GLib.SimpleAction action, GLib.Variant? parameter) { + if (parameter == null) nitpick: always use {} also for 1 line if (also below) ::: src/world.vala @@ +395,3 @@ + locations.append (item); + content_view.add_item (item); + save (); Yes, we should prevent adding duplicates, though that should be not done here but more generically so that we also prevent adding duplicates from the "new" dialog. I do not consider this a blocker to merge this patch, but if you feel like doing such a patch it would be *very* appreciated
Created attachment 291000 [details] [review] Add add_location action Add an action to add a location to world. The parameter is a serialized GWeather.Location which is unfortunate but for the moment might be the best. This is needed to allow other applications, such as Maps, to share locations with us.
Created attachment 291001 [details] [review] world: Do not add duplicate locations Add checks to see if the location already exists before adding. In the case of adding through the 'add-location' action just silently omit to add. When adding through the LocationDialog do not allow adding if the location exists.
So with this approach the 'Add' button will not become sensitive if the location already exists.
Tested with the code in the maps bug, tried adding different locations, duplicates did not get added.
Review of attachment 291001 [details] [review]: We could maybe be slightly fancier with a signal to avoid coupling the two classes, but who cares, it is pretty straight forward internal code. Looks good to commit with a minor style nitpick (see below) Thanks a lot for going the extra mile with this ::: src/world.vala @@ -181,1 +182,1 @@ - public LocationDialog (Gtk.Window parent) { + public LocationDialog (Gtk.Window parent, Face worldFace) { worldFace -> world_face
Review of attachment 291000 [details] [review]: perfect, thanks
Review of attachment 291001 [details] [review]: Thanks for review! Will fixup and commit!