Merge lp:~donadigo/pantheon-agent-geoclue2/code-improvements into lp:~elementary-pantheon/pantheon-agent-geoclue2/trunk

[Adam Bieńkowski]

* Initialize fields in construct
* Make GeoClue2Manager a singleton
* Add null checks for methods that need them

This branch improves the way initializing variables, client and manager handling works.

Initializing is now done in the construct clause, and the assignments in fields are now gone.

Instead of getting the proxy of the GeoClue Manager everytime, it is now a singleton, so it is initialized once.

There are also some null checks added because methods like get_geoclue_client could return a nullable object.

There are still some things to think about, but I think it's a good start.

[lp:~donadigo/pantheon-agent-geoclue2/code-improvements]

17. By Adam Bieńkowski on 2017-02-25

Resolve conflicts

18. By Adam Bieńkowski on 2017-02-25

Start / stop the client only when needed

19. By Adam Bieńkowski on 2017-02-25

Make get_remembered_accuracy nullable; initialize client everytime authorization happens

[David Hewitt (davidmhewitt)]

Thanks for this, makes some good improvements.

review: Approve (code, function)

=== modified file 'src/Agent.vala'
--- src/Agent.vala	2017-02-25 00:36:08 +0000
+++ src/Agent.vala	2017-02-25 15:02:07 +0000
@@ -21,19 +21,20 @@
 
 namespace Ag {
     public class Agent : Gtk.Application, GeoClue2Agent {
-        private const string app_id = "org.pantheon.agent-geoclue2";
-
         public uint max_accuracy_level { get { return GeoClue2.AccuracyLevel.EXACT; } }
+
         private MainLoop loop;
 		private uint object_id;
 		private bool bus_registered = false;
 
 		private GeoClue2Client? client = null;
-		private Settings settings = new Settings (app_id);
+		private Settings settings;
 		private VariantDict remembered_apps;
 
-        public Agent () {
-            Object (application_id: app_id);
+        construct {
+            application_id = "org.pantheon.agent-geoclue2";
+            settings = new Settings (application_id);
+
             loop = new MainLoop ();      
 			load_remembered_apps (); 
         }
@@ -53,11 +54,9 @@
 			    object_id = conn.register_object ("/org/freedesktop/GeoClue2/Agent", (GeoClue2Agent)this);
 			    bus_registered = true;
 			    register_with_geoclue.begin ();
-
-				
 		    } catch (Error e) {
 			    error ("Error while registering the agent: %s \n", e.message);
-		    }
+		    }	    	
 	    }
 
 	    private void watch (DBusConnection connection) {
@@ -75,10 +74,11 @@
 		    if (bus_registered) {
 			    connection.unregister_object (object_id);
 			}
+
 		    base.dbus_unregister (connection, object_path);
 	    }
         
-        public void authorize_app (string id, uint req_accuracy, out bool authorized, out uint allowed_accuracy) {
+        public async void authorize_app (string id, uint req_accuracy, out bool authorized, out uint allowed_accuracy) {
 			debug ("Request for '%s' at level '%u'", id, req_accuracy);
 
 			DesktopAppInfo app_info = new DesktopAppInfo (id + ".desktop");
@@ -92,28 +92,28 @@
 			// Reload the config in case something else changed it
 			load_remembered_apps ();
 
-			Variant remembered_accuracy = get_remembered_accuracy (id);
+			Variant? remembered_accuracy = get_remembered_accuracy (id);
 			if (remembered_accuracy != null) {
 				var stored_accuracy = remembered_accuracy.get_uint32();
 				if (req_accuracy <= stored_accuracy) {
 					authorized = true;
 					allowed_accuracy = req_accuracy;
 					return;
-				}				
+				}
 			}
 
 			string app_name = app_info.get_display_name ();
 			string accuracy_string = accuracy_to_string (app_name, req_accuracy);
 
-			debug ("Registering client...");
-			get_geoclue_client.begin ((obj, res) => {
-				client = get_geoclue_client.end (res);
+			client = yield get_geoclue_client ();
+			debug ("Starting client...");
+			if (client != null) {
 				try {
 					client.start ();
-				} catch (Error e) {
-					warning ("Error while registering geoclue client: %s", e.message);
-				}
-			}); 
+			    } catch (Error e) {
+			    	error ("Could not start client: %s", e.message);
+			    }
+			}
 
 			var dialog = new Widgets.Geoclue2Dialog (accuracy_string, app_name, app_info.get_icon ().to_string ());
 			dialog.show_all ();
@@ -137,15 +137,16 @@
 
 			dialog.destroy ();
 
-			if (client != null) {
+            allowed_accuracy = req_accuracy;
+
+            debug ("Stopping client...");
+		    if (client != null) {
 				try {
 					client.stop ();
-				} catch (Error e) {
-					warning ("Error while stopping geoclue client: %s", e.message);
-				}
-			}
-
-            allowed_accuracy = req_accuracy;
+			    } catch (Error e) {
+			    	error ("Could not stop client: %s", e.message);
+			    }
+		    }            
         }
 
 		private string accuracy_to_string (string app_name, uint accuracy) {
@@ -175,11 +176,11 @@
 		}
 
         private async void register_with_geoclue () {
-            yield Utils.register_with_geoclue (app_id);
+            yield Utils.register_with_geoclue (application_id);
         }
 
-		private async GeoClue2Client get_geoclue_client () {
-			return yield Utils.get_geoclue2_client (app_id);
+		private async GeoClue2Client? get_geoclue_client () {
+			return yield Utils.get_geoclue2_client (application_id);
 		}
 
 		private void load_remembered_apps () {
@@ -191,7 +192,7 @@
 			settings.set_value ("remembered-apps", remembered_apps.end ());
 		}
 
-		public Variant get_remembered_accuracy (string desktop_id) {
+		public Variant? get_remembered_accuracy (string desktop_id) {
 			return remembered_apps.lookup_value (desktop_id, GLib.VariantType.UINT32);
 		}
     }

=== modified file 'src/Interfaces.vala'
--- src/Interfaces.vala	2017-02-21 00:10:21 +0000
+++ src/Interfaces.vala	2017-02-25 15:02:07 +0000
@@ -29,7 +29,7 @@
     [DBus (name = "org.freedesktop.GeoClue2.Agent")]
 	public interface GeoClue2Agent : GLib.Object {
         [DBus (name = "AuthorizeApp")]
-		public abstract void authorize_app(string desktop_id, uint req_accuracy_level, out bool authorized, out uint allowed_accuracy_level) throws DBusError, IOError;
+		public abstract async void authorize_app(string desktop_id, uint req_accuracy_level, out bool authorized, out uint allowed_accuracy_level) throws DBusError, IOError;
         [DBus (name ="MaxAccuracyLevel")]
 		public abstract uint max_accuracy_level {  get; }
 	}

=== modified file 'src/Utils.vala'
--- src/Utils.vala	2017-02-21 00:10:21 +0000
+++ src/Utils.vala	2017-02-25 15:02:07 +0000
@@ -23,28 +23,27 @@
     public const string GEOCLUE2_MANAGER_IFACE = "org.freedesktop.GeoClue2";
     public const string GEOCLUE2_MANAGER_PATH = "/org/freedesktop/GeoClue2/Manager";
 
-    public async void register_with_geoclue (string app_id) {
-        try {
-            GeoClue2Manager? manager = yield Bus.get_proxy (BusType.SYSTEM, GEOCLUE2_MANAGER_IFACE, GEOCLUE2_MANAGER_PATH);
-            yield manager.add_agent (app_id);
-        } catch (Error e) {
-            warning ("Unable to register with GeoClue2: %s", e.message);
+    public async void register_with_geoclue (string app_id) {    
+        GeoClue2Manager? manager = yield get_geoclue_manager ();
+        if (manager != null) {
+            try {
+                yield manager.add_agent (app_id);
+            } catch (Error e) {
+                warning ("Unable to register with GeoClue2: %s", e.message);
+            }
         }
     }
 
     public async GeoClue2Client? get_geoclue2_client (string app_id) {
+        GeoClue2Manager? manager = yield get_geoclue_manager ();
+        if (manager == null) {
+            return null;
+        }
+
         GeoClue2Client? client = null;
-        ObjectPath? path = null;
-
-        try {
-            GeoClue2Manager? manager = yield Bus.get_proxy (BusType.SYSTEM, GEOCLUE2_MANAGER_IFACE, GEOCLUE2_MANAGER_PATH);
-            path = yield manager.get_client ();
-        } catch (Error e) {
-            warning ("Unable to register with GeoClue2: %s", e.message);
-            return null;
-        }
-
-        try {
+
+        try {
+            ObjectPath? path = yield manager.get_client ();
             client = yield Bus.get_proxy (BusType.SYSTEM, GEOCLUE2_MANAGER_IFACE, path);
         } catch (Error e) {
             warning ("Unable to get Private Client proxy: %s", e.message);
@@ -57,4 +56,17 @@
 
         return client;
     }
+
+    private static GeoClue2Manager? instance;
+    private static async unowned GeoClue2Manager? get_geoclue_manager () {
+        if (instance == null) {
+            try {
+                instance = yield Bus.get_proxy (BusType.SYSTEM, GEOCLUE2_MANAGER_IFACE, GEOCLUE2_MANAGER_PATH);
+            } catch (Error e) {
+                warning ("Unable to connect to %s: %s", GEOCLUE2_MANAGER_PATH, e.message);
+            }
+        }
+
+        return instance;
+    }    
 }