Add support for gnome-shell 43 #59
Open
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
gnome-shell 43 replaces AggregateMenu with QuickSettings: https://gjs.guide/extensions/upgrading/gnome-shell-43.html#quick-settings The current strategy used for modifying brightness indicator behaviour is as follows. When the extension is enabled, the existing brightness indicator is destroyed, and a new one extending imports.ui.status.brightness.Indicator is injected into AggregateMenu, replacing the original one. When the extension is disabled, an "official" brightness indicator object is re-initialized, and injected back to its original position in AggregateMenu. When considering how to implement GNOME 43 support, several considerations went into the decision of moving from the current approach to a "monkey-patching" approach. First of all, the brightness indicator module moves the bulk of its functionality to a private class: https://gitlab.gnome.org/GNOME/gnome-shell/-/commit/7bbd59838a6cfe14b189fa7bd8e743fb0cac9bc3 Strictly speaking, the class is still accessible and could be extended in a way similar to current code, but accessing the class causes a warning to be printed to the console: "The property access will work as previously for the time being, but please fix your code anyway." The brightness indicator code was also changed quite significantly, and a wrapper class is added in gnome-shell 43. That suggests two possible approaches: (1) Keep extending for GS 42-, but copy-and-modify for GS 43+ (2) Copy-and-modify for both, with conditional clauses for 42-/43+ differences Both of these approaches result in quite a lot of gnome-shell code being duplicated. Copying GS 43+ code could also be quite fragile if future releases make any changes to the brightness indicator. As such, a "monkey-patching" approach is used in this commit, minimizing the differences between GS 42-/43+ clauses. The biggest downside to this approach is dealing with some weirdness surrounding the Brightness proxy object and how it calls the _sync function (see comments in code for a full explanation).
|
Thank you for your contribution. I tested this on Fedora 37 with Gnome 43.1 and it works great! |
|
Works perfectly on Gnome 43.2 |
gnome-shell 44 removes the Meta.MonitorManager.get shortcut, and instead standardizes on calling global.backend.get_monitor_manager. https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/2557
Wrap ScreenshotClass definition in a function which accepts softBrightnessExtension as an argument. This allows the caller to pass a reference to the extension class without exposing it as a global. This may not be an ideal solution since it calls GObject.registerClass every time the extension is enabled. On the other hand, I could not find instances of other extensions "unregistering" classes.
This is no longer needed by ScreenshotClass.
According to review for fork to be listed on extensions.gnome.org, the fork's name cannot be the same as that of the original extension. Description should also mention that this is a fork.
In old versions of GNOME, this member does not exist. Directly checking for 'get_monitor_manager' in global.backend will result in complaining about using the `in` operator on an undefined object.
I don't have an easy way of testing these versions with my current x11docker setup. And as far as I know, odd 3.x releases are unstable releases. So let's drop support for these. I'm leaving comments that refer to these versions untouched so that we still know when separate code paths are required, regardless of "official support".
Mostly changes requested by extensions.gnome.org review.
|
working as expected for gnome-shell 43.1 on ubuntu 22.10 |
Appears to no longer be used.
….org Put them back after finishing the review process.
- English title of extension should be "Soft Brightness Plus". - All other instances of extension name in code, namespaces, UUID, should be "soft-brightness-plus".
This reverts commit 7348eb0. Revert while fixing a problem with ScreenshotClass.
…closure" This reverts commit e1af5dd. Wrapping ScreenshotClass's definition in a closure is causing issues when disabling/re-enabling the extension. When re-enabling, GObject.registerClass gets called on ScreenshotClass for a second time, which throws an error. AFAIK there is no way of "unregistering" a class with GObject. Thus classes cannot be registered as part of an extension's enable() function. Other extensions seem to register the class as part of the extension's module code, which only runs once when gnome-shell loads. Possible options: (A) Switch to a delegate pattern [1] (B) Subclass Shell.Screenshot, but keep definition in module namespace Since official documentation talks about migrating from native JavaScript classes (such as those used for a delegate pattern) to GObject classes [2], choose option (B). [1] https://bugzilla.gnome.org/show_bug.cgi?id=688973 [2] https://wiki.gnome.org/Projects/GnomeShell/Extensions/MigratingShellClasses
Set ScreenshotClass.ext when the extension is enabled, and access via this.constructor in ScreenshotClass functions. Bind `this` to ScreenshotClass functions before passing them to `then()`.
Note that the code is very rough. In particular: - Adding Gtk.Labels directly to a PreferenceGroup is not *really* supported as far as I can tell. I might consider just removing the header and footer altogether. - Maintaining state of enums is no longer so simple. A `bind` call is not sufficient since the type of the selected dropdown element is now returned as an integer. - Converting the illegible really-long-dropdown to a triple Gtk.CheckButton PreferenceGroup has resulted in insanely complicated code for maintaining state. This could probably do with some gentle refactoring. - The control for showing built-in monitors... well, it was always complicated. Now it's just slightly worse, due to not being able to use `bind`, and having maintain a separate model object for the strings. On the upside, this solves the problem of the preferences window not being wide enough by default.
The proxy returned by `Utils.newDisplayConfig` is not guaranteed to be initialized: https://gitlab.gnome.org/GNOME/gjs/-/blob/master/modules/core/overrides/Gio.js#L255 Make this explicit by removing the return value from `newDisplayConfig`. Switch the order of initialization to ensure that `this.displayConfigProxy` has a valid initialized proxy before being used in `_bindBuiltinMonitorControl` and `_refreshMonitors`.
Probably doesn't make sense to use this shared util file if we keep modifying it and don't have access to the upstream repo.
Fix up some style issues, including: - Spaces around operators (e.g. +) - Spaces around keywords (for, switch) - Indent switch blocks - No multiple spaces in assignment statements - Use const instead of let whenever possible - Opt for camelCaseVariables - Include braces for one-line conditionals
Major refactor of Extension class.
Make function accessible via a hook injected into OverlayManager.
This function is called when the "use-backlight" setting is changed. It actually throws an error due to accessing this._brightnessIndicator, which was removed in a previous commit. Instead, just point the callback directly to _on_brightness_change.
- Update MouseSpriteContent in src/cursor.js to match
changes in gnome-shell/js/ui/magnifier.js.
- Rename Clutter.Container function calls:
add_actor --> add_child
remove_actor --> remove_child
- Rename Clutter.Container signals:
actor-added --> child-added
actor-removed --> child-removed
- Add version 46 to metadata.json.
Check for `Clutter.Container === undefined` as suggested by gjs.guide.
Support for GNOME 46
In magnifier.js, this function looks like:
/**
* scrollToMousePos:
* Position all zoom regions' ROI relative to the current location of the
* system pointer.
*
* @param {[xMouse: number, yMouse: number] | []} args
*/
scrollToMousePos(...args) {
Although the actor and event arguments that we specify in our extension
are not being used, they should be removed if they are incorrect.
In cases where proxy was not checked to be defined, proxy.Brightness was being accessed, causing a "proxy is undefined" error in some cases.
While waiting for quickSettings._brightness,
Main.panel.statusArea.quickSettings was previously be stored in a
const. However, upon using Alt+F2 and "r" to initiate a restart of
gnome-session, this sequence was observed:
soft-brightness-plus: Brightness slider ready, continue enable procedure
JS ERROR: TypeError: Main.panel.statusArea.quickSettings._brightness is undefined
_enable@file:///home/linolium/.local/share/gnome-shell/extensions/[email protected]/extension.js:348:9
enable/this._enableTimeoutId<@file:///home/linolium/.local/share/gnome-shell/extensions/[email protected]/extension.js:320:22
@resource:///org/gnome/shell/ui/init.js:21:20
I do not believe this actually solves the issue, but could perhaps help
in reasoning about the state of quickSettings across gnome-shell
replacement restarts.
A full solution may involve implementing retries whenever reading
_brightness, or within the first N seconds of enabling the extension.
Adding support for gnome-shell 46 has introduced a new bug: When cursor cloning is enabled, resizing windows causes the cursor to freeze up until ESC pressed, after which clicking on windows to change focus no longer works correctly until the extention is disabled. There did not appear to be an easy "fix" for this, as there aren't any obvious differences in how cursors are dealt with in between gnome-shell 45 and 46. This commit simplifies and refactors mouse cloning, fixing the bug in the process. Note that it does not jive with the magnification tool -- it ends up showing two cursors in slightly different positions. Adjust the warning in the preferences window to read as such, unfortunately breaking translations in po/. See GitHub issue: #20
Fix mouse cloning bug: #20
Updated French translation
In certain cases, this._indicatorManager.getProxy() appears to return undefined. Replace conditional `proxy === null` with `!proxy` to handle this case.
In some cases (perhaps when restarting gnome-shell using Alt+F2 "r"), quickSettings._brightness is set to undefined. This messes up the timer which waits for the _brightness object. Check truthiness instead. Also get rid of a useless `tries >= 0` check, and s/tries/attempt/g.
Also standardize on single spaces between sentences.
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.
gnome-shell 43 replaces AggregateMenu with QuickSettings:
https://gjs.guide/extensions/upgrading/gnome-shell-43.html#quick-settings
The current strategy used for modifying brightness indicator behaviour is as follows. When the extension is enabled, the existing brightness indicator is destroyed, and a new one extending imports.ui.status.brightness.Indicator is injected into AggregateMenu, replacing the original one. When the extension is disabled, an "official" brightness indicator object is re-initialized, and injected back to its original position in AggregateMenu.
When considering how to implement GNOME 43 support, several considerations went into the decision of moving from the current approach to a "monkey-patching" approach.
First of all, the brightness indicator module moves the bulk of its functionality to a private class:
https://gitlab.gnome.org/GNOME/gnome-shell/-/commit/7bbd59838a6cfe14b189fa7bd8e743fb0cac9bc3
Strictly speaking, the class is still accessible and could be extended in a way similar to current code, but accessing the class causes a warning to be printed to the console: "The property access will work as previously for the time being, but please fix your code anyway."
The brightness indicator code was also changed quite significantly, and a wrapper class is added in gnome-shell 43. That suggests two possible approaches:
Both of these approaches result in quite a lot of gnome-shell code being duplicated. Copying GS 43+ code could also be quite fragile if future releases make any changes to the brightness indicator.
As such, a "monkey-patching" approach is used in this commit, minimizing the differences between GS 42-/43+ clauses. The biggest downside to this approach is dealing with some weirdness surrounding the Brightness proxy object and how it calls the _sync function (see comments in code for a full explanation).