projecthamster / hamster-shell-extension

Shell extension for hamster
http://projecthamster.org
GNU General Public License v3.0
214 stars 91 forks source link

Stop tracking when the screensaver cuts in. #332

Closed mebourne closed 4 years ago

mebourne commented 4 years ago

Listen to the ScreenSaver dbus signal to know when to stop the current activity.

Ideally it would backout the idle time before the screensaver cuts in, but I can't see any way to know if it cut in by idle activity or a key shortcut (in which case there should be no time backed out).

Frankly I was horrified that the most useful feature was removed from core. Without this I might as well track my time in a spreadsheet. After a week of time tracking frustration this is a 40 minute hack to solve the problem.

Take any or nothing of this as you please, I will not be offended. I'm not a js programmer nor ever hacked shell nor dbus before, so the fact that it worked at all was result enough for me. Putting the pull request up so others hopefully benefit. (Yes probably it needs a config option but I'll never turn it off so someone else is welcome to take that over.)

Thanks to all of the devs for hamster and extension so far, 10 years a happy user until the last upgrade!

hedayat commented 4 years ago

Thanks for your interest and contribution to the project. I think this is a needed feature, however I agree that a config option is needed. As you might also track an activity which you don't necessarily use the system during it: e.g. you might be talking about something with a co-worker.

I hope you or someone else goes for it, but personally think that it should not be enabled without any way to disable (I myself won't enable this option most of the time).

matthijskooijman commented 4 years ago

I would also not personally use it, but given others find it invaluable, I'm all in favor of including this as a feature.

As for making this a setting, I'm wondering how to approach that. Originally, this was a hamster setting, but we can't really put this in the main hamster settings window, since that should work even without gnome-shell (or with other dbus clients). So I guess this would end up in the extension configuration screen? But that might be confusing to users? Are there already any extension-specific settings?

A clean solution might be to let the extension register additional settings with hamster to be added to the settings screen, but that will probably also get complicated quickly (and be limited in flexibility, I guess). Or maybe the extension could register just one thing: A way to open the extension settings, so the hamster settings could display a button with "go to extension settings" or something like that?

hedayat commented 4 years ago

I agree with having the setting among main hamster settings. I'm thinking about having no flexibility! Which means that having the setting hard coded into the core, but hidden by default unless enabled by someone externally, which in gnome case will/can be hamster-shell-extension.

In other words, the core can have a screen saver manager plugin.

matthijskooijman commented 4 years ago

I agree with having the setting among main hamster settings. I'm thinking about having no flexibility! Which means that having the setting hard coded into the core, but hidden by default unless enabled by someone externally, which in gnome case will/can be hamster-shell-extension.

Hm, I guess that would work, but scale very badly. If it's just a matter of turning the option on or off, that could work, but as soon as additional configuration is needed, especially if it turns out to be e.g. gnome-specific, then you're stuck again.

Unless we maybe just expose a setIdleState(bool) (or something) API on dbus to be called by any desktop-system-specific code, and then do all the handling (e.g. deciding what to do exactly, if anything, with this info) in the application itself. I do wonder where such an API (and the related daemon) would live, though, since it does not seem storage related, and not GUI/window related either (and those are the two services that run right now, believe).

hedayat commented 4 years ago

Yes, it doesn't scale. It is not flexible. But, IMHO, it is OK for now. When the flexibility or scale was needed, fix it. But I fear that turning a small feature into a big plan would cause it forever to finish... :P

mebourne commented 4 years ago

I'm closing this because unfortunately it is not working. Testing with the key shortcut was OK but with idle timeout it is stopping the activity when you exit the lock screen not when it is engaged. My attempts to fix this have failed, it appears that this code in gnome-shell is not running while the lock screen is active and only gets to run when you exit it, so the messages are queued up.

For what it's worth I consider this a hack in the extension anyway. You shouldn't have to run a gnome-shell extension to get this basic function working. On occasion I've been unable to run the gnome-shell extension, sometimes for years, due to the constant gnome-shell interface breakage. During this time hamster has always been completely usable, even if slightly less convenient, from its own window, and through all of this time the idle timeout function has always worked flawlessly.

I think the discussion above about putting the setting in the main hamster window clearly shows that users expect it to be in in core and not in a gnome-shell extension. It's worked perfectly well there for over a decade until it was unnecessarily deleted.

Nearly all of the Linux desktop envs send screensaver messages over DBUS and hamster already talks DBUS. Even though the message paths vary slightly I really don't see any significant overhead from this being supported in the core. There seems to be a desire to have hamster core isolated from the outside world in its own bubble, but that doesn't seem to be a productive user-centric approach.

I suppose next I'll go hack hamster core to reinstate the feature there, perhaps if I'm lucky I can just revert the commit where it was removed and fix that up. Then I can just carry that locally for as long as needed. Alternatively I could easily hack up a script to hook the dbus signals together in a new process, but it makes no sense to me to have a third process running to provide basic functionality, and the risk that this might not be running.