DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.22k stars 40 forks source link

Use UI file for Install Dialog #212

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

Overview

This PR converts the install dialog to use a Qt Designer .ui file. This is more consistent with the other dialogs that ProtonUp-Qt uses, and allows for some cleanup of the install dialog class.

This also has an indirect benefit: it works around a PySide6 bug with Plasma Wayland, where dialogs on scaled displays don't render correctly until moved to a non-scaled display and back again.

image

I attempted to keep the UI as close as possible to the existing install dialog but I did have to increase the size of the dialog just a small amount. On main, the dialog and its elements extend it to a size of 314x279. However in Qt Designer, with the way it aligns elements, this meant that everything was slightly off-center; elements on the right had slightly more spacing than those on the left.

To allow for even spacing, the dialog dimensions were increased to 320x280, as round figures that would divide evenly. I felt that a width of 310 was a bit too narrow and I felt giving the description box more room rather than less was a better compromise, so I rounded that size upward.

I hope the size change isn't too noticeable. I thought increasing the size was the better tradeoff than having the UI be slightly off-center. I'll provide a screenshot of main to compare with.

CtInstall dialog on Main ![image](https://user-images.githubusercontent.com/7917345/226702288-7330408d-5b52-4869-80f4-26812e238bb8.png)

Another change I made was to disable the install dialog comboboxes and install button if no tags were found for the given tool, i.e. if GitHub is down, or the user is rate-limited.

I tried my best to rate-limit myself to get a screenshot of this, but of course I can't do it when I need to showcase something :sweat_smile: So please be sure to test this as well. I implemented this by changing the logic to not emitting is_fetching_releases(False) if the tags list is empty. This is a little bit hacky I suppose but I guess it also still follows logically, as if the user is rate limited or if GitHub is down, it never really fetches the releases.


There were also some minor code cleanups done along the way that I noticed, like removing some unused variables, and replacing for loops to add items with addItems and a list comprehension to help streamline the code a little more. The logic for loading, setting up and showing the UI for the installer dialog was also moved into its __init__ instead of in the pupgui2 file where its called, which is again more consistent with other parts of the code.

I felt this would be a good change for consistency. It could be done for other dialogs to but the CtInstaller dialog was the last "major" one as far as I could see that didn't use a UI file.

Thanks :-)

DavidoTek commented 1 year ago

This also has an indirect benefit: it works around a PySide6 bug with Plasma Wayland, where dialogs on scaled displays don't render correctly until moved to a non-scaled display and back again.

Interesting... why would that be different with ui files?

I attempted to keep the UI as close as possible to the existing install dialog but I did have to increase the size of the dialog just a small amount I hope the size change isn't too noticeable

That should mean that they are now consistent with the other dialogs as they are using the default spacing of Qt Designer :) I don't think it will break anything.

Another change I made was to disable the install dialog comboboxes and install button if no tags were found for the given tool, i.e. if GitHub is down, or the user is rate-limited

Good.

This is a little bit hacky I suppose but I guess it also still follows logically, as if the user is rate limited or if GitHub is down, it never really fetches the releases.

Makes sense. I will do some further testing, but code looks good.


Thanks a lot!

sonic2kk commented 1 year ago

Interesting... why would that be different with ui files?

I was wondering that, too. Maybe with however the QUiLoader works it's able to properly create the window? I'm not sure, though it's probably something that I should get around to reporting to KDE/PySide6. I'm kinda waiting until Plasma 6 before doing that though, as there are a few little graphical problems with Plasma 5 and Qt6 apps on Wayland + scaled displays, which may be fixed when Plasma moves to Qt6 in a few months' time.

I did get a screenshot of how the dialog looks - I put in details tag because this only happens on scaled displays, meaning the screenshot is large! The issue goes away when dragged onto a normal display and back again.

![Screenshot_20230321_211122](https://user-images.githubusercontent.com/7917345/226742056-8e7d67a6-1e91-45e2-b1e2-4633f3c642c4.png) With this PR, the dialog renders normally on the scaled display. ![image](https://user-images.githubusercontent.com/7917345/226743177-6fbf60cc-6791-40b5-9384-3e7f6a7ea756.png) I think I brought it up in another PR but there are a couple of small hiccups with Plasma and Qt6 apps, like using the sort feature of the ProtonUp-Qt Games List will cause a segfault if pressed repeatedly. This doesn't happen when using the X11 backend though (`QT_QPA_PLATFORM=xcb python3 -m pupgui2`), only with Wayland. That's why I call this an "indirect" benefit, as this is really just a Plasma/Qt bug that is fixed by happenstance with this change :-)