Restore shutdown #631
Restore shutdown #631
Conversation
|
I haven't looked at details yet, but in general this is highly welcome. I wonder if you've also looked into stopping tracking when the lock screen is activated? Have you seen #568 and the related discussion? |
|
I'm using hamster since a long time and I'm missing this 'stop tracking on shutdown` feature. But I haven't much time to work on this so it takes a quite long time to annoy me enough to take more active actions. I have take a quick look at the 'stop tracking when idle' feature when writing this. It looks like it could be restored too but I haven't planned to do it, at least in a short future. I have just read #568 and a few related discussions. Well, at least there are some life, we will see how it goes. |
|
I've had a look at this PR. I like the idea of restoring this behavior. What confuses me, though, is that one commit talks about shutdown and the other about logout, but in practice I think the feature applies to both (i.e. any clean termination of the gnome session), right? Also, it seems this PR is very Gnome-specific? IMHO it's acceptable to only support a subset of environments (i.e. only Gnome right now maybe), but what happens with the current code on e.g. KDE? Is there (I believe not?) any way for the user to know that the feature is not supported, or is the checkbox in the settings just shown but ineffective? That would not be acceptable IMHO. Also, would the lack of a GnomeSession DBUS service be handled gracefully? I wonder if we cannot detect logout in a desktop-environment-agnostic way? If the hamster daemon is running inside the user session, it will be terminated as part of logout, so couldn't that just be a moment to stop tracking? Or is that hard to distinguish from manually stopping hamster (but does that ever happen)? Alternatively, maybe systemd-logind can help here? That might work across different desktop environments and maybe even allow detecting any secondary sessions for the same user, if we think that's relevant. Though logind might not be enabled on all systems, of course. |
|
Indeed, it stop the tracking on logout. I have kept shutdown because I think it was the name used before this feature has been removed. I have not checked what's happen on KDE, I think it will just do nothing. I think I have checked how this feature has been implemented and just replace it with something similar. I haven't found a general way to achieve that. I think it's fine if we stop the tracking when the user stop the daemon but I don't know if stopping the tracking won't need another user process which can be already stopped. Then using systemd-logind or something else for KDE will probably not work on all systems, so in any case we need to check what's happen when the DBUS service is missing and probably disable this option. |
|
This definitely breaks Hamster on non-Gnome systems. I've tried on XFCE and get this vomit: |
|
Thank you for checking that, I have not much time but I will fix that. |
|
I have caught the exception and added a warning if the GNOME session is not available. The I have added a tool tip on the setting to warn the user that this function is not available outside GNOME. It was pretty straightforward. I have put all this in a third commit. Do you prefer that I merge those changes in the previous two commits? I have kept the name shutdown because it was the old name. Do you prefer that I call that "Stop tracking on logout"? Then for other environments. I'm afraid that stopping the tracking when the hamster daemon is terminated by the log out could lead to nasty bug depending on the order in which other processes are killed. Do you think it can be an issue? Else, we can probably get an end session signal on KDE and perhaps XFCE or by using systemd. Do you have a preference? |
src/hamster/session.py
Outdated
| dbus_interface = "org.gnome.SessionManager.ClientPrivate", | ||
| bus_name = "org.gnome.SessionManager") | ||
| except dbus.exceptions.DBusException: | ||
| logger.warning("Unable to connect to GNOME session manager, stop trcking on logout won't work") |
There was a problem hiding this comment.
Please either make this logger.info() or just replace this line with pass so command line users don't see this message on every single command (because warning is the default logging level). Also trcking -> tracking.
There was a problem hiding this comment.
Ok, I have changed it but I think this message will appear only when your start the hamster daemon so probably not on each command.
There was a problem hiding this comment.
Ah right, I forgot that I was using a cover script that restart the daemon before each command. Thanks anyway.
|
Tried again on XFCE and no longer seems to cause problems. I didn't notice any problem when killing the hamster daemon from the command line or when logging out (i.e. an ongoing activity remains open) . For me it is not a problem to introduce functionality available only on GNOME. |
Maybe this should be a new issue / feature request. Perhaps one could listen for org.freedesktop.login1 PrepareForShutdown and PrepareForSleep signals? |
There was a problem hiding this comment.
I had a look over the code. It looks ok overall, left some inline remarks (mostly questions). I haven't tested the code.
Maybe this should be a new issue / feature request. Perhaps one could listen for org.freedesktop.login1 PrepareForShutdown and PrepareForSleep signals?
Agreed, I do no think this must be solved before merging this, provided that this limitation if sufficiently clear for the user.
On that track, I like mentioning it is GNOME-only in the label more than the tooltip, since people might not be reading the tooltip and then be surprised it doesn't work.
I have kept the name shutdown because it was the old name. Do you prefer that I call that "Stop tracking on logout"?
I personally would find "shutdown" confusing, since that implies a system shutdown to me.
Maybe "Stop tracking when GNOME session ends" could be an alternative, which neatly also includes the GNOME limitation?
Then for other environments. I'm afraid that stopping the tracking when the hamster daemon is terminated by the log out could lead to nasty bug depending on the order in which other processes are killed. Do you think it can be an issue?
What issue are you afraid of exactly? I can imagine that whenever the hamster process is terminated, it could just stop tracking. This probably needs a SIGTERM handler or something like that, though maybe Python has some "on process end" code (or maybe the mainloop already neatly stops running on a SIGTERM and you could just add some code after the mainloop?).
I still think it would be good to consider this alternative, since if it would work, would not only work on non-GNOME systems, but also on GNOME systems, so I expect that the code for this would be simpler and maybe less fragile than listening on the gnome session dbus.
One thing that might prevent this from working properly, though, is that it relies on the hamster daemon being stopped when the login session ends, which might not actually be the case. I.e. there is systemd-logind, which I think supports processes that are tied to a user, not a particular login session (so they are killed only when the last login session of that user is logged out). I'm not sure how that works with dbus-autostart, though (which I think is how hamster is being started now in most cases? Or is it a .desktop file in an autostart directory?).
| """Inform that the session is about to end. It must reply with | ||
| EndSessionResponse within one second telling if it is ok to proceed | ||
| or not and why. If flags is true, the session will end anyway.""" | ||
| self.__session_client_private_iface.EndSessionResponse(True, "") |
There was a problem hiding this comment.
Why is this handler needed? If we're not doing anything other than saying it is ok to end the session, isn't this just the default? Or is it so that if you register something (I'm not sure about right terminology here) against SessionManager, that you need to also handle this QueryEndSession event?
There was a problem hiding this comment.
I have not checked, but according to the comment I have understood that when you register against the SessionManager you have to handle this message.
If it's not done, I think it can delay the log out by 1 seconds, eventually the session manager could remove the corresponding program and we will not get the end session message.
| """Override this method to do something at the end of a session. | ||
| It must not attempt to interact with the user and will be given | ||
| a maximum of ten seconds to perform the actions.""" | ||
| pass |
There was a problem hiding this comment.
Maybe this should raise NotImplementedError?
There was a problem hiding this comment.
I think pass is fine, there is no need to do something.
Eventually, I could extend this object to provide a hook in query_end_session_handler. I will allow to use it to delay the end of a session and it could be that nothing is needed when the session end. Anyway, it's not done currently, so I could raise NotImplementedError if you think it's better.
| @@ -38,7 +40,7 @@ | |||
| quit() | |||
|
|
|||
|
|
|||
| class Storage(db.Storage, dbus.service.Object): | |||
| class Storage(db.Storage, dbus.service.Object, DbusSessionListener): | |||
There was a problem hiding this comment.
I wonder if adding a mixin class here is the cleanest way to implement this. Also, it doesn't seem a responsibility of a class called Storage to implement a "stop-tracking-on-shutdown", but maybe that class already does more than its name implies (haven't looked too closely now).
There was a problem hiding this comment.
I agree with that. It's probably the simplest way but not the cleanest one.
I think I can create a separate object having a reference on the storage object to call StopTracking. To make it even more separated, this object can send a stop tracking message on the dbus, it could be even a separate process.
Should I change the name of the gconf key from
When you log out, all user programs are killed, I think it includes supporting processes used to exchange dbus signal or read gconf key. I'm afraid that those functions are used by hamster and don't work at that time but I haven't checked anything.
I don't think it's a problem is the tracking is not stop in such case. |
Restore the ability to stop the current task on log out.
Add a new class to listen for GNOME session events on DBus.
Restore shutdown check box,