Skip to content
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

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

sonic2kk
Copy link
Contributor

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.

image

Implementation

This PR accomplishes marking the tool as global, in summary, by doing the following:

  • Adding a property is_global to SteamApp. This is then toggled manually in pupgui2.py when fetching the compat tool mapping list
  • We update this property using a new steamutil function which returns the name of the 0 entry in config.vdf's CompatToolMapping. It looks in config.vdf, gets CompatToolMapping, and returns the name field for the 0 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.
  • In 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 default
  • In get_displayname, which is called to return a string to set as the tool name in the games list, this function checks if the compat tool is_global and if it is, it sets (global). We use an if/elif so that a tool is not marked as global and unused.

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:

  • Use badges instead of text, as it may still not be clear which tool is the global one
  • Move the global compatibility tool to the top of the list, to make it visually clearer
  • Show a warning dialog when trying to remove the global compatibility tool, like we do when trying to remove a used compatibility tool
    • I'm not sure how Steam handles a missing global compatibility tool and if it tries to fall back, but if you force something like GE-Proton8-22 for a single game in the Properties dialogue for that game, and then remove it, Steam will get confused and will not display the "Play" button, and there will be text beside it saying "Available for [Windows icon, and maybe a macOS icon]".
    • This is because the tracked compatibility tool in config.vdf under CompatToolMapping is still the previous tool, which would be GE-Proton8-22 in this case. Checking and unchecking the use of a compatibility tool, or removing this entry altogether from config.vdf, will resolve the issue.
    • When removing the entry in 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 under config.vdf's CompatToolMapping). 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?
    • And so to go full circle, I Have no idea what happens if the global compatibility tool itself is missing, but marked in the 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 it
    • All this to say, at the very least a warning is probably necessary when removing the global tool, if not outright disabling the remove button (maybe we could only enable it in Advanced Mode?).
  • Add an option to mark a given tool as the global compatibility tool. Steam would need to be closed for this action. This was also discussed in Tool marked as unused even if it is the "global" compatibility tool. #254, but there was not a solid consensus on where to put it. Implementation shouldn't be that tricky since, at its core, it should just be setting the 0 compat tool's name in CompatToolMapping to the given tool's internal name.
  • Potentially more I'm not thinking of 😄

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!

@sonic2kk
Copy link
Contributor Author

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 0 entry under CompatToolMapping section to use a different name. This is how I did most of my testing. For at least GE-Proton versions since the new naming scheme (though possibly further back as well) the internal name is the same as the actual name, so it can simply be set to something like GE-Proton8-25.

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Nov 24, 2023

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 os_from and os_to information from somewhere about which tool is using the global compatibility tool, or rather probably just infer it from any Windows game that runs on Linux which doesn't have a compatibility tool forced by the user or selected by Valve. But I'm not sure how to do this, and computing this might have a performance cost depending o where we source the information from.

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.

@DavidoTek
Copy link
Owner

Thanks!

Code looks good at first glance and what you wrote makes sense. Will do an in-depth review of the code later.


Use badges instead of text, as it may still not be clear which tool is the global one

Ah, that would be nice 😄

Move the global compatibility tool to the top of the list, to make it visually clearer

Good idea, that makes it more obvious for people with >10 compatibility tools.

All this to say, at the very least a warning is probably necessary when removing the global tool, if not outright disabling the remove button (maybe we could only enable it in Advanced Mode?).

We don't want anyone to break their setup, so a warning + advanced mode seems right.

Add an option to mark a given tool as the global compatibility tool. Steam would need to be closed for this action. [...] but there was not a solid consensus on where to put it

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 should also just clarify that the info dialog game list is technically empty for this tool

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.

maybe in this PR or a follow-up we should disable that if the compat tool is marked as global.

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 give it a separate dialog or modify the info dialog to not show the games list or game count

We could hide the game list and maybe rename "batch update" to "change global tool" or something

@@ -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
Copy link
Owner

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

Copy link
Contributor Author

@sonic2kk sonic2kk Nov 25, 2023

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__.

Copy link
Contributor Author

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.

@sonic2kk
Copy link
Contributor Author

Forgot to reply to this, sorry 😅

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 agree.

We could hide the game list and maybe rename "batch update" to "change global tool" or something

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.

image

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 :-)

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Nov 25, 2023

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.

image

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 :-)

I actually tested with a smaller font, the above was `26` but I lowered it to `18` and think it looks better. I tested out `16` and thought it was a little too small.

image

@DavidoTek
Copy link
Owner

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
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).
If you already have it, feel free do to a PR :)

@DavidoTek
Copy link
Owner

Thanks. Will be merged.

Changing the Batch update / Ctinfo dialog is probably something for a later PR.

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.

I think I need to take a look at all those attributes when I find time and maybe revise it everywhere.
Python is a bit strange regarding instance attributes, for example, you also get this: https://stackoverflow.com/questions/472000/usage-of-slots
😆

@sonic2kk
Copy link
Contributor Author

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 versions.json.

Will get a PR up in future for it, no idea when though :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants