Extensions are “rebased” so that patches (usually method overwrites) are always restored in-order.
As a result disabling some extension could cause disable() → enable() to be called for another extension. It would be useful if there was a way of not requiring that an extension stops all its operation when it is “rebased”. Especially, since this consistently happens for the first session-mode change user → unlock-dialog:
As the purpose for the “rebasing” is to avoid conflicts when reverting monkey-patching, I wonder, if it would be permissible to only fully disable if Main.extensionManager.lookup(this.uuid).state === Utils.ExtensionState.DISABLING and otherwise partially disable by only reverting all patches.
This would work since Utils.ExtensionState.DISABLING is only set for the extension which causes the “rebasing”: js/ui/extensionSystem.js · main · GNOME / gnome-shell · GitLab
(Note: Although this could allow extensions to operate more reliably, it would still be possible for an version update to interrupt another extension since _disableAllExtensions is called in _onVersionValidationChanged)
Not all state is suitable to be stored in settings or the filesystem. It would be useful to keep some transient state in order to not interrupt what the extension is doing.
Here is an example:
disable() {
// Revert order-sensitive modifications:
// 1. Revert all patches
this.injectionManager.clear();
this.injectionManager = null;
// 2. Remove inserted widgets
this.myIndicator.destroy();
this.myIndicator = null;
// Only disable the extension fully if it is not just being rebased
const state = Main.extensionManager.lookup(this.uuid).state;
if (state === Utils.ExtensionState.DISABLING) {
// Stop extension operation (e.g., long download, process of a user-defined command) or clear other non-persistent handlers (such as timeouts)
this.myService.destroy();
this.myService = null;
}
}
In my opinion, this is not something extensions should be allowed to do.
For an extension to conditionally respond to its own internal state seems like a hand trying to grab itself. Exceptions to these rules also make it difficult to review, since you have to remember for any given extension that a section of code may be run as a result.
Of the three examples, only the “long download” seems like a good rationale. Processes defined by users should be fine to leave uninterrupted since there’s nothing unexpected about that and timers would be better stored as a timestamp in the future and picked up later.
Either way, it would be far more reasonable to me to have persistent operations registered by extensions and handled by the extension system (i.e. GNOME Shell).
Either way, it would be far more reasonable to me to have persistent operations registered by extensions and handled by the extension system (i.e. GNOME Shell).
“Maybe, it would be a nice feature for gnome-extensions to have a not interrupted dbus-service running (which does not require ‘rebasing’). The dbus service would only be stopped if the extension is disabled by the user.”
Of the three examples, only the “long download” seems like a good rationale. Processes defined by users should be fine to leave uninterrupted […]
The theoretical possibility of handling child-processes without a gjs-handle and storing timestamps for any timeout is different from it practically being worth the cost for extension developers.
For instance, I dropped the “check-command” feature of the ShutdownTimer extension because of this.
Exceptions to these rules also make it difficult to review
While the risk of incorrect rebasing is certainly there, I think, the responsibility of the extension working (i.e., not breaking other extensions) is on the extension developer.
As far as I see, the hidden assumption for rebasing that no monkey-patching occurs during operation (between enable() and disable()) is already being ignored during review.
Ah, I remember we had this conversation previously in the extensions channel. My opinion hasn’t changed, but I’m not the only voice here and I don’t have the final say.
We’ll just have to see what the opinions of others are
As far as I see, the hidden assumption for rebasing that no monkey-patching occurs during operation (between enable() and disable() ) is already being ignored during review.
Not that I know of, as I wouldn’t knowingly publish something broken
being ignored during review
I suggested a more hands-off reviewing approach where ignoring would be a good thing. I’m sorry if this sounded like an accusation.
Honestly, I do not know of an extension that is overriding/patching methods during operation, however, there is no rule stopping extensions.
I heard of an extension (but do not know where) that patches/unpatches a method based on a setting.
Using the InjectionManager’s restoreMethod method in some callback instead of disable is another example.
I just think that it would be fine to ignore this most of the time since two extensions would need to override the same method for things to break. ( I stumbled upon this since I have multiple extensions which override Main.panel.statusArea.quickSettings._addItems and then revert their override as soon as they are done.)
For what it’s worth, that’s not really what I was referring to when I said:
What I meant was, if you have an exception like:
// Only disable the extension fully if it is not just being rebased
const state = Main.extensionManager.lookup(this.uuid).state;
if (state === Utils.ExtensionState.DISABLING) {
// Stop extension operation (e.g., long download, process of a user-defined command) or clear other non-persistent handlers (such as timeouts)
this.myService.destroy();
this.myService = null;
}
The reviewer now has to remember that this.myService.destroy() may not be called every time disable() is called and double-check anything added or removed. In a typical review, you just ensure it cleans up in destroy() anything it does during construction/run-time because you’ve already checked destroy() is always called.
To keep things simple, the reviewer could pretend as if state === Utils.ExtensionState.DISABLING is always true.
To be clear, this is a separate issue (but relevant to review complexity):
If overrides (or override reverts) happen during run-time it is not enough to check everything is destroyed and reverted in disable(). Here is an example which leaves the override Main.example@B after disabling all extensions:
A enables
B enables and overrides Main.example@shell → Main.example@B
During run-time, A overrides Main.example@B → Main.example@A
Disable all extensions
4.1 Due to rebasing, B disables first and reverts Main.example@A → Main.example@shell
4.2 Then, A disables and reverts Main.example@shell → Main.example@B
I do not know if this is considered during review but this is arguably a more complex condition to check.
Thanks for the explanation. I don’t think anyone has considered that case so far (at least not that I’m aware).
I don’t see how this could be addressed any other way than banning optional overrides in the review guidelines. That seems like a good idea to me to avoid potential conflicts, in particular as extensions can easily move the condition check into the override.
Anything that is modified and restored needs to happen in sync with enable() and disable() (e.g., method injections, as widget insertions or modifications) for rebasing to work. However, in my opinion, banning this is similar to banning any modifications/overrides which could conflict with another extension. By the nature of patches, all conflicts can not be avoided. For instance, if extension A removes the system indicator and B does tries to change it, B’s patch will be broken.
Since there are only conflicts if two extensions access the same resource, I think, it only makes sense to proactively avoid conflicts where they are likely to occur.
Here is an example where, in my opinion, it would make sense to put restrictions on overrides and/or help extensions inject without breaking other extensions:
As of GNOME 45 it is hard to add an indicator at a specific location due to the asynchronous startup. On shell startup extensions get enabled before _setupIndicatorsjs/ui/panel.js · 388d2b9faa1bf14dfa42c033b9be2fb7614b8ac6 · GNOME / gnome-shell · GitLab has completed. For instance, if an extensions uses addExternalIndicator while await import('./status/network.js'); has not yet completed then the sibling would be null: this._addItemsBefore(indicator.quickSettingsItems, null, colSpan);.
To be clear, all this is only meant to contrast the current complexity of extension review with the extra cost of reviewing a partial disable.
I reject the extension if I see the optional restore on disable, because that can fall under selective disable which can bypass the GNOME Shell re-basing. (we have a rule for that).
That’s why this line won’t get approved:
// Only disable the extension fully if it is not just being rebased
if (state === Utils.ExtensionState.DISABLING) {
For extensions overriding the same method or property:
IMO, conflict will be inevitable and that issue should be solved between two extensions rather than on review.
I mean adding a rule that all monkey-patching must be done when enable() is called., and undone when disable() is called, and not at any other time. Otherwise we can run into conflicts like the one mentioned above:
extension A monkey-patches a shell method
extension B monkey-patches the same shell method
extension A undoes its monkey-patching at runtime (the method is now the original)
extension B is disabled, undoes its monkey-patching (the method has the modifications from A again)
At that point, both extensions consider the method unmodified, because each one undid its own monkey-patching. Yet in reality, the original method has been replaced irrevocably until the next login.
A requirement to only change monkey-patching on enable/disable would allow extension rebasing to work correctly, and prevent that class of errors.
Extensions can still modify behavior conditionally, they just have to move the condition check into the monkey-patched method:
originalMethod.call(this);
if (!this._settings.get_boolean('fancify'))
return;
// do fancy stuff
Toggles the Background Menu Visibility option off.
backgroundMenuDisable() is getting called (runtime monkey patching).
On disable, backgroundMenuEnable() is getting called.
If we force extensions to apply the logic inside the monkey patched method, that will create many complications for extensions with so many settings (Just Perfection, Dash to Dock, …).
The API I made for Just Perfection extension tries to don’t touch the GNOME Shell code at all when user wants to use the default GNOME Shell setting (for example, that background simply returns).
What do you mean? That case is the whole point why the method was added in the first place. It adds indicator and quick items at the expected positions (before the background-apps item etc.), regardless of whether it’s called before or after built-in items are initialized.
I get the sentiment, but this undermines the whole extension rebasing we do to avoid potential conflicts.
Mind you, if we made “don’t use extensions that modify the same components, or you get to keep the pieces when stuff goes wrong” our policy, we could remove quite a bit of complexity from extension management (tracking extension order, rebasing etc.).
But having the complexity and at the same time encourage patterns that undermine the effort looks like the worst of both worlds (extension conflicts, plus the complexity to avoid them (unsuccessfully)).
This was just meant as another example of how limiting this ban would be. Without being allowed to patch after enable(), extensions are limited to adding their quick settings items before the background-apps item.
How so? I don’t see how that requires monkey-patching in the first place, and even if it does, you could patch the code in enable(), but only add your actors whenever you see fit.
(Nobody is talking about restricting when extensions can modify the UI by adding or removing actors. This is strictly about modifying gnome-shell’s own code)
Nobody is talking about restricting when extensions can modify the UI by adding or removing actors. This is strictly about modifying gnome-shell’s own code.
Adding and removing actors is also order-sensitive and thus correct rebasing requires these changes to be contained in enable() and disable(). However, I see that these kind of conflicts may not seem so critical.
