After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740356 - Add 'add-location' action
Add 'add-location' action
Status: RESOLVED FIXED
Product: gnome-clocks
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Clocks maintainer(s)
Clocks maintainer(s)
Depends on:
Blocks: 728117
 
 
Reported: 2014-11-19 12:00 UTC by Jonas Danielsson
Modified: 2014-11-20 06:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add add_location action (2.83 KB, patch)
2014-11-19 12:00 UTC, Jonas Danielsson
none Details | Review
Add add_location action (2.86 KB, patch)
2014-11-19 12:22 UTC, Jonas Danielsson
none Details | Review
Add add_location action (2.86 KB, patch)
2014-11-19 12:23 UTC, Jonas Danielsson
accepted-commit_now Details | Review
Add add_location action (3.34 KB, patch)
2014-11-19 14:00 UTC, Jonas Danielsson
committed Details | Review
world: Do not add duplicate locations (2.35 KB, patch)
2014-11-19 14:00 UTC, Jonas Danielsson
committed Details | Review

Description Jonas Danielsson 2014-11-19 12:00:26 UTC
Add an action to add a location to the world clock.
Comment 1 Jonas Danielsson 2014-11-19 12:00:44 UTC
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.
Comment 2 Jonas Danielsson 2014-11-19 12:22:51 UTC
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.
Comment 3 Jonas Danielsson 2014-11-19 12:23:57 UTC
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.
Comment 4 Jonas Danielsson 2014-11-19 12:24:54 UTC
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?
Comment 5 Paolo Borelli 2014-11-19 13:09:50 UTC
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
Comment 6 Jonas Danielsson 2014-11-19 14:00:32 UTC
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.
Comment 7 Jonas Danielsson 2014-11-19 14:00:36 UTC
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.
Comment 8 Jonas Danielsson 2014-11-19 14:01:08 UTC
So with this approach the 'Add' button will not become sensitive if the location already exists.
Comment 9 Jonas Danielsson 2014-11-19 14:01:38 UTC
Tested with the code in the maps bug, tried adding different locations, duplicates did not get added.
Comment 10 Paolo Borelli 2014-11-19 14:45:15 UTC
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
Comment 11 Paolo Borelli 2014-11-19 14:45:36 UTC
Review of attachment 291000 [details] [review]:

perfect, thanks
Comment 12 Jonas Danielsson 2014-11-20 06:04:36 UTC
Review of attachment 291001 [details] [review]:

Thanks for review!

Will fixup and commit!