Created attachment 258161 [details] [review]
data: Support launching via DBus activation

This will be the preferred way of launching applications, so install a
DBus .service file, rename the .desktop file to use reverse DNS notation
and mark it as DBus activatable.
Using reverse DNS notation for the .desktop file is also a prerequisite
for using GLib's new notification API.

Created attachment 258162 [details] [review]
alarm: Add a unique id property to alarms

Notification buttons work differently in the new notification API;
rather than invoking a callback on the notification itself, they
will activate an application action.
This means we will need a way to unambiguously identify a particular
alarm, so add a unique id property to that purpose.

Created attachment 258163 [details] [review]
Port to the new GLib notification API

hi Florian, thanks a lot for the patches. I have not looked at them yet since I am writing from 
my phone.

Note that we already had a branch porting to dbus activation, however in last cycle there were some major blockers to not push this:
- clocks could not be used anymore in jhbuild
- there was no simple way to simply  add a printf and check the output
- there was no simple way to export env vars like GDEBUG and launch the program
These are major problems for any contributor

Do you know if anything changed in these regards?

(In reply to comment #4)
> Note that we already had a branch porting to dbus activation

Ha! Should have checked that :-)


> however in last cycle there were some major blockers to not push this:
> - clocks could not be used anymore in jhbuild
> - there was no simple way to simply  add a printf and check the output
> - there was no simple way to export env vars like GDEBUG and launch the program

I don't understand this - this affects how clocks is launched via its .desktop file, I don't see how it has any affect on running the executable directly (I certainly used "jhbuild run gnome-clocks" and printf and friends without problems while writing the patches)


> Do you know if anything changed in these regards?

I'm afraid I don't. Maybe the problem isn't actually anything of the above, but the patch not adding a .service file (so "dbus-activatable" is kind of a lie)?

Review of attachment 258163 [details] [review]:

Quick round of trivial review

::: src/alarm.vala
@@ -494,0 +490,5 @@
+        var app = GLib.Application.get_default();
+        var action = app.lookup_action ("stop-alarm");
+        ((GLib.SimpleAction)action).activate.connect ((action, param) => {
... 2 more ...

(trivial) I prefer using curly braces also for one line "if" and "foreach" (also in the other hunks of the patch)

::: src/utils.vala
@@ -303,1 +303,1 @@
     public delegate void ActionCallback ();

this can be removed I think.

Review of attachment 258161 [details] [review]:

trivial comment: is the use of org.gnome.Clocks (with capital C) a convention documented somewhere? it looks a bit odd to me, I guess I would prefer using lower case clocks in filenames etc

Created attachment 258170 [details] [review]
Port to the new GLib notification API

Another thing that is not clear to me is how it works for alarms: can I schedule a notification to wake up the app in 12 hours? Or is that still an open question?


I also forgot to mention to feel free to push this as a wip/notification branch if it makes life easier for you.

(In reply to comment #7)
> trivial comment: is the use of org.gnome.Clocks (with capital C) a convention
> documented somewhere?

It's what most GNOME apps use on the bus, but as there are exceptions (evince and gedit come to mind), I wouldn't call it a convention.

Review of attachment 258170 [details] [review]:

::: src/alarm.vala
@@ -558,2 +571,4 @@
     public signal void ring ();
 
+    private Item? find_item (string id) {
+        foreach (var i in alarms)

thanks for the super quick, update, but you missed this part :-)

(In reply to comment #10)
> (In reply to comment #7)
> > trivial comment: is the use of org.gnome.Clocks (with capital C) a convention
> > documented somewhere?
> 
> It's what most GNOME apps use on the bus, but as there are exceptions (evince
> and gedit come to mind), I wouldn't call it a convention.

I see, I do not really mind one way or the other. If I had done the patch I would have probably used the lower case, but as you prefer.

(In reply to comment #5)
> (In reply to comment #4)
> > Note that we already had a branch porting to dbus activation
> 
> Ha! Should have checked that :-)
> 
> 
> > however in last cycle there were some major blockers to not push this:
> > - clocks could not be used anymore in jhbuild
> > - there was no simple way to simply  add a printf and check the output
> > - there was no simple way to export env vars like GDEBUG and launch the program
> 
> I don't understand this - this affects how clocks is launched via its .desktop
> file, I don't see how it has any affect on running the executable directly (I
> certainly used "jhbuild run gnome-clocks" and printf and friends without
> problems while writing the patches)
> 
> 

I guess we should compare your patch with the other branch that was made by desrt and see what changed. Apart from that we can probably ask Ryan

(In reply to comment #9)
> Another thing that is not clear to me is how it works for alarms: can I
> schedule a notification to wake up the app in 12 hours? Or is that still an
> open question?

This is still an open question. For now, notifications are send directly via GApplication, so the app has to be running. Scheduling notifications at a later time is certainly a valid use case, so I expect us to support this eventually (though it might involve politics like systemd-in-user-session ...)

Created attachment 258171 [details] [review]
data: Support launching via DBus activation

Use lowercase.

Created attachment 258172 [details] [review]
Port to the new GLib notification API

Add missing braces.

Thanks for the quick comments! I've also pushed the patches to wip/gnotification now (should make testing easier).

Okay, I tested the branch and it works great. I amended it a little bit and pushed it to master.

The problems mentioned in my comment #4 are only present when splitting the application in a real service binary installed in libexec, like it is done in the wip/appservice branch. We'll cross that bridge when Ryan finishes building it :-)


Thanks a lot for your help!

(In reply to comment #4)
> Note that we already had a branch porting to dbus activation, however in last
> cycle there were some major blockers to not push this:
> - clocks could not be used anymore in jhbuild
> - there was no simple way to simply  add a printf and check the output
> - there was no simple way to export env vars like GDEBUG and launch the program
> These are major problems for any contributor
> 
> Do you know if anything changed in these regards?

Bug 710965 might be a good way forward for you.