(In reply to comment #0)
> In gnome-maps @3.11.92 an ugly error is displayed when gnome-maps is not able
> to connect to geoclue2. I think some information could be provided to the user;

Agreed.

> or if geoclue2 is really needed it could be listed in configure.ac.

configure script is for checking buildtime deps, not runtime.

Created attachment 274839 [details] [review]
Add init method for geoclue module

The dbus code can fail if geoclue is not present on the system. Move
code that can fail out of the _init method to make sure object
creation does not fail.

Created attachment 274840 [details] [review]
geoclue: return status from findLocation

Return false if findLocation fails because of no connection with geoclue.

Created attachment 274841 [details] [review]
notification: Add NO_LOCATION notification type

Created attachment 274842 [details] [review]
mainWindow: Show notification if geoclue fails

Created attachment 274879 [details] [review]
geoclue: Handle Geoclue service not available

The dbus code can fail if geoclue is not present on the system. Move
code that can fail to a try/catch block to handle geoclue not
available. Also make sure that methods using Geoclue are aware that
it might not be there.

The last patch obsoletes "Add init method for geoclue module". Since I ended up not caring if initDBus returned false or not.

Review of attachment 274840 [details] [review]:

ack

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +81,3 @@
 });
 
+const NoLocation = new Lang.Class({

* I'm not sure we want a new class for just one particular hardcoded notification.
* If we do, it belongs in geoclue module.

@@ +91,3 @@
+});
+
+const Type = {

What is this Type about? I don't see it being accessed from anywhere.

@@ +97,1 @@
 const Manager = new Lang.Class({

I think we should keep all non-simple classes in their own files and named appropriately.

Review of attachment 274842 [details] [review]:

::: src/mainWindow.js
@@ +268,3 @@
                 this.mapView.gotoUserLocation(true);
             }).bind(this));
+            if (!this.mapView.geoclue.findLocation()) {

IMO it should be geoclue that should display this notification. Would be also a way to avoid adding more code to MainWindow.

Review of attachment 274879 [details] [review]:

'Geoclue' redundant in commit shortlog. Looks good otherwise.

::: src/geoclue.js
@@ +95,3 @@
     },
 
     findLocation: function() {

hmm.. wasn't this returning false after another patch of yours?

Review of attachment 274879 [details] [review]:

::: src/geoclue.js
@@ +95,3 @@
     },
 
     findLocation: function() {

Yeah, this is meant to go in first, then I had one were findLocation returns false on fail, but if we go with geoclue displaying the Notification then that patch is redundant.

Review of attachment 274842 [details] [review]:

::: src/mainWindow.js
@@ +268,3 @@
                 this.mapView.gotoUserLocation(true);
             }).bind(this));
+            if (!this.mapView.geoclue.findLocation()) {

Agreed.

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +81,3 @@
 });
 
+const NoLocation = new Lang.Class({

Yeah, I understands your thinking. Having a class is a way of making sure that the notification does not get added to the reveal-thingie more than once. So repeated presses of the location buttons does not mean a lot of notifications to dismiss.

I guess this is modeled after how Documents does it in its notifications.js with its PrintNotification and so on.

@@ +91,3 @@
+});
+
+const Type = {

It's a thingie to identify the classes you see it accessed when firing the notification in the patch that does it from MainWindow

  let notification = Notification.Type.NO_LOCATION;
  Application.notificationManager.showNotification(notification);

I guess it could be just be Notification.NoLocation, I remember asking Mattias about that when notifications was about to go in, but do not remember the reason why the Type step was needed.

@@ +97,1 @@
 const Manager = new Lang.Class({

so a notificationManager.js for this?

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +81,3 @@
 });
 
+const NoLocation = new Lang.Class({

I don't get it. I don't see this subclass doing anything to ensure that its the only one or is the Manager ensuring that based on types of notification?

@@ +91,3 @@
+});
+
+const Type = {

Seems really weird so lets find out that rationale.

@@ +97,1 @@
 const Manager = new Lang.Class({

yeah

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +81,3 @@
 });
 
+const NoLocation = new Lang.Class({

Yes, the manager will create an instance of the Class specified in the notificationType you send in. Then it will cache that instance. The next time a request to show a notification of that type the cached instance will be fetched. Then the manager checks if that instance has a parent, if not it will be added to the overlay.

@@ +91,3 @@
+});
+
+const Type = {

So the rationale can be read here: https://bugzilla.gnome.org/show_bug.cgi?id=723996#c33

To sum it up, the reason to pass the type is that then the caller will not have to create an instance each time you want to display that notification. And it will make it easier to see avoid duplicates.

The API is a bit awkward so something simpler would be nice. Will post the patch series again to show you how this could be with the current interface.

Created attachment 274950 [details] [review]
Split out notification manager to its own module

Created attachment 274951 [details] [review]
notification: Remove unused Type const

Created attachment 274952 [details] [review]
geoclue: Handle service not available

The dbus code can fail if geoclue is not present on the system. Move
code that can fail to a try/catch block to handle geoclue not
available. Also make sure that methods using Geoclue are aware that
it might not be there.

Created attachment 274953 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
a error notification.

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +91,3 @@
+});
+
+const Type = {

Thanks, very weird and obscure indeed! Can we please change it to something saner before buidling on top of it?

Created attachment 274975 [details] [review]
notificationManager: Simplify showNotification

Right now showNotification takes a tuple of name and class to create
and cache a kind of simpleton Notification class. This makes the API
a bit awkward.

Instead we can just have the method take a Notification instance
directly. If the caller want the functionality of re-usable
notifications they can do the simpleton creation on their end.
The method still checks if the notification instance is already
in the overlay.

Created attachment 274976 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
an error notification.

Created attachment 274977 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
an error notification.

Created attachment 274978 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
an error notification.

Review of attachment 274975 [details] [review]:

Looks good but shouldn't the using code be updated in the same patch? Otherwise, this change is not self-contained and breaks Maps.

Review of attachment 274950 [details] [review]:

ack

Review of attachment 274951 [details] [review]:

unused -> now unused. ACK either way.

Review of attachment 274952 [details] [review]:

ack

Review of attachment 274975 [details] [review]:

As far as I know there is no using code of the showNotification method, the geoclue use that comes later in this series would be the first.

Review of attachment 274978 [details] [review]:

::: src/geoclue.js
@@ +99,3 @@
+        if (!this._clientProxy) {
+            let notification = this._getNoServiceNotification();
+            Application.notificationManager.showNotification(notification);

* Better just let the function take care of both creation of notification and displaying it: _showNoServiceNotification.
* Does this mean, notification will be displayed each time use hits 'Find me' button? If so, thats not likely what we'd want. How about we display notification only once and then disable the button?

Review of attachment 274975 [details] [review]:

Ah ok then.

Review of attachment 274978 [details] [review]:

::: src/geoclue.js
@@ +99,3 @@
+        if (!this._clientProxy) {
+            let notification = this._getNoServiceNotification();
+            Application.notificationManager.showNotification(notification);

Sure, the function can do it all.

Yeah, it will be displayed every time you hit the button (if you have dismissed the previous one).
Disabling it after the first time seems like the right thing to do, but then we also need to make mainWindow aware of the failure, so bring back the "return false" of findLocation? Or have a isConnected method on geoclue?

Review of attachment 274978 [details] [review]:

::: src/geoclue.js
@@ +99,3 @@
+        if (!this._clientProxy) {
+            let notification = this._getNoServiceNotification();
+            Application.notificationManager.showNotification(notification);

return false approach seems fine to me but can't geoclue module disable the button?

Created attachment 274992 [details] [review]
Split out notification manager to its own module

Created attachment 274993 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
an error notification.

Created attachment 274994 [details] [review]
Disable user location button if findLocation fails

resubmitted the split out notificationManager patch because of a bug where it tried to instantiate Plain and not Notification.Plain and added the whole GPL/AUTHOR thing while I was at it.

Created attachment 274995 [details] [review]
geoclue: Show notification on failed connection

If findLocation is called when Geoclue is not available display
an error notification.

Created attachment 274996 [details] [review]
Disable user location button if findLocation fails

Review of attachment 274992 [details] [review]:

Hm, why?

Review of attachment 274975 [details] [review]:

This patch reverts the code that makes notificationManager ensure that we never present duplicate notifications. 
It's definitely possible to make the API simpler without doing this. For example one could make a simple factory
function for message like notifications which would have removed a lot of the noise.

Also in your old patch you defined the notification in geoclue.js, I would have added it to notification.js as
a static notification and added it to the Types dict there and just use it inside geoclue.js.

Review of attachment 274975 [details] [review]:

It is still possible not to have duplicate notifications with this code, it just moves the responsibility of the singleton to the caller.
As was the case with my old geoclue patch that created a singleton notification that was added each time the button was pressed.

I had the notification in notification.js in my first patch revision but Zeeshan pointed out that it should live with geoclue.
Which makes sense. The approach you have in mind seem to be that notification.js holds all the static notification Maps would use and we get them from there.

If we go that route maybe there should be a notificationFactory sort of pattern. But I am wondering if it's over-engineering it.

With the approach of this patch one can choose if you want the "complex" notifications to be re-usable or not from the caller side, by sending in the same instance over and over again or not.

Review of attachment 274996 [details] [review]:

::: src/mainWindow.js
@@ +270,3 @@
             }).bind(this));
+            if (!this.mapView.geoclue.findLocation())
+                this._gotoUserLocationButton.sensitive = false;

This means that the button won't become sensitive again if geoclue becomes available again right?

Also I think the "correct" way to do this is disabling the 'goto-user-location' action instead. Look at https://developer.gnome.org/gio/unstable/GSimpleAction.html#GSimpleAction-activate

Review of attachment 274995 [details] [review]:

::: src/geoclue.js
@@ +98,3 @@
+        if (!this._clientProxy) {
+            let msg = _("Failed to connect to Geoclue location service");
+            Application.notificationManager.showMessage(msg);

Like previously stated this would mean that any further calls to findLocation would add another message above the previous even if there is another instance of this notification around.

Review of attachment 274995 [details] [review]:

Yes, that I am aware of. I changed to this instead of the previous singleton since we were going to disable the button on failure anyway. Then it felt unnecessary to create a notification class to pass in.
So in this patch this is by design.

Review of attachment 274996 [details] [review]:

::: src/mainWindow.js
@@ +270,3 @@
             }).bind(this));
+            if (!this.mapView.geoclue.findLocation())
+                this._gotoUserLocationButton.sensitive = false;

Yes that is true. This is more a case for when users to not have Geoclue or something is wrong with their install. I am not aware of a use-case where Geoclue can be not available and then available again later, Zeeshan?

You are right about the "correct" way, thanks!

Created attachment 274998 [details] [review]
Disable user location button if findLocation fails

Review of attachment 274998 [details] [review]:

::: src/mainWindow.js
@@ +268,3 @@
             }).bind(this));
+            if (!this.mapView.geoclue.findLocation()) {
+                let action = this.window.lookup_action('goto-user-location');

The action is actually passed to the activate callback (as the first parameter I think), so you shouldn't have to do this lookup.

Created attachment 275003 [details] [review]
Disable user location button if findLocation fails

Review of attachment 275003 [details] [review]:

ACK

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +91,3 @@
+});
+
+    _init: function() {

Here's the discussion behind this: https://bugzilla.gnome.org/show_bug.cgi?id=723996

Could you please not imply that my code isn't "sane" in the future btw? I put some effort into this to try to find a good solution to notifications (and ended up with something surprisingly similar to what gnome-documents does btw). The API isn't perfect but it can be fixed.

Review of attachment 274841 [details] [review]:

::: src/notification.js
@@ +91,3 @@
+});
+
+const Type = {

I don't know why you are taking this personally but I only stated an opinion that seems to be shared by Jonas and apparently you yourself don't think of your solution very highly either. Me pointing out issues in your code should not discourage you in any way but rather make you think if I'm correct and if so, its an opportunity to learn. If I'm wrong, feel free to point out and then its an opportunity for me to learn.

Your efforts are always appreciated. :) Just because gnome-documents does something, doesn't make it a good solution btw.

Review of attachment 274992 [details] [review]:

Read the previous reviews for discussion but in short, we want each non-trivial class to be in its own file. IIRC we discussed this coding-style long time ago and I thought you agreed.

Review of attachment 274992 [details] [review]:

ack

Review of attachment 274995 [details] [review]:

::: src/geoclue.js
@@ +98,3 @@
+        if (!this._clientProxy) {
+            let msg = _("Failed to connect to Geoclue location service");
+            Application.notificationManager.showMessage(msg);

Jonas, I think we shouldn't be showing the message at all here but rather only when we fail to connect in _init.

Review of attachment 275003 [details] [review]:

I don't think we should wait for user to click on the button to tell user that the button doesn't actually do anything. Rather we should disable the button as soon as we realize geoclue is not present. I'm not even sure if we should show the notification to user as this is most likely an installation/setup or packaging issue and enduser should not be bothered about it. a warning on console should suffice I think. Wouldn't you agree?

Attachment 274951 [details] pushed as a9ab836 - notification: Remove unused Type const
Attachment 274975 [details] pushed as 210cb8c - notificationManager: Simplify showNotification
Attachment 274992 [details] pushed as f2376ac - Split out notification manager to its own module

Created attachment 275960 [details] [review]
geoclue: Handle service not available

The dbus code can fail if geoclue is not present on the system. Move
code that can fail to a try/catch block to handle geoclue not
available. Also make sure that methods using Geoclue are aware that
it might not be there.

Created attachment 275961 [details] [review]
Do not enable location button when no geoclue

Wait for geoclue module to signal connection before enabling
the goto-user-location button.

Review of attachment 275960 [details] [review]:

ack

Review of attachment 275961 [details] [review]:

::: src/mainWindow.js
@@ +136,3 @@
+            let action = this.window.lookup_action('goto-user-location');
+            action.enabled = true;
+        }).bind(this));

property binding instead of signal would be better approach here? That way, we can also later make it easily so that action is disabled if geoclue dies or something?

Created attachment 276042 [details] [review]
geoclue: Add connected property

Turn geoclue into a GObject and add a connected property that tells
us if we are connected to the GeoClue2 DBus service.

Created attachment 276043 [details] [review]
mainWindow: Disable location button if no geoclue

Created attachment 276044 [details] [review]
geoclue: Add connected property

Turn geoclue into a GObject and add a connected property that tells
us if we are connected to the GeoClue2 DBus service.

Review of attachment 276044 [details] [review]:

ACK. Micro nitpick: connected -> 'connected' in shortlog.

Review of attachment 276043 [details] [review]:

::: src/mainWindow.js
@@ +137,3 @@
+        this.mapView.geoclue.bind_property('connected',
+                                           action, 'enabled',
+                                           GObject.BindingFlags.SYNC_CREATE);

Would have been nicer if geoclue itself handled all this but not a lot of code so its fine here too I guess.

Attachment 275960 [details] pushed as ad686e2 - geoclue: Handle service not available
Attachment 276043 [details] pushed as d5cb892 - mainWindow: Disable location button if no geoclue