sailfishos-patches / patchmanager

Patchmanager for SailfishOS
https://openrepos.net/content/patchmanager/patchmanager
Other
21 stars 22 forks source link

Split "Allow incompatible Patches" from "Developer Mode" #333

Closed nephros closed 1 year ago

nephros commented 1 year ago

"Developer Mode" currently disables (or rather relaxes) the version check, but it also does more, such as enabling the message about old patch.json formats in the Patch Details page.

We should split "Version Check" from "Developer Mode", and offer either as a config option.

Originally posted by @nephros in https://github.com/sailfishos-patches/patchmanager/issues/322#issuecomment-1295056631

nephros commented 1 year ago

Well, that was easier than expected.

One thing remains: If users upgrade to this, their "developerMode" (which we present as "Allow incompatible patches" in the UI) setting from an old version will map to "developerMode" in the new version, and "strictCompatability" will default to true (i.e. "Allow incompatible patches" is OFF).

While this is probably unexpected, it's "fail-safe", because the dangerous option falls back to a safe default.

We could rename "developerMode" something else to force both to defaults, and handle a migration for the now split semantics. Or not - is it important?

Olf0 commented 1 year ago

"Developer Mode" currently disables (or rather relaxes) the version check, but it also does more, such as enabling the message about old patch.json formats in the Patch Details page.

We should split "Version Check" from "Developer Mode", and offer either as a config option.

I absolutely concur that the current "Developer Mode" shall be split into two separate user settings, as I already denoted in issue 322.

Well, that was easier than expected.

Indeed, as your three commits show: d2d8387, e712296 and 9fa793e.

I also like the fact, that this splits the issues users have been experiencing for so long with the original "Developer mode" from the developer facing issue #322.

But …

One thing remains: If users upgrade to this, their "developerMode" (which we present as "Allow incompatible patches" in the UI) setting from an old version will map to "developerMode" in the new version, and "strictCompatability" will default to true (i.e. "Allow incompatible patches" is OFF).

IMO this is problematic: Most users utilise the current "Developer mode" to override the strict SFOS version check. Hence the current setting of developerMode shall be translated into a new relaxSfosVersionCheck ("Relax SailfishOS version check" {off|on} at the UI) boolean setting; for a fresh installation of Patchmanager this setting (IMO) should default to "off" (false; i.e., retain the current behaviour). Then a new setting patchDevelMode can be introduced (at the UI: "Mode for Patch developers"), which covers the other effects of the former developerMode.

While this is probably unexpected, it's "fail-safe", because the dangerous option falls back to a safe default.

This is pretty unexpected, even though I like that the fact that your strictCompatibility setting defaults to true. But exactly that has the potential to upset people who currently have the "Developer Mode" switched on to circumvent the "Strict SailfishOS version check".

IMO we should try to make this change unnoticeable ("seamless") for most users. I believe that the best approach is to leave the old developerMode setting as it is and simply copy its (boolean) value to relaxSfosVersionCheck if unset (i.e., "not yet set"). On fresh installations the old developerMode setting does not exist (and nothing may ever set it, any more) and relaxSfosVersionCheck defaults to false if unset.

We could rename "developerMode" something else to force both to defaults, and handle a migration for the now split semantics. Or not - is it important?

As denoted above, I think it is, but rather "handle a migration for the now split semantics" properly for most users and hence (in case of migrating) initialise only the new patchDevelMode with its default (false), than "to force both to defaults".

I know this involves some tedious search and replace, but IMO this is a clean solution to introduce two new settings whose names are not too similar to the old one (also at the UI): This is why I came up with (after some pondering) with the boolean settings relaxSfosVersionCheck (at the UI: "Relax SailfishOS version check") and patchDevelMode (at the UI: "Mode for Patch developers") to supersede the developerMode (at the UI: "Allow incompatible patches" lately, before that "Developer mode"). Simply reusing "Developer mode" at the UI would be evil, because so many forum messages in the past 5 years state "Switch on 'Developer mode' to relax the SFOS version check for Patches". And any term / phrase containing "compatibility [check]" / "[in]compatible" might be understood that more is done than a simple version comparison, which is not the case AFAIK. OTOH, to express the compatibility to certain SFOS release version is what the Patch developer intended, but we might better retain the terms "compatibility [list|check]" and "[in]compatible" (or any setting using these terms) for exactly that: resolving issue #322, i.e., something aimed at Patch developers.

nephros commented 1 year ago

Good points.

As I still want to implement #322 after this, maybe I'll switch the setting to a ComboBox/Enum, something like:

With for now, Strict and Any taking the place of the Boolean Switch, and the other two reserved for the implementation of #322.

Olf0 commented 1 year ago

Good points.

As I still want to implement #322 after this, maybe I'll switch the setting to a ComboBox/Enum, something like:

* Strict (exact version) 
* Relaxed (X.Y.Z)
* Careless (X.Y)
* Any

With for now, Strict and Any taking the place of the Boolean Switch, and the other two reserved for the implementation of #322.

Cool, that would also shorten my suggestion for an appropriate settings name to sfosVersionCheck.

One thing to consider: Relaxed (X.Y.Z) and Careless (X.Y) are identical since SailfishOS 3.3.0 (i.e., on all SFOS releases the current Patchmanager supports; 3.2.0 / 3.2.1 was the last real micro release upgrade, as 4.0.1 was the only 4.0 release), and Jolla does not seem to change that. Plus, sometimes the changes for Lipstick, Silica and Jolla's QML-apps were larger between consecutive micro releases than from the last micro release to ".0" of the next minor release. Thus I would ditch this separation.

Hence I suggest to simplify this scheme and use these three values for sfosVersionCheck:

I like this, because it provides the necessary leeway to relax the default from "strict" to "relaxed", if an implementation which resolves issue #322 is not used by Patch developers despite pushing them to (verbally, only :wink:).