Refactor extension to use an inhibitor manager

[stuarthayhurst]

This is a fairly extensive rewrite of extension.js, and I'm not proud of putting it in 1 commit, but absolutely nothing would work in the slightest until everything was moved over to an inhibitor manager, as I suggested in #285.

The current solution seems to create an inhibitor for every excuse to inhibit, which has lead to the extension losing track of its inhibitors in the past, and suffered from race conditions too. The new solution abstracts the inhibiting away to a new class, InhibitorManager. It connects to signals and calls _updateState(), which uses _getInhibitReasons() to scan every excuse to inhibit (user toggle, apps, fullscreen). It then compares these against the user's settings, and decides whether to inhibit or not, and updates the inhibitor and night light accordingly. This means we only every have 1 inhibitor, so we can't easily lose track of it. It also makes the app handling simpler, as we just add methods to handle the chosen app trigger, and return whichever app is responsible for triggering Caffeine.

This means adding new ways to enable and disable Caffeine just means making _getInhibitReasons() aware of them, making issues like #217, #307 and #330 much easier to approach.

All in all, this slims down the codebase, slims down the DBus interfaces and self-contains a lot of code. Currently it has some issues with overriding the triggers via user choice, and some features need more testing, but any feedback would be appreciated :)

Closes #332

[stuarthayhurst]

Whoops, the version change from 53 -> 54 slipped in, I can remove it if you guys like since it's unrelated, or leave it because it syncs us to the latest released version.

[stuarthayhurst]

Finished my testing, nearly everything works, except the night light control is completely unresponsive and the extension quickly re-enables itself if you disable it with a trigger active, but I still need to compare that to the old behaviour.

EDIT: Fixed the manual disable

[stuarthayhurst]

Fixed the night light, any issues with this are new to me

[stuarthayhurst]

Also, this removes the toggle-state settings key to control its state, since it didn't really fit into the new structure. That does mean we lose CLI control, but I'm not sure people actually used this as there are other desktop apps that would be better suited to it, and it kind of seemed like a side-effect of the old design.

[stuarthayhurst]

Apparently this closes #332 as well

[stuarthayhurst]

@pakaoraki @eonpatapon GNOME 47.alpha is out, I'll start work on porting soon, but ideally this would be merged by then. Any chance of a quick review?

Thanks :)

[pakaoraki]

Sorry for the late answer. I think it's a really nice improvement, I like this new approach. For now, this version is not working on my computer (Ubuntu 24/Fedora 40, Gnome 46): I have this error message "TypeError: app is null". I will test/debug it soon.

[stuarthayhurst]

Do you have your app trigger set to active workspace only? I think I missed a check there.

EDIT: If that was it, it should be fixed now

[pakaoraki]

I ran this version for the last few days.
I find some issues:

• Apps trigger not working: it create multiple inhibitors again. I reckon, you removed some mechanism I have previously added. For some reasons, apps events are not clean and can be erratic, calling multiple time _updateState() function in a row (for few millisecond). The concept of having one inhibitor flag to manage is really great but I don't think it can handle all the race condition problem due to this events.

• There is a minor problem with the subtitle label when a timer have been selected: when caffeine is off, the duration is displayed as subtitle: for some reason here it's not displayed immediately, you need to switch workspace (or sometimes close and reopen the toggle menu) to refresh the label.

Also, if it's possible, it could be nice to keep the toggle-state settings key, but from what I saw, it's seams a bit tricky to add it.

[pakaoraki]

I'm really concern about this because it's calling this._updateState() all the time for any apps events. I strongly suggest to filter this events as it have been to remove all unwanted calls.

• First, I think it would be better to create/destroy the signal only when it need to be:
  -app-state-changed when ON_RUNNING or ON_ACTIVE_WORKSPACE are enable.
  -notify::focus-window when ON_FOCUS is enable.
  -workspace-switched when ON_ACTIVE_WORKSPACE is enable.

• Maybe use a dedicated function to call _updateState() and block/unblock before/after for 200ms the signal to avoid multiple unwanted calls using block_signal_handler.

• Maybe filter app id as well before to limit _updateState() call.

[stuarthayhurst]

Implemented relevant signals suggestion. I'm not convinced on the signal blocker, since if we have an event happen and then revert in a short space of time, the revert would be missed and it would be stuck in the old state. I think that's just going to cause bugs down the line.

Filtering the app id feels a little redundant, as it would be trying to do part of _updateState()'s job for it, I think it's better to leave it as a sort of sync method, that just takes in the environment around it when given a nudge.

I'm happy that this point is resolved, if you're not just let me know :)

[stuarthayhurst]

• Apps trigger not working: it create multiple inhibitors again. I reckon, you removed some mechanism I have previously added. For some reasons, apps events are not clean and can be erratic, calling multiple time _updateState() function in a row (for few millisecond). The concept of having one inhibitor flag to manage is really great but I don't think it can handle all the race condition problem due to this events.

I ran into this during development but thought I fixed this, I'll take another look, thanks

• There is a minor problem with the subtitle label when a timer have been selected: when caffeine is off, the duration is displayed as subtitle: for some reason here it's not displayed immediately, you need to switch workspace (or sometimes close and reopen the toggle menu) to refresh the label.

I've got a fix for that, thanks

Also, if it's possible, it could be nice to keep the toggle-state settings key, but from what I saw, it's seams a bit tricky to add it.

It shouldn't be too hard to implement it back, I dropped it since it seemed like an artefact of the previous design and less of an intended feature, it allowed slimming down the gsettings schema and it was one less entry point to deal with. But if people miss it it can be added back, I'll tackle that after the new system works properly.

[stuarthayhurst]

Since JavaScript is single-threaded, we can just test if we're waiting to create an inhibitor, as well as if we've already created one. If we need to delete and inhibitor that's waiting to be created, we can check for this in the callback to DBus, and just destroy it straight away.

This hasn't broken anything on my system, but I also don't know if it fixed the issue, since I couldn't reproduce it.

[stuarthayhurst]

Hopefully that should be any actual bugs fixed. The trigger signals could be improved to connect based off of what it actually needs, and I'll look for an elegant way to bring back toggle-state.

EDIT: It only connects to the relevant signals now

[stuarthayhurst]

Additionally, this fixes a bug that moving an app between workspaces would sometimes be missed depending on which app was focused. I don't think that had been reported yet.