-
-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tool List: Mark Tool as Global #314
Conversation
Forgot to mention that this PR can be tested both by setting the global compatibility tool within Steam, or (with Steam closed to prevent any overwrites) by temporarily editing the |
Just noticed the "unused" counter still counts the global tool as unused. The counter says "5" but only 4 are actually unused (GE-Proton8-25 is the global tool but appears to still be counted). I will address this tomorrow :-) I should also just clarify that the info dialog game list is technically empty for this tool since it isn't forced as a compatibility tool in Steam, it's just the default. We could potentially fetch some This list could also be large, in my library this could be close to a thousand games. The overarching installed games list is pretty long but it's a much more general dialog than the compat tool info dialog. I have to wonder just how useful the info list would be at that point for the default tool... Batch update also would not make much sense for the global tool either, maybe in this PR or a follow-up we should disable that if the compat tool is marked as global. Searching is always an option but searching, to me, is useful on this dialog to see which games I have selected a compatibility tool for, and that doesn't really apply to the default tool. I'm not sure, I figured I'd leave all of thet out of this PR and focus on only marking the tool as global without getting too wrapped up in the finer details just yet 😄 There could be an argument to have the global compat tool at the very top of the games list, with a separate more distinct background color and style, to note that it's the global tool. We could give it a separate dialog or modify the info dialog to not show the games list or game count. Essentially treating the tool differently on the UI to make it more visually clear it's the global tool but also changing how a user can and cannot interact with it. |
Thanks! Code looks good at first glance and what you wrote makes sense. Will do an in-depth review of the code later.
Ah, that would be nice 😄
Good idea, that makes it more obvious for people with >10 compatibility tools.
We don't want anyone to break their setup, so a warning + advanced mode seems right.
Probably the ct info dialog is the best place to put it. I think the main ui would get to crowded if we put it there.
I guess that is fine. We could add a text saying "global tool" or something but it should be obvious from the main ui already.
We could leave it as a way to change the global compatibility tool from ProtonUp-Qt. But maybe there is a cleaner way to do this, maybe from the gamelist, idk.
We could hide the game list and maybe rename "batch update" to "change global tool" or something |
pupgui2/datastructures.py
Outdated
@@ -112,17 +112,26 @@ def __init__(self, displayname, install_dir, install_folder, ct_type = CTType.UN | |||
self.install_dir = install_dir | |||
self.install_folder = install_folder | |||
self.ct_type = ct_type | |||
self.is_global = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we should initialize the class attribute here, other attributes are defined and initialized like static variables above init...
I guess your way seems the "more correct" one, although Python has this thing called dataclasses which allow initialization from outside __init__
(we don't use the @dataclass
decorator though)...
Fun fact: There are three (four) ways you can add or initialize attributes for an object
# "correct" one
class A:
def __init__(self):
self.my_attr = 3
# using static variables, which will also init the attributes for object
# I think it works because of Python's dataclasses
class A:
my_attr = 3
# dataclasses with correct decoration that will move initialization to __init__
@dataclass
class A:
my_attr: int = 3
# please don't do it this way
class A:
pass
my_obj = A()
my_obj.my_attr = 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, you're right about how it's initialised, that didn't even cross my mind.
I'm more in favour of following the existing "convention" here (we also do this in other places too, so it's a general convention I mean), so I'll gladly move it outside of __init__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently I forgot to do this, sorry! Should be moved now.
Forgot to reply to this, sorry 😅
I agree.
I like this, I wonder if it should be a separate widget altogether that we hide by default and show when necessary. That way we don't have to do any checks for a click event :-) The UI portion of hiding the games list is something I'm thinking about. I think it's a good idea, but to add to it, it might be nice to replace it with a greyed-out bit of text centered relative to where the games list would be, that indicates that the tool is global. Or, if possible, we could put this in the games list. I'm essentially imagining something similar to what KDE's Dolphin file manager does when a folder is empty, since in this case the games list is also empty. Something like this, although with less empty space since our games list is smaller. The text in our case would say something like "Global Compatibility Tool". We could even extend this idea to unused compatibility tools, displaying "No Games" in a similar fashion. If we choose to put both labels in the list, it would make for a consistent bit of UX: when a list is empty, we display a label, and for the global tool, we display a different label :-) |
On my last point above, on a branch I tested out implementing something like this for "No games", here's how it could look visually. The greyed out text is achieved by "disabling" the label. The exact sizing, font, etc could be figured out later, this is just a visual representation of what we could have for no games and then for the global tool as well. The UX is all wired up here too (refresh will enable/disable elements). If we want something like this I can get a PR up for the branch I have now :-) |
That looks good. I also like the smaller font (18). |
Thanks. Will be merged. Changing the Batch update / Ctinfo dialog is probably something for a later PR.
I think I need to take a look at all those attributes when I find time and maybe revise it everywhere. |
Figured out how to do this for Lutris, still got a few things to iron out though. It stores this information in its cache directory, under Will get a PR up in future for it, no idea when though :-) |
Addresses at least part of #254.
Overview
This PR adds
(global)
beside installed compatibility tools which are set as the global tool in the Steam client's Steam Play settings. When no compatibility tool is forced, but when Steam detects that a game/app is a Windows-only application, this is the default tool that will be used to run that app from the Steam Client. This can be any compatibility tool, including things like GE-Proton. In such a case, even though the compatibility tool is technically not explicitly used, it is still the default and so used by all Windows games which do not have an alternative compatibility tool forced in the Properties dialog.Implementation
This PR accomplishes marking the tool as global, in summary, by doing the following:
is_global
toSteamApp
. This is then toggled manually inpupgui2.py
when fetching the compat tool mapping liststeamutil
function which returns the name of the0
entry in config.vdf'sCompatToolMapping.
It looks inconfig.vdf
, getsCompatToolMapping
, and returns the name field for the0
entry. This should always exist when Steam Play is enabled (required to even use Proton flavours and even show the "Compatibility" section of the Properties dialog), but if it's not, we have fallbacks in place to return an empty string.pupgui2.py
, when we're fetching all of the compatibility tools, if the name of a tool matches the global tool name, we mark it as defaultget_displayname
, which is called to return a string to set as the tool name in the games list, this function checks if the compat toolis_global
and if it is, it sets(global)
. We use an if/elif so that a tool is not marked asglobal
andunused
.Future Work
There are some future things we could do for this functionality, and this PR serves as a baseline for implementing them since it allows us to track which tool is the global one:
config.vdf
underCompatToolMapping
is still the previous tool, which would beGE-Proton8-22
in this case. Checking and unchecking the use of a compatibility tool, or removing this entry altogether fromconfig.vdf
, will resolve the issue.config.vdf
, Steam will fall back to the global compatibility tool and uncheck the checkbox (Steam knows to check/uncheck the checkbox based on whether that app's AppID has an entry underconfig.vdf
'sCompatToolMapping
). But I have no idea what happens if you remove the global compatibility tool if that makes sense. Since the fallback for all other apps is the global (0
) compatibility tool, what happens when that one is missing? Maybe it just turns that setting off?config.vdf
still. Unless Steam has a fallback this will not get updated, which may mean all tools show the same message as when a per-app forced compatibility tool is missing, and a user would have to manually update it0
compat tool'sname
inCompatToolMapping
to the given tool's internal name.I actually wrote most of this back in June when I left my comment on #254, but I guess I abandonded it for some reason. Maybe I wanted to try and accomplish a lot more, like some of the things mentioned in Additional Work, but for now I figured this works and is better than not showing anything. I rebased it on main and polished it up a little before making this PR and made a couple of tiny adjustments, and now it's ready for review :-)
Thanks!