Follow-up from "Systemd user units"
The following discussions from !507 (merged) should be addressed:
-
@fmuellner started a discussion: On a high-level, I like this: The behavior of the switches in the prefs-tool is that the
active
property refers to whether the user requested the extension to be enabled, and thestate
one reflects the actual extension state.Except that the former is currently really a "did the user toggle the switch" state, so making it reflect the actual preference makes a lot of sense.
I don't like the name though, as it's easily confused with the enabled state. How about "requested"?
-
@fmuellner started a discussion: This name is misleading. The array contains all extensions that are requested to be enabled by either the session mode or the user. By contrast, the existing array contains all extensions that actually should be enabled (which may also be session mode or user extensions).
-
@fmuellner started a discussion: This is wrong. You are now trying to enable the extension if it's enabled by the setting, but independent from whether it should actually be enabled.
-
@fmuellner started a discussion: I don't like the magic bool - it's completely unobvious what
getEnabledExtensions(true)
andgetEnabledExtensions(false)
do without checking the actual function.Maybe something like this?
_getRequestedExtensions() { rlet extensions = [ ...this._getModeExtensions(), ...global.settings.get_strv(ENABLED_EXTENSIONS_KEY ]; // filter out 'disabled-extensions' which takes precedence let disabledExtensions = global.settings.get_strv(DISABLED_EXTENSIONS_KEY); return extensions.filter(item => !disabledExtensions.includes(item)); } _getEnabledExtensions() { return this._getRequestedExtensions().filter(uuid => this._shouldEnable(uuid)); } _shouldEnable(uuid) { let isMode = this._getModeExtensions().includes(uuid); let modeOnly = global.settings.get_boolean(DISABLE_USER_EXTENSIONS_KEY); return isMode || !modeOnly; }
-
@fmuellner started a discussion: This looks more complicated than necessary.
this._requestedExtensions = this._getRequestedExtensions(); for (let extension of this._extensions.values()) { let requested = this._requestedExtensions.includes(extension.uuid); if (extension.requested == requested) continue; extension.requested = requested; this.emit('extension-state-changed', extension); }
-
@fmuellner started a discussion: This is pointless, the
disable-version-check
doesn't affect the list of extensions that are requested by the mode or user. -
@fmuellner started a discussion: Same here: The session mode only affects whether extensions should actually be enabled, not whether the mode/user requests them
-
@fmuellner started a discussion: (commit message nit)
The commit message is misleading in several ways:
- the change that "allows disabling extensions when globally disabled" is the one that changes the
canChange
property, not this one - that's also not the behavior of the new
canChange
behavior, that's "allow the user to enable or disable any extension regardless of error state or whether user extensions are disabled"
What this commit really does is making the switches'
active
state reflect correctly whether an extension has been requested (in contrast to thestate
which reflects whether it is actually enabled).Also a more logical commit sequence to me would be:
- Add isEnabled property to extensions
- Make active state reflect whether an extension has been requested (i.e. this commit)
- Redefine canChange property (although "Allow disabling extensions when globally disabled" would make a better subject for that commit imho)
- the change that "allows disabling extensions when globally disabled" is the one that changes the
-
@fmuellner started a discussion: Mmh, why not set both
state
andactive
here? -
@fmuellner started a discussion: I don't like this. The name sounds very crystal-bally "this is going to change".
It's also completely unused, so just don't add it at all.
FWIW,
canChange
was never meant to mean "the underlying setting can change", but: "is the user allowed to enable/disable this extension". I don't want to change that meaning (which doesn't mean that we can't change the answer to the "is the user allowed to change this" question). -
@fmuellner started a discussion: I can see the point in allowing users to disable extensions that are in ERROR state or even potentially crashed a previous setting. But this isn't what you are doing here, you are simply allowing to enable or disable any extension regardless of the error state or
disable-user-extensions
key, and I'm less certain about that.On the one hand, it's weird to be able to toggle a switch off, but not on again. On the other hand, a toggle that doesn't have any effect (because the extension won't be enabled no matter how often the user enables the switch) is confusing as well.
Maybe ask the design team for help here?