Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nephros opened this issue Jan 6, 2023 · 4 comments · Fixed by #334
Closed

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

nephros opened this issue Jan 6, 2023 · 4 comments · Fixed by #334
Labels
feature request feature requests, suggestions, ideas etc.

Comments

@nephros
Copy link
Contributor

nephros commented Jan 6, 2023

"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 #322 (comment)

@nephros nephros added the feature request feature requests, suggestions, ideas etc. label Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
@nephros
Copy link
Contributor Author

nephros commented Jan 6, 2023

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?

nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
@Olf0
Copy link
Contributor

Olf0 commented Jan 6, 2023

"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
Copy link
Contributor Author

nephros commented Jan 6, 2023

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.

@Olf0
Copy link
Contributor

Olf0 commented Jan 6, 2023

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:

  • strict (exact SFOS release version X.Y.Z.point)
  • relaxed (X.Y.Z must match)
  • careless (any SFOS release version matches)

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 😉).

nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 6, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 7, 2023
nephros pushed a commit to nephros/patchmanager that referenced this issue Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request feature requests, suggestions, ideas etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants