Fix: Preferences: Incomplete version check and update #9709

[CaptainSifff]

This would close #9709 in my opinion. I have introduced the QVersionString to simplify the final if statement.

[Swiftb0y]

This looks very good, I didn't know QVersionNumber was a thing, thats very useful. @uklotzde is this what you had in mind when filing #9709?

Please also make sure to setup pre-commit to ensure CI passes. See more in our Coding Guidelines.

[Swiftb0y]

Also before we can merge your work, we need your permission to distribute that with Mixxx.
Please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done.

[CaptainSifff]

One thing that just came to my mind, when I accepted your suggestion to use the QVersionString::fromString method:
It's available also in 5.15 but just overloaded for a QString. How far do we want to go back?

[CaptainSifff]

@Swiftb0y I have signed the contributor agreement, thanks for considering!

[Swiftb0y]

It's available also in 5.15 but just overloaded for a QString. How far do we want to go back?

Ah good point, I didn't see that it was available previously. IIRC the minimum version we need to support is Qt 5.12 and the QVersionNumber::fromString QString overload is available there. Since configVersion is a QString, I see no reason not to use that. That would simplify the code a lot.

Since this a bug, it would be appropriate to go into the 2.3 branch. Are you familiar on how to rebase on 2.3?

[daschuer]

Since this is a bugfix, it can go to the 2.3 branch. Please rebase your fix to 2.3.

[CaptainSifff]

It's available also in 5.15 but just overloaded for a QString. How far do we want to go back?

Ah good point, I didn't see that it was available previously. IIRC the minimum version we need to support is Qt 5.12 and the QVersionNumber::fromString QString overload is available there. Since configVersion is a QString, I see no reason not to use that. That would simplify the code a lot.

Since this a bug, it would be appropriate to go into the 2.3 branch. Are you familiar on how to rebase on 2.3?

I checked further and QVersionNumber::fromString with the QString is available since 5.6.0 (https://devdocs.io/qt~5.6/qversionnumber#fromString) which means 2015, hence I will go ahead and simplify that. I will figure out and rebase towards 2.3. Is your release schedule tight here?
One other thing I was wondering is whether I should replace other comparisons in that file also with QVersionNumber. Or do you want me to open a new issue for that?

[daschuer]

Thank you. In general it is better to do small independent pull requests.

[Swiftb0y]

I will figure out and rebase towards 2.3. Is your release schedule tight here?

Not really, not for patch / bugfix releases afaik.

One other thing I was wondering is whether I should replace other comparisons in that file also with QVersionNumber. Or do you want me to open a new issue for that?

I'd also prefer a new PR, especially since the surrounding code does not seem very trivial to understand. Also the scope of that is rather a refactoring instead of bugfix so it should not go into the stable 2.3 branch.

[Swiftb0y]

It seems like there are still more code style issues. Please resolve.

[CaptainSifff]

I have rebased my changes to branch 2.3 . Since github doesn't allow me to modify the source branch I will open a new pull request.

[CaptainSifff]

The new PR for 2.3 is over here: #11229

[daschuer]

Thank you. Next time you may also just push the rebated branch to the original feature branch (not main like here)
GitHub can handle that.

[CaptainSifff]

Closing this as the rebased branch has been merged: #11229