Improve Version Parsing to handle more separators

[Edgars-Cirulis]

This pull request updates the parse method in the Version class to improve its handling of version strings. The previous implementation used the '.' separator to split the version string into sections, but this approach is not robust as there may be other separators present in version strings. The new implementation uses a lambda function to check for changes in character class (digit or non-digit) or the presence of '-' or '+' characters as separators. This approach is more robust and can handle a wider range of version string formats. This change will make the Version class more reliable and easier to use.

Signed-off-by: Edgars Cīrulis edgarsscirulis@gmail.com

[Ryex]

as for addressing the FIXME...
IMO it should also split every time it changes over between alpha and numeric categories and if it sees - or _

so 22w43a would count as [ "22", "w", "43", "a"]

that would make snapshot versions more order-able
it's not perfect of course but I think it's serviceable.

but
with how you have this written I'm fairly sure 1.5.4 gets split into ["1", ".5", "5", ".4", "4"]
because of the nested capture groups.
not sure that is ideal or intended.

I'm fairly sure regex is just a bad tool for this

I think it would be better to write a simple parser that loops though the string character by character. Each time it encounters one of [\.\-_] or the capture class (\d or [a-zA-Z]) switches it appends the last complete section and starts on a new section.

[Edgars-Cirulis]

as for addressing the FIXME... IMO it should also split every time it changes over between alpha and numeric categories and if it sees - or _

so 22w43a would count as [ "22", "w", "43", "a"]

that would make snapshot versions more order-able it's not perfect of course but I think it's serviceable.

but with how you have this written I'm fairly sure 1.5.4 gets split into ["1", ".5", "5", ".4", "4"] because of the nested capture groups. not sure that is ideal or intended.

I'm fairly sure regex is just a bad tool for this

I think it would be better to write a simple parser that loops though the string character by character. Each time it encounters one of [\.\-_] or the capture class (\d or [a-zA-Z]) switches it appends the last complete section and starts on a new section.

You are correct that the regex approach may not work as intended, especially for version strings that contain both numeric and alphabetic characters. A simple parser that loops through the string character by character, as you suggested, would be a better approach to handle this use case.

This way, the version string "22w43a" would be split into ["22", "w", "43", "a"] as you expected.

[Edgars-Cirulis]

Sorry for the force push. The previous commit was 8011886

[Ryex]

Looking good but it could be clearer. 👍

[flowln]

considering you're already messing with the version decomposition here, you could might as well try following FlexVer decomposition to try avoiding fragmentation in the version comparison format between applications (not that it will solve all the problems, but it's a step in the right direction IMO). It would also give us a few cool test instances we could test the implementation in :-)

[Edgars-Cirulis]

considering you're already messing with the version decomposition here, you could might as well try following FlexVer decomposition to try avoiding fragmentation in the version comparison format between applications (not that it will solve all the problems, but it's a step in the right direction IMO). It would also give us a few cool test instances we could test the implementation in :-)

I have tried FlexVer decomposition and found it to be too messy. Therefore, I have decided to stick with my current implementation as it works. However, in the future, if I decide to revisit this issue, FlexVer decomposition could be a step in the right direction for avoiding fragmentation in the version comparison format between applications.

[Ryex]

considering you're already messing with the version decomposition here, you could might as well try following FlexVer decomposition to try avoiding fragmentation in the version comparison format between applications (not that it will solve all the problems, but it's a step in the right direction IMO). It would also give us a few cool test instances we could test the implementation in :-)

I have tried FlexVer decomposition and found it to be too messy. Therefore, I have decided to stick with my current implementation as it works. However, in the future, if I decide to revisit this issue, FlexVer decomposition could be a step in the right direction for avoiding fragmentation in the version comparison format between applications.

could you elaborate on what you mean by too messy?

as I understand flexver it should be as simple as doing this

    auto classChange = [](QChar lastChar, QChar currentChar) {
        return !lastChar.isNull() && ((!lastChar.isDigit() && currentChar.isDigit()) || (lastChar.isDigit() && !currentChar.isDigit()));
    };

then checking the first character in a section for specials like ., -, _, and +

[Edgars-Cirulis]

Ohh. Well

considering you're already messing with the version decomposition here, you could might as well try following FlexVer decomposition to try avoiding fragmentation in the version comparison format between applications (not that it will solve all the problems, but it's a step in the right direction IMO). It would also give us a few cool test instances we could test the implementation in :-)

I have tried FlexVer decomposition and found it to be too messy. Therefore, I have decided to stick with my current implementation as it works. However, in the future, if I decide to revisit this issue, FlexVer decomposition could be a step in the right direction for avoiding fragmentation in the version comparison format between applications.

could you elaborate on what you mean by too messy?

as I understand flexver it should be as simple as doing this

    auto classChange = [](QChar lastChar, QChar currentChar) {
        return !lastChar.isNull() && ((!lastChar.isDigit() && currentChar.isDigit()) || (lastChar.isDigit() && !currentChar.isDigit()));
    };

then checking the first character in a section for specials like ., -, _, and +

I attempted to implement the FlexVer structure for decomposition, but found that it required significant adjustments to other components such as flameapi, among others. which I could be done later.

[Edgars-Cirulis]

@Ryex Unfortunately, your implementation 3bec4a8 did not work as I had already anticipated. Removing the commit 'your implementation' passes all tests with all possible Minecraft versions.
Additionally, by removing those who failed, it passed with 0.04 seconds, whereas my implementation passed with 0.02 seconds.

[Ryex]

@Edgars-Cirulis, the failure cases are from comparisons between incomplete versions. this is because we are not following the flexver comparison function correctly. In flexver the null padding components are always compared as less than.

I think we want the additional rule that null and 0 are equal so that "1.2" and "1.2.0" are equal

https://github.com/Ryex/PrismLauncher/commits/flexver_version has the two commits needed to fix this.
feel free to merge or cherry pick from it.

EDIT: I may have forgotten to include the QDebug header because it mysteriously works without it under Qt6

[Edgars-Cirulis]

I'm assuming it's done my own tests passed successfully. Now need review.

[flowln]

@Edgars-Cirulis I went ahead and changed the comparison functions to match FlexVer too, so we can get the test vectors working too. In the meanwhile I also found a slight bug with your implementation based on the test cases. Please see if you're fine with my changes, and feel free to make any other ones

@Ryex I've changed that behavior you've described of 1.5 == 1.5.0, explained a bit of my reasoning in the last commit's message :-)

[Ryex]

@flowln Looks good, I figured that we didn't want to potentially anything by breaking from the current treatment of 1.2 vs 1.2.0 but I suppose it is and edge case and if it becomes a problem we can always address it elsewhere.

New test's pass for me

[Edgars-Cirulis]

Looks good to me too! 👍🏼