About Dialog: Refactor

[sonic2kk]

This PR refactors the about dialog to be a little bit more concise. There is a significant change to the version check which we may need to discuss/revert, but I put it up like this as if it does work, it would be a lot simpler and more readable than what was there before :-)

The main idea behind this refactor is to make the code a bit more concise and easier to work in, as we may be changing this soon to accommodate other UI elements and related logic, or doing a significant change to turn it into a settings dialog. The idea is to just make this a little bit nicer to work in and """more consistent""" with the other UI classes, consistency as in using lambdas for .clicked.connect etc to replace class methods which are only used for this action.

Overview

This PR refactors

• Move themes to be in the constants file, might be useful if we start adding more themes like the Steam theme (#309).
• Use lambdas instead of class methods for Qt event bindings.
• Heavily simplify the version check, comparing the strings instead of trying to calculate the version difference.

The first two changes, I think, are fairly inconsequential overall and just make things a bit cleaner. But the change to the version check is the major part here that I think may need discussion.

Refactoring the Version Check

The existing version check is like this:

(10000 * int(v_current[0]) + 100 * int(v_current[1]) + int(v_current[2])) < (10000 * int(v_newest[0]) + 100 * int(v_newest[1]) + int(v_newest[2])

This breaks apart each part of the version string for the current version and the version on GitHub taken from the API, and performs some calculation to compare them. I'm not sure what this exactly does or why exactly it's needed, but Python allows you to compare strings. To do that we would assume the version string format is the same, i.e. X.Y.Z, which is an assumption we already make in tag_name_to_version. In Python we can compare strings like 2.8.1 < 2.8.2. I'm pretty sure this just compares the string as unicode(?), but for a version check, I think this is fine.

If we didn't want to compare on strings, we could remove the . from the strings and cast them to integers, something like int(v_current.replace('.', '')) < int(v_newest.replace('.', '')).

The existing version check is probably much more "strict", but I'm not sure if it's strictly needed compared to this check. I tried to build an AppImage of my branch to test, but I struggled to use appimage-builder, so I was unable to test in-context. But the actual version comparison worked in some test scripts and in testing with just variables in a Python shell.

I'm not saying this way is better, it likely isn't "better" per-se, but it may be simpler overall and may work just as well for this purpose. At least, I don't yet see a reason why it wouldn't, but would be happy to be told otherwise and to revert it :-)

---

These changes, currently, are fairly... insignificant for the moment I suppose? But my hope is that they make this part of the code easier to work with in future if and when there is a go-forward for working in here.

Thanks :-)

[DavidoTek]

Thanks!

Move themes to be in the constants file, might be useful if we start adding more themes like the Steam theme

Good idea.

Use lambdas instead of class methods for Qt event bindings.

Yeah, I guess that makes it a bit cleaner

Refactoring the Version Check In Python we can compare strings like 2.8.1 < 2.8.2

I agree that the currently implementation is a bit sketchy and it should be cleaned up a bit :smile: You're right, the string comparison should work if each part only contins one digit, but consider the following example:

>>> '2.8.2' < '2.8.3'
True
>>> '2.8.4' < '2.8.3'
False
>>> '2.8.10' < '2.8.3'
True

I don't know whether there ever will be a version with two digits, but I wanted to keep the options (that's why the number increments by a factor of 100 in the "sketchy" code). I wonder if we could use something like else. I found this https://stackoverflow.com/questions/11887762/how-do-i-compare-version-numbers-in-python and this https://www.geeksforgeeks.org/compare-two-version-numbers/, but I don't feel like adding another dependency for it. We could add the second example in a util function maybe.

[sonic2kk]

Ah, very good catch with the double digit version points. I agree with keeping the options open. My hope is/was to make the version check "simpler" without sacrificing too much functionality.

I wonder if just casting to an integer and removing the decimal points would work. i.e. int(v_current.replace('.','')) < int(v_newest.replace('.',''))

That would turn the comparison in the last example into 2810 < 283. We could even make this a util function which can take two versions and do that string stripping for us. e.g:

def is_update_available(current: str, newest: str) -> bool:
    """ Check if a version formatted as 'X.Y.Z' is the newest available. """

    return int(current.replace('.','')) < int(newest.replace('.',''))

# Usage
if is_update_available(v_current, v_latest):
    # ...

[DavidoTek]

My hope is/was to make the version check "simpler" without sacrificing too much functionality.

Yes, I agree that it would be nice to clean that.

I wonder if just casting to an integer and removing the decimal points would work. i.e. int(v_current.replace('.','')) < int(v_newest.replace('.','')) 2810 < 283

That sadly also won't work in all cases. See following example:

>>> # 2.8.10 < 2.8.3 -> Wrong
>>> 2810 < 283
False
>>> # 1.8.10 < 2.83 -> Correct
>>> 1810 < 283
False

We definitively need to do either one of the following: a) Compare each section of the version individually b) Add leading zeros (in our case one should be enough, I don't think well get 2.8.100 :smile: )

---

I played a bit around and found out you can just compare arrays in Python.

>>> c = lambda a,b: tuple(map(int, a.split('.'))) < tuple(map(int, b.split('.'))))
>>> c('2.8.10', '2.8.10')  # 2.8.10 < 2.8.10 -> Wrong
False
>>> c('2.8.10', '2.8.3')  # 2.8.10 < 2.8.3 -> Wrong
False
>>> c('2.8.3', '2.8.10')  # 2.8.3 < 2.8.10 -> Correct
True
>>> c('1.8.10', '2.8.3')  # 1.8.10 < 2.8.3 -> Correct
True
>>> c('1.80', '2.8.10')  # 1.80 < 2.8.10 -> Correct
True
>>> c('2.8.10', '2.8.10.1')  # 2.8.10 < 2.8.10.1 -> True
True

I also tested other versions, that seems to work great. (I think [2, 8, 10] < [2, 8, 10, 1] is interpreted like [2, 8, 10, 0] < [2, 8, 10, 1])

[sonic2kk]

I love this solution! It's super clean and readable, I think this is the one we should go with.

I did try to find out how exactly Python does the list comparison just to make sure we won't run into any potential pitfalls, but I can't find it out, and also in my tests I'm not seeing any cases where this wouldn't work for us.

[sonic2kk]

Created a lambda is_update_available to do the check as you described with b73c6e2. I figured having a function for this was more readable (since we can say if self.is_update_available) and since it was a one-liner check, a lambda made sense to me.

It could be a util function in future but I'm not sure that we'll ever use this kind of version check so generally. If we implemented some kind of update check it would work differently (probably parsing the version for a given tool using regex and comparing it, not with a general function like this which is specifically for ProtonUp-Qt's semantic versioning).

[DavidoTek]

I made two small changes:

• Removed the leading v from the version string. Otherwise the map inside is_update_available won't work.
• Reverted the string format argument names to be compatibile with the existing translations.
• EDIT: APP_THEMES.index(config_theme()) will cause a crash if the theme name does not exist in APP_THEMES. (figured that out when switching to the Steam Theme branch and editing the config)

Thanks, will be merged now.