Gracefully handle hamster restarts for development #358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hedayat
reviewed
May 1, 2023
This catches all errors in DBUS calls to hamster and passes them to a new reportError method, which logs them and creates a notification to also inform the user.
When either dbus proxy cannot be created (i.e. hamster is not running and cannot be autostarted), report that to the user. This only reports these errors, not other handling is changed (on error, the proxy objects passed will be undefined or null, so further initialization of the extension should already be skipped). Making error handling more robust (and e.g. recovering of hamster is started later) would be nice, but is too complex to handle now.
matthijskooijman
force-pushed
the
handle-hamster-restarts
branch
from
914fcc4 to
0457681
Compare
|
I've force-pushed an update that resolves the remaining comments (one more string is now translatable, replaced "fact" with "activity"). |
hedayat
reviewed
May 1, 2023
Previously, if this happened the extension would be disabled using the `disable()` method. However, this caused an error (because disable was called twice, once for each DBUS service) and gnome-shell would not be told that the extension was disabled, so the user could not re-enable it. With this commit, when the apiProxy disappears, the panel widget is hidden. When it reappears, the panel widget is shown again. Note that there are still some rough edges, but this change is mostly intended to simplify hamster development - you can now restart hamster after making changes (or to switch between installed and source versions) and still have the extension work without needing to restart gnome-shell (which means logging out with wayland). This commit does not attempt to handle windowsProxy disappearing, since keeping track of the status of both services can get messy quickly. Also, now you can restart just the api service, and leave the windows service to be autostarted when needed if you want (and if autostart fails, the user will be shown a proper error message). Also, this does not disable the keybindings, so pressing that while the DBUS proxy is down will probably still allow opening up the menu (but that's ok - making any changes will just autostart hamster again, or show an error message otherwise).
matthijskooijman
force-pushed
the
handle-hamster-restarts
branch
from
0457681 to
6d9807c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the code to more gracefully handle a hamster restart. Previously it would try to disable itself and then run into an error. Now it hides the panel item and shows it again when hamster is started again. The implementation is not perfect (see commit message for some rough edges), but that's ok since users normally have no need (or even a good way) to restart the hamster services, so this change mostly serves to make development easier.
In addition, I've preceded this change with two more commits that improve error handling for dbus calls. This was prompted because one of the rough edges is that you can now technically end up in a situation where the extension is shown and usable, but the windows service is not running, so clicking e.g. "Show Overview" will not work. That will now show an error to the user, which should make this an acceptable corner case. However, showing error messages when a dbus call fails seems an improvement in general too (for example, this now also shows an error for the case in #334 - though the python backtrace is so long the actual error message is cropped, at least it shows something at least - and the log shows the full error).