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

Game List Improvements #182

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

This PR fixes a couple of bugs introduced in #180, and adds a small feature that I figured out how to do in the process of fixing those bugs :smile:

No screenshot this time as there isn't really much to "show" :sweat_smile:

The double-click actions were implemented by appending the relevant URLs to a Qt.UserData data entry for each QTableWidgetItem. Initially I thought of having type checks in the item_doubleclick_action method, but this got a bit unwieldy, and it was going to become messy to build things like the ProtonDB URL that way. So instead, we build pass the relevant URLs as data when we create the QTableWidgetItem.

The main fix here is the ProtonDB rating breaking, apologies for missing that initially.

Thanks! :-)

sonic2kk commented 1 year ago

Hang on, the first item in the games list seems to never show the ProtonDB rating. The click event doesn't seem to do anything. I will investigate.

sonic2kk commented 1 year ago

Ah, it seems like it's to do with this logic:

if i := self.games.index(game):
    # ...

When the index is 0, I guess Python sees this as falsy and doesn't run the block? I tested the following code snippet and got the same result:

games = [ 'Cookie Clicker', 'PowerWash Simulator', 'Sonic Mania', 'Fallout: New Vegas' ]

# Won't print anything
if i := games.index('Cookie Clicker'):
    print('First game')

if i := games.index('PowerWash Simulator'):
    print('Second game')

Not sure what the best solution is here yet. Will think about it some more :-)

sonic2kk commented 1 year ago

Pushed a couple of UI enhancements that I originally had on another branch where I was experimenting with some other UI improvements - Most fell through except for the ones I just pushed.

The most recent commit adds the following:

Making the dialog wider and the Game Name column makes it easier to see the game name and gives more padding. It fits most of the titles I have installed except for a small handful which have comically long names.

Before:

image

After:

image


In my humble opinion it looks nicer, and I really wanted to fix that scrollbar issue :sweat_smile:

DavidoTek commented 1 year ago

Critically, fixes the ProtonDB rating text having the wrong flag, meaning it didn't display

Actually I haven't noticed that... :smile:

Adds double-click actions to cells in the Game List, similar to the "Show info" dialog

That's useful, great!

When the index is 0, I guess Python sees this as falsy and doesn't run the block? Not sure what the best solution is here yet. Will think about it some more :-)

Yes. I don't think the := makes a difference and Python just reads it as if 0: ... Simplest solution if can think of is following (maybe there is a more Pythonic way):

i = self.games.index(game)
if i != None:
    print('...')

Makes the Games List dialog wider, from 650px to 800px make the Games List column 300px to allow more room for game titles In my humble opinion it looks nicer, and I really wanted to fix that scrollbar issue :sweat_smile:

Looks much cleaner now without the scrollbar. I'm wondering, does the width impact use on Steam Deck? I don't think it uses Scaling on the desktop, right? Then 800px (<1280px) will be just fine.


Thanks!

sonic2kk commented 1 year ago

Simplest solution if can think of is following (maybe there is a more Pythonic way):

Hmm, I implemented it and it works but I think there is a potential problem here. We already do the None check above, with if not game, but I guess there could be an instance where game is not None, but also is not in the self.games list. This will probably never happen I would assume but still.

Though having said that, I am not sure the i := self.games.index would help in this instance, either

games = [ 'Cookie Clicker', 'PowerWash Simulator', 'Sonic Mania', 'Fallout: New Vegas' ]

# Should print an error
if i := games.index('STEINS;GATE'):
    print('What?')

Given that, the following code might be a solution:

if i := self.games.index(game) != None:

But in this case, I am not sure how this differs from just:

if self.games.index(game) != None:

Since i seems to go completely unused.

This could just be my inexperience showing, so please correct me :smile:

I'm wondering, does the width impact use on Steam Deck? I don't think it uses Scaling on the desktop, right?

Good catch! There is indeed no scaling by default on Steam Deck. A user could enable it... but they could also enable a lot of things, so we may not have very much to worry about :sweat_smile: It should just "catch" on the corners of the screen even with scaling.

I would test this myself, but unfortunately I am not sure how to test on Steam Deck anymore as of SteamOS 3.4, the virtualenv stuff I was using before doesn't seem to work. It definitely used to, because I tested a bunch from source for the STL work in the past.

DavidoTek commented 1 year ago

This will probably never happen I would assume but still.

As we are relying on external data from other apps (Steam client), let's better be safe than sorry :smile:

if i := self.games.index(game) != None:

I think what this does is:

i = (self.games.index(game) != None)
if i:
    ...

Since i seems to go completely unused.

Seems to be a leftover from an older revision of the code: self.ui.tableGames.setCellWidget(i, 4, lbl_protondb_compat)

There is indeed no scaling by default on Steam Deck. A user could enable it... but they could also enable a lot of things, so we may not have very much to worry about sweat_smile

All right, then it shouldn't be a problem. And even with 1.5x scaling it will be less than 1280px.

sonic2kk commented 1 year ago

As we are relying on external data from other apps (Steam client), let's better be safe than sorry :smile:

I agree, it's easy to be burned by black boxes like that.

I think what this does is:

Yup, looks to be correct. We can probably just do self.games.index(game) != None :smile:

Seems to be a leftover from an older revision of the code

Ahh I see, I thought perhaps it might've been but wasn't totally sure.


Pushed a commit that implements the change to the self.games.index check. I did a quick test after changing and nothing seems to have broken :+1: