Split "Allow incompatible Patches" from "Developer Mode"

[nephros]

Fixes: #333

[nephros]

Marking as Draft, pending changes requested in #333

[Olf0]

I still have my reservations WRT this "easy implementation", as already denoted.

I feel I must at least insist on creating a seamless migration from the old developerMode setting to the new strictCompatability setting (value needs to be negated, something I tried to avoid in my suggestions) in order to avoid negative feedback from many Patchmanager users, when this change is released.

[nephros]

@Olf0 sorry, I have the bad habit of using fixup and force-push in my "private" forks.
I overwrote one of your commits doing this.

Please hold off any pushes until I take this PR out of Draft mode.

[Olf0]

@Olf0 sorry, I have the bad habit of using fixup and force-push in my "private" forks. I overwrote one of your commits doing this.

That is fine: I have the bad habit of committing trivial changes right away.

I created another private branch and a MR, but …

Please hold off any pushes until I take this PR out of Draft mode.

… failed to update it twice: Will wait and continue to analyse docker@GitHub to achieve caching of the many GBs of docker-image data which are downloaded for each CI run.

[nephros]

Okay, have not tested it yet, but it looks okay. Have at it!

[nephros]

migration snippet needs work.

Dropdown is not set to "allow any".

[Olf0]

migration snippet needs work.

I first hesitated a bit with the migration logic, but I cannot see any better way to handle that (and see that my original ideas were abstractly nice, but not implementable with the extant boolean setting developerMode; i.e., purely academic 😜). Also setting the patchDevelMode to true when the developer mode was set to true is fine, if it really only provides additional debug output, but causes no change in behaviour. Otherwise, I suggest to set the patchDevelMode to false in the migration function (if developer mode was true), because presumably this was an unintended side-effect of the original developerMode from the perspective of most users.

So to me the basic logic of the migration function is looking good.

Dropdown is not set to "allow any".

??? Does this address the FixMe statement in src/qml/SettingsPage.qml, line 115?

Minor linguistic and comments improvements

Aside of pondering about the migration function, I have only four minor linguistic and comments improvements (= commits) to suggest via MR #3.

[Olf0]

IMO this is basically looking fine.

Still I would love to comprehend these two statements, either by being addressed by some commit or a brief explanation. But never mind, I am aware these were just two thoughts being denoted late at night.

migration snippet needs work.

As stated above, IMO its basic logic is O.K. and hard to design differently.

Dropdown is not set to "allow any"

I simply fail to parse this statement, though had an idea what it may address.

[nephros]

Pls do not merge yet.

While the migration logic is correct, the Combonox does not yet work correctly.

1. Its menu has only 2 Entries, while the enum has 4. Therefore when selecting 'No Check' a value of 1 is saved where it should be 3.
2. As the migration logic saves a 3, when loading the combobox it has no value for it, and falls back to -1/undefined, which is equivalent to the first entry in the menu.

I have workarounds for both but must test them first.

[Olf0]

Pls do not merge yet.

I sure won't, it is "your baby" (unless you have ceased to care about it).

While the migration logic is correct, the Combonox does not yet work correctly.

1. Its menu has only 2 Entries, while the enum has 4. Therefore when selecting 'No Check' a value of 1 is saved where it should be 3.
2. As the migration logic saves a 3, when loading the combobox it has no value for it, and falls back to -1/undefined, which is equivalent to the first entry in the menu.

Thanks for the details.

I have workarounds for both but must test them first.

I will happily review them, when they are ready.

[nephros]

Please see 4a96399.

I decided to do the simple solution, have no gaps in the value list.

I know this results in a slightly weird order of the menu options (Strict - None - Relaxed), if I had to argue this I'd say, 0 is always the default, and the others are sorted in order of decreasing strictness.

Anyway, doing it this way means we need no workarounds, and also no workarounds/migrations needed when new options are added.

I also have removed the planned "Careless" option for now - partly because you pointed out that in current SFOSes, two options would be superfluous, and partly because fine-grainedness of a "relaxed" option can be tuned somewhere else (i.e. an extra tunable) if necessary.

[Olf0]

This looks fine to me. So I guess you are all set, now.

[nephros]

And like a true evil American Senator, I sneak in an almost unrelated UI fix in 1dc424b as well.

[Olf0]

                            ▲
Oh, good catch.  │

[nephros]

@olf you asked to not squash your commits in nephros#2 and nephros#3, do you want to keep them when merging this as well?

If yes, I'd clean up and rebase this branch first. If not I´d just squash-merge this as usual.

[Olf0]

@olf you asked to not squash your commits in nephros#2 and nephros#3, do you want to keep them when merging this as well?

Well, that is not really worth the effort, hence …

If yes, I'd clean up and rebase this branch first. If not I´d just squash-merge this as usual.

… "just squash-merge this as usual".