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.16k stars 39 forks source link

CtInfo: Add Button to Mark Compat Tool as Global #336

Open sonic2kk opened 6 months ago

sonic2kk commented 6 months ago

Should fully round off #254 (except for replacing unused/global with a "badge" which is a general and more involved change not specific to global ctool alone).

Marked this PR as draft while I work out how to write to the config.vdf file, and also figure out how to update the Tool List on the Main Window. I think a signal is needed similar to what we do for batch_update_complete.

Overview

This PR adds a button on the CtInfo dialog that allows for marking the current compatibility tool as the Global one. This is only shown for the Steam Launcher, when the current compatibility tool is not already global, and only when the current ctool type is CUSTOM so that we don't allow setting an incompatible compatibility tool type as the default (i.e. Proton EasyAntiCheat Runtime).

Below are some screenshots showing the different behaviours:

image image image image

We may want to make some UI tweaks to the button, I'm not super sold on the button text or positioning, but no good alternatives have come to mind as of yet. I put it beside Batch Update to keep it further out of the way to avoid accidental clicks. Batch Update is a reasonably deliberate action, but it could be easy to accidentally click this button if it was beside what might be more commonly used buttons such as the Close button, or Search/Refresh.

Considerations

We also may want to add a warning dialog, to help in the case of accidental clicks. There is no way to revert this action from ProtonUp-Qt, the only way to undo it is to go into the Steam Client.

I'm not sure what happens if a user tries to change this while the Steam Client is running. I suspect it does the same thing as updating other CompatToolMapping values, and any values updated outside of Steam are overwritten when the Client closes. But there also isn't a great way for us to show the warning on the CtInfo dialog either. We could disable the button when Steam is running, but it might not be obvious that it's disabled because the Steam Client is running.

I think we have two options to deal with this:

  1. Show a dialog if the user tries to do this when the Steam Client is running. If we're going to show one to confirm the action of marking the tool as global anyway, we could probably just add another dialog to say "Close the Steam Client before updating the compatibility tool."
    • The problem is that we'd have two different paths to manage here, which could add a bit of complexity.
  2. Add this warning message to the warning dialog before marking a tool as global, either conditionally if the Steam Client is running or just always show it. If we always display it then it would be a very simple solution, but would not match the other warnings that only show up when the Steam Client is open, and a user may mistakenly think that their changes will not apply. Only showing the warning conditionally would mean the user only sees it when relevant.
    • The problem with this approach is that it's possible a user will miss it. If we always display this warning message too, it would be easy for a user's eyes to gloss over it. Conditionally disabling the button to allow the user to proceed would also be a bit complex, like if we were to prevent them from applying the changes when the Steam Client is running, we'd have to make a path to check for that.

Complexity isn't a dealbreaker or anything per se, I'm just looking for the best approach, and those are my thoughts so far.

Remaining Work

I still need to wire up the functionality, updating the global ctool VDF dictionary should be working, but it isn't writing it out yet. But I have some implementation thoughts that I wanted feedback on, so I figured this was in a reasonable state to open as draft :-)

sonic2kk commented 6 months ago

Having taken some more time to think on it, I'm really not sure about that position for the "Set as Global" button. To be honest, I'm envisioning something more like this (mockup made in GIMP, not code):

edit mockup

I feel like having the bottom row clearer again looks a bit better. Then again, having a button like that really sticks out along the top, moreso than I would like. It might look better as a square utility button like we have for refresh and search, but it would still stand out.

Putting it as a square util button alongside the search/refresh buttons may look good as well but introduces the problem I mentioned of it being accidentally clicked. Perhaps a square button beside Batch Update could look nice? (made in Qt Designer)

image

But this ends up looking a bit out of place without "Batch Update" beside it I think.

image

Using the text button looks more natural in cases where we keep this button along the bottom, and where Batch Update is missing.

And, having it as a utility button means we'd need to find an icon for it, and I don't know if there's a good "universal" icon like we have for refresh/search. We could always add a tooltip of course to go along with this, but a lot of users may miss this. It isn't super obvious in my mind, and ProtonUp-Qt's UI is in my opinion very good at predictable behaviour (I like that the buttons say "Add" and "Remove" instead of being + and -, etc).

Maybe for the form layout we'd have to add a 3rd column for the 1st row and put the button there, but no idea how feasible that is...

Any thoughts about the UI portion of this, I'd really appreciate! Actually editing the VDF file should be straightforward, my main concerns with this PR are to do with making the UI part actually "fit" well enough :-)

sonic2kk commented 5 months ago

For health reasons I will be taking a break from working on public projects for a while. I won't be around to continue this work for a bit, likely a couple of weeks at least.

If someone else wants to pick this up in the meantime please go ahead, either by salvaging this or superseding it with another PR. :slightly_smiling_face:

sonic2kk commented 5 months ago

I took another quick look at this branch this evening and I'm not happy with where I've placed the button. I tried to find some way to fanagle the button placement into where I showed in the screenshots, but I wasn't able to. Probably the only way to do that would be with some kind of Grid layout -- Qt has a Grid layout but I'm not sure it could be used in this way? I wasn't able to find his to set widget positions from Qt Designer but maybe I'm just blind πŸ™ƒ

Even so with the above it might alter the uniform look of the UI. Since a button is taller it might make all the cells on a given row taller.

Having some kind of Horizontal layout that contains the FormLayout and then inserting a Vertical layout inside it was another approach I tried, but it still didn't look right... If there was some way to get this to fit the design I had in mind, we could expand it to add a "Copy to Clipboard" button for the compat tool path.


As I'm kinda stumped right now on the UI portion, I think I'll take a quick look at the rest of the implementation again and mark it out of draft, and we can discuss how to implement this on the UI side. At worst, we could add another button along with the refresh and search buttons, but I'm conscious of accidental presses for such an action. I guess a warning dialog is a possible solution...

This also has conflicts so I'll do a rebase before marking out of draft as well πŸ™‚

sonic2kk commented 5 months ago

That rebase was scarier than it needed to be, it had conflicts across commits which was a new experience for me, but it seems to have went smoothly!

During that experience I had an idea that if we wanted to put the global compatibility tool as global option up with the search/refresh buttons, we could do it like this:

I moved and altered the button (but didn't commit the changes), here's what it might look like:

image

It is selected by default, but we should be able to fix that.

Even if it might ultimately be a little futile, we should help the user to be careful, at least as much as is reasonably possible without being intrusive. This is a "destructive" operation in the sense that there is no way to revert this change, the user has to know what tool they had selected as global before and then manually mark it again, or select it from the Steam Client if their previous selection was not a custom compatibility tool.

sonic2kk commented 5 months ago

Ah, it seems I left the writing to the VDF file commented out. I even left a comment saying I was unsure if it would work, but it does in fact work. I threw caution to the and tested and it correctly marked GE-Proton8-28 as global with this PR. It also correctly updates on the Main Menu that the tool is global!

I pushed the uncommented line in b2ded73.

sonic2kk commented 2 months ago

Sorry, I totally missed the replies here... I'll take a look into it now.

sonic2kk commented 2 months ago

Addressed most of the feedback. Once done, the UI may still need a bit of consideration. I'm not overly happy with the current placement. The more I look at my mockup with the star util button the more I like that one, but I also recognise the shortcomings that it is not totally visually obvious and may be easy to press.

Another possibility may be to hide the "Set as Global" button when the Search bar is present. This would mean that a user couldn't search for a game, keep that list on screen, and then press "Mark as Global". The user would have to close the search bar before they can press the button, if the button is kept in its current position. It may not be clear to the user where the button went, or it may get reported as a bug that the button vanishes if it is not intuitive behaviour.

Basically, the actual updating of the global compat tool works, but I'm still not entirely sure on the UIX for it. Even all these months later :sweat_smile:


Regardless of the approach to the UI though, I think one thing I would like to include (either here or in a follow-up PR soon after and definitely before this gets into a release) is a confirmation dialog for the user to let them know that this will mark the tool as global for all compatibility tools. We should also note that for the change to take effect, the Steam Client must be closed beforehand (since this modifies config.vdf which gets overwritten when Steam is closed).

The wording on this dialog should be somewhat dynamic, since this will be used for Lutris as well (hopefully in the near-ish future, a couple months ago I was fiddling around with some global-tool related stuff for Lutris). The warning about closing Steam, for example, shouldn't be shown. We can probably just set custom messages based on install_loc.get('launcher').

We should be able to add the dialog reasonably easily I think, we should be able to use show_msgbox_question the same way we do for SteamTinkerLaunch (or maybe there's another approach we have that I'm forgetting, it's been a while...)

Maybe we would also want to add a "Don't show me this again" type checkbox on the dialog (unticked by default)? Not sure if that's worth it or if it could cause more problems for the UX.


So three things remain for this PR so far:

  1. Address feedback around splitting out some of the VDF file writing into a separate function with update_vdf_file as a separate steamutil function.
  2. Discuss the current UI and how it could be improved, or if it is fine to leave it the way it is.
  3. Add a confirmation dialog before writing any changes to config.vdf.
sonic2kk commented 2 months ago

I had some more thoughts on breaking the VDF stuff into a separate function, and to cut down on reviewing too much of a refactor, I would like to leave it for a separate PR if that's okay. What you said about giving the variables better names than c and d is something that could be tackled in a separate PR as well, a general refactor of how we do our VDF interactions and how best to cut down some of the duplication.

DavidoTek commented 2 months ago

Address feedback around splitting out some of the VDF file writing into a separate function with update_vdf_file as a separate steamutil function. I had some more thoughts on breaking the VDF stuff into a separate function, and to cut down on reviewing too much of a refactor, I would like to leave it for a separate PR if that's okay. What you said about giving the variables better names than c and d is something that could be tackled in a separate PR as well, a general refactor of how we do our VDF interactions and how best to cut down some of the duplication. Having this separated out might also be good for unit testing, hint-hint-wink-wink πŸ˜„

Okay, that's fine for a separate PR.

Discuss the current UI and how it could be improved, or if it is fine to leave it the way it is. Another possibility may be to hide the "Set as Global" button when the Search bar is present

Seems fine to me as it is. I guess hiding the "Batch update" buttons makes sense at it is applied to all games shown. The mark global button is independent of that. What we could do is to place if more to the left so it does not jump to the right when the search bar is displayed.

Add a confirmation dialog before writing any changes to config.vdf. Regardless of the approach to the UI though, I think one thing I would like to include (either here or in a follow-up PR soon after and definitely before this gets into a release) is a confirmation dialog for the user to let them know that this will mark the tool as global for all compatibility tools.

We can also move that to a follow-up PR.

we should be able to use show_msgbox_question the same way we do for SteamTinkerLaunch (or maybe there's another approach we have that I'm forgetting, it's been a while...)

I think we can just use the QMessageBox here. The reason we don't use it for the compatibility tools is that they are running on a separate thread from the main ui.


Basically, the actual updating of the global compat tool works, but I'm still not entirely sure on the UIX for it. Even all these months later πŸ˜…

Should/can we merge it for now though?

TODOs for future PRs would be:

sonic2kk commented 2 months ago

Makes sense to me to keep all those for future PRs!

Should/can we merge it for now though?

Not just yet, in testing I had hoped the answer would be yes, but there are a couple of things I want to address:

  1. The binding for btnMarkGlobal is incorrect as it's inside the block that also checks for 'Proton' in self.ctool.displayname, when it should really be bound if self.is_mark_global_available. I have addressed this locally, not committed yet.
  2. "Set as Global" is highlighted by default, meaning if you open the dialog and press a button like Space or Enter, the "destructive" action of setting the global compat tool will be performed. Either "Close" or something else should be highlighted by default (or better yet, no button hightlighted by default, but I think I looked into this a while ago and ran into trouble... wonder if it's feasible to simply give focus to the list if present, and in cases where it isn't present, focus the close button).
  3. Luxtorpeda for some reason saves as %name% when set as the global compat tool and I am not sure why. All other compat tools that I have tested work fine (Proton-tkg, GE-Proton8-32, Steam-Play-None, SteamTinkerLaunch also correctly sets as Proton-stl).

There is another UX issue I found but that would probably depend on what direction we go with for #350. After marking a compat tool as global, the list doesn't update. It should go from whatever state it was in (showing a games list, or displaying "No games") to displaying "Tool is Global." The list on the main menu will update to say a tool is "(global)" though.

I'm unsure if the UI should update or if we should close the games list, similar to how to go with #350.

sonic2kk commented 2 months ago

The Luxtorpeda name issue appears to be an upstream bug that was already fixed a few months ago, my bad: (luxtorpeda-dev/luxtorpeda#271). Updating has fixed the issue.

When Luxtorpeda is built it looks like it sets the name at build time in the compatibilitytool.vdf, based on its compatibilitytool.template file and substituting the %name% and %display_name% values with those defined in post_build.rs (as seen in the linked PR diff).

The name was actually being set as %name% in the compatibilitytool.vdf file, so ProtonUp-Qt was fetching it properly.

So the Luxtorpeda issue is solved, and I pushed a fix for the incorrect binding logic in 6fd4d26.

The remaining issue I want to tackle in this PR before merging is the focusing issue.

DavidoTek commented 2 months ago

Not just yet, in testing I had hoped the answer would be yes, but there are a couple of things I want to address:

Okay.

I'm unsure if the UI should update or if we should close the games list, similar to how to go with #350.

Does it actually change anything about the game list inside the ct info dialog? I think it only shows the games which have the tool specifically selected and not all games which use it because it is a global tool, or am I confusing something?

sonic2kk commented 2 months ago

When you set the tool as global, currently nothing changes in the dialog. If you close and re-open the dialog, it shows the "Tool is Global" message (although I'm considering a way to add a button to toggle this, thinking ahead to Lutris global tools because of how Lutris manages tools it can be useful to see the games anyway).

It may be unintuitive to have a games list displayed after marking as global, and then when you open the dialog again it'll show the "Tool is Global" label. Switching to this label immediately after may be more intuitive is what I'm thinking :-)

After that I think this is in a mergable state if you're still happy with the placement of the button to mark a tool as global πŸ˜„

Tiagoquix commented 1 month ago

Hi, many thanks for this PR!

I think it would be cool to have the "Set as Global" button in the main screen because the global version is shown there with "(global)".

sonic2kk commented 1 month ago

A user would have to select a tool to mark it as global, and we'd have to do some button logic similar to what we do for "remove selected". I'm not sure where the button would go on the Main Menu either. This is also not something to have "prominently displayed" I don't think.

The tool is displayed as global on the Main Window tool list the same way "unused" and the version of tools are shown. A global indicator is also displayed on the compat tool info dialog.

There's no reason from the code side why this couldn't go on the Main Window though, it's just a case of moving the button, I'm just personally not in favour of having it there πŸ™‚


Perhaps as a "compromise" we could have a context menu. I've been thinking about this for a while and probably mentioned it in an issue somewhere here before, that having a context menu here for right clicking on tools and being able to remove/see the info dialog/and here mark a tool as global.

The reason I put forward against this and the reason it probably won't be added is that ProtonUp-Qt generally has a pattern of things being "visible", that is things are controlled by buttons and displayed visually rather than in a context menu. There is also no precedent for context menus elsewhere apart from built-in ones from Qt for things like search boxes.

But maybe having a context menu where a user can mark a tool as global on the Games List, maybe alongside a keyboard shortcut after the warning dialog is introduced, we could have this option to mark as global in both places.

The question then gets raised about whether it's a good idea to have this option in two places πŸ˜…


That's just my opinion. I'll leave it up to DavidoTek to decide if it should be moved or placed elsewhere.

I'm already on the fence about it's current position on the ctinfo dialog so I'm happy to see other suggestions too. This is something I mull over a lot and ever since opening the PR I'm still conflicted. This is a case where ultimately I think I'd prefer someone make the call for me in the end πŸ˜…

Tiagoquix commented 1 month ago

I agree with your logic of "being visibile", and I don't think a context menu is a good idea.

I still prefer for the button to placed in the main menu, at the right of "Remove selected". But this could cause a problem of people mistakenly setting an anti-cheat as global, so we would need to hide the anti-cheat runtimes from the main window.

sonic2kk commented 1 month ago

We wouldn't need to hide Anti-Cheat runtimes, we can just disable the button to mark as global for those runtimes. We already have logic for this on the ctinfo dialog, there is a function we call to check if marking a given compat tool as global is available for the current compat tool, and we don't enable this for runtimes including the anti-cheat runtimes.

https://github.com/DavidoTek/ProtonUp-Qt/pull/336/files#diff-82e042b373a322747cadb7182df7b36620a7a8c807fb382c41aca09d714250adR897-R915

DavidoTek commented 1 month ago

I think it would be cool to have the "Set as Global" button in the main screen because the global version is shown there with "(global)". Perhaps as a "compromise" we could have a context menu. ProtonUp-Qt generally has a pattern of things being "visible", that is things are controlled by buttons and displayed visually rather than in a context menu. That's just my opinion. I'll leave it up to DavidoTek to decide if it should be moved or placed elsewhere.

I wouldn't want to put it in a context menu as like you said everything should be visible and obvious. I guess one could call it Affordance in UX jargon. That also wouldn't be very touchscreen friendly, i.e. Steam Deck friendly. I'm actually happy with the placement in the CtInfoDialog as I think the main UI shouldn't be too crowded. On the other hand, this functionallity might be used quite a lot if someone changes their global compatibility tool often. Maybe we can add a shortcut like Ctrl + Shift + G for "mark as global" in the main UI?

sonic2kk commented 1 month ago

Sorry, lots of things ate up time and this got pushed out of my mind...

Maybe we can add a https://github.com/DavidoTek/ProtonUp-Qt/discussions/358#discussioncomment-8558019 like Ctrl + Shift + G for "mark as global" in the main UI?

I think a shortcut for this is a good idea. However I want to be mindful of this being done "by accident", so we should make a note to add a warning dialog ASAP. We had discussed this before, where we want to show a dialog warning the user before marking a tool as global, as it is a "destructive" operation in that it is not really possible to just "undo" and a user would have to either mark their previous tool as global again if they remember it, or change it back to a Valve Proton version in the Steam Client. It is not disastrous or anything, but it is something we might want to warn users about. As discussed though that warning can go in a separate PR.

To add the shortcut though we may want to do a little refactor though. The assumptions we made in CtInfo about the available information wouldn't apply on the Main Menu or any other screen that we try to call this on, so we may want to do a refactor to make the logic more easily callable from anywhere.

In the CtInfo dialog we currently use steamutil#set_global_compat_tool when the global button is pressed. But we might want to do this a bit differently. We may want to create a util function that takes in the launcher, and based on the launcher, calls the relevant steamutil or in future lutrisutil/heroicutil(?) function to set the tool

Something like this (untested, just mockup code):

# util.py
def set_launcher_global_tool(install_loc, compat_tool: BasicCompatTool):
    if install_loc.get('launcher').lower() == 'steam':
        # We know Steam will always need 'vdf_dir', and now we let set_launcher_global_tool
        # handle the implementation details on how to set the global tool
        #
        # There's an argument that set_global_compat_tool could just take install_loc and
        # then pull vdf_dir from there too.
        set_global_compat_tool(compat_tool, install_loc.get('vdf_dir'))
    elif install_loc.get('launcher').lower() == 'lutris':
        set_lutris_global_compat_tool(compat_tool)

# pupgui2ctinfodialog.py
def btn_mark_global_clicked(self):
    # We just call set_launcher_global_tool and let it handle the launcher-specific logic
    set_launcher_global_tool(install_loc, self.ctool)
    self.btn_refresh_games_clicked()

# pupgui2.py
def setup_ui(self):
    # ...

    # We set up our shortcut, but we need a separate method
    # in order to cleanly fetch the ctool to pass to global_ctool_shortcut_pressed
    QShortcut(QKeySequence('Ctrl+Shift+G'), self.ui).activated.connect(self.global_ctool_shortcut_pressed))

    # ...

def global_ctool_shortcut_pressed(self):
    # Return early if the ctool list is empty
    # Return early if no item in the ctool list is selected
    # Return early if multiple items are selected (can't set multiple compat tools)
    #    - Or perhaps just choose the first item in the selection?
    # Get the BasicCompatTool object that corresponds to the selected list item (not sure how?)
    # Get the install_loc equivalent on pupgui2 (surely possible but not sure at time of writing)
    #    - Perhaps we should do an earlier check and return if install_loc is invalid? Probably not necessary...
    # Return early if `util#is_mark_global_available` is `False`
    #    - Alternatively, let `util#set_launcher_global_tool` deal with this (we only need to call it in CtInfo to determine whether the button should be enabled or not).
    # 
    # If all of these pass, that means we have a ctool list with an item selected, and that the
    # BasicCompatTool object corresponding to this item passes our `is_mark_global_available`
    # check to determine if it can be marked as a global compat tool.
    # 
    # This means we have determined the selected tool can be marked as global and so we can call
    # `util#set_launcher_global_tool`.
    #
    # May also want to update the Games List so that the menu shows "(global)" beside the tool name.

    pass 

This refactor can be done in this PR, but perhaps the shortcut could go in a follow-up PR because of the implementation details in global_ctool_shortcut_pressed having a few unknowns.


The last remaining issue that I wanted to tackle here was the focusing issue. It's something I remember fighting with a few weeks ago and not getting anywhere at the time, but I will try to look at it again soon (maybe after an iced coffee!). So the focusing issue is probably the last thing to tackle here.


So in terms of work for this PR:

That means in a follow-up PR we will handle the following (partly copypasted from your earlier comment):

sonic2kk commented 1 month ago

I did the refactor described above in 9f7c1c2. I made a couple of other changes, like renaming the steautil function to be more clear that it's for Steam. Since we just import it and use it without qualifying it as steamutil.foo, putting steam in the function name imo makes it clearer that this function is specific to Steam, and will help avoid any potential name clashes :slightly_smiling_face:

I think the new approach is cleaner. We just have one function now that we can call anywhere with a given ctool and install_loc, and it'll be able to set the global compat tool for us.

sonic2kk commented 1 month ago

Fixed the focusing issue by re-arranging the focus order for the CtInfo dialog. This is probably worth visiting in other screens too but I don't think it's a super high priority for now.


So pending some further review I think this PR is complete in my eyes, finally :sweat_smile: Took me way too long.

The warning dialog, Mark Global keyboard shortcut, and VDF logic refactor can go in separate PRs. Implementing this functionality for Lutris should also be possible and can go in a follow-up PR in future too!

DavidoTek commented 2 weeks ago

Thanks! :tada: I tested it and it works great.

sonic2kk commented 2 weeks ago

Based on a comment above, I pushed some changes in 05fb631 and 08849b7 which toggles the visibility of the Mark Global button based on whether set_launcher_global_tool successfully updates the launcher.

What all this means is if the Mark Global button is clicked and the tool is successfully marked as Global, we will hide the button, and the logic to do this is the same as the logic used to do this when the dialog opens.

sonic2kk commented 2 weeks ago

Damn, I just realised that because we're using set_launcher_global_tool to update is_mark_global_available, if the global tool is updated externally from ProtonUp-Qt, the refresh button won't account for this because is_mark_global_available is only set when the dialog opens, and in btn_mark_global_clicked.

This could be fixed by setting self.is_mark_global_available = is_mark_global_available(self.install_loc, self.ctool) in update_game_list_ui I guess...


On this note, I wasn't sure what would happen if we updated config.vdf with, say, GE-Proton9-7 while the CtInfo dialog was open, and then tried to mark that tool as global. Thankfully it seems like it all works fine, so if the tool is updated externally while the CtInfo dialog is open to the same tool we want to mark as global, nothing falls over and the button is still shown/hidden correctly.