CtInfo: Add Button to Mark Compat Tool as Global

[sonic2kk]

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:

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]

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

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)

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

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]

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

[sonic2kk]

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]

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:

• Make it a button with an icon, like the existing buttons on that row, and perhaps use a "star" icon of some kind? I think that might convey the behaviour. We will also note in the tooltip that this button marks a tool as global.
• When the button is clicked, we can show a confirmation dialog. This would give a bit more information. We might want to make this dialog flexible so we can use different strings for different launchers (I've mostly implemented internally tracking the global compatibility tool for Lutris, pending some cleanup before PR).
  • Optionally we could have a "Don't show this again" for users who do this often, although I can't imagine this gets done often.

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

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]

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]

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

[sonic2kk]

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 😅

---

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]

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]

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:

• VDF Stuff
  • "breaking the VDF stuff into a separate function"
  • "giving the variables better names"
  • "a general refactor of how we do our VDF interactions and how best to cut down some of the duplication."
• Maybe improve UI by moving the button to the left
• Add a confirmation dialog

[sonic2kk]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

Maybe we can add a #358 (comment) 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:

• Address the focusing issue on CtInfo.
• Refactor the code a bit to make setting the global compat tool a bit more flexible.
  • Use a util method to call the appropriate launcher-specific logic to set a global compat tool.
  • This util function just takes the compat tool and current launcher.
  • Right now in CtInfo we specifically call only the steamutil function for it. In order to add further launchers in future we would need an if/else check in CtInfo, and we would need to duplicate this later on for the shortcut logic.
  • To fix that duplication issue we can abstract the details of setting the global compat tool for a given launcher away into a util function that does that check for us, regardless of what screen we want to set the global compat tool on.

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

• VDF Stuff
  • "breaking the VDF stuff into a separate function"
  • "giving the variables better names"
  • "a general refactor of how we do our VDF interactions and how best to cut down some of the duplication."
• Maybe improve UI by moving the button to the left
• Add a confirmation dialog
• Shortcut for setting global tool on Main Menu

[sonic2kk]

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 🙂

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]

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 😅 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]

Thanks! 🎉
I tested it and it works great.

[DavidoTek]

I wonder if we should disable the button after clicking because there currently is no visual feedback of the event.

That could be as simple as self.btnMarkGlobal.setEnabled(false). We would need to add another check in the constructor so that the user won't click the button if it is already global.

[sonic2kk]

Oh that's a good idea!

There's no reason to click this button again after marking a tool as global, at least not in the context of the currently open dialog.

An edge case could be opening two ctinfo dialogs at once, say GE-Proton9-7 and Luxtorpeda (just to have distinct examples). This is extremely unlikely to ever happen and could be alleviated with other behaviour, it's not a reason against implementing this button disabling but an outline of a potential problem scenario (although again, I really don't think anyone would do this if they weren't trying to break something)

1. User opens both dialogs at once.
2. User marks GE-Proton9-7 as Global.
3. With this suggestion, the button is disabled.
4. User then marks Luxtorpeda as Global without closing the prior dialog.
5. Perhaps a user did this in error and now they want GE-Proton9-7 to be the global tool again.
6. But since the global button is disabled on the GE-Proton9-7 dialog, they cannot.
7. Re-opening the dialog would fix this but it may not be very visually obvious.

A big way to alleviate this being a problem is the confirmation dialog noted earlier, because it would vastly reduce the likelihood of accidentally setting a tool as global.

There's also the possibility of closing the dialog which could be seen as expected since it's on the bottom row of the dialog buttons, but perhaps also not.

---

Disabling the button makes sense to me, I'll add it when I get home. I'm not sure if we want to close dialogs after a tool is marked as global, it might be a little jarring.

I also had a thought, I don't remember what happens if a tool is marked as global and then the game list is refreshed. If we get the "tool is global" text like if the user closed and re-opened the dialog, maybe the button also gets hidden, which I think is good too.

[sonic2kk]

Oh, actually it seems like we don't hide the Mark Global button when the games list is refreshed.

We check if we should display the Mark Global button when loading the ctinfo dialog with self.ui.btnMarkGlobal.setVisible(self.is_mark_global_available), perhaps we should also call this in update_game_list_ui?

I'm thinking we should rework the logic like this:

1. Remove the initial btnMarkGlobal.setVisible call from setup_ui.
2. setup_ui calls update_game_list (updates launcher-specific UI elements) which in turn calls update_game_list_ui (handles generic non-launcher-specific UI elements)
3. In update_game_list_ui we can call check if the Mark Global button should be displayed using self.ui.btnMarkGlobal.setVisible(self.is_mark_global_available) (since we may want to display this for other launchers, and is_mark_global_available will handle checking if Mark Global is available for the current launcher anyway)
   a. Right now the logic is only set if the current launcher is Steam, but with the recent refactor, we can call this generically and call this regardless of our current launcher, because is_mark_global_available is responsible for checking if the current launcher supports this action
4. We also move the self.ui.btnMarkGlobal.clicked.connect action to connect the button logic from update_game_list to update_game_list_ui
   a. Similar to above, this is also nested in a Steam-specific launcher check, but calling it from update_game_list_ui means we can connect this action regardless of launcher. Then the only thing we need to care about is if is_mark_global_available is True or False, and we don't need to care about knowing whether or not the launcher should support it, that's taken care of by the call to is_mark_global_available.
5. Update set_launcher_global_tool to return a success boolean
6. Change the call to set_launcher_global_tool to actually update is_mark_global_available, like this: self.is_mark_global_available = set_launcher_global_tool(self.install_loc, self.ctool). We could I guess hardcode self.is_mark_global_available = False but that would assume set_launcher_global_tool will always succeed. If set_launcher_global_tool fails, this approach means we will update the value correctly.
   a. We could simply leave set_launcher_global_tool unchanged and instead make another call like this: self.is_mark_global_available = is_mark_global_available(self.install_loc, self.ctool) to achieve the same thing. The approach here is more stylistic preference, although I think personally making set_launcher_global_tool return a success boolean is a bit cleaner.
7. This would mean we wouldn't need to disable the button. Instead, we will update is_mark_global_available and our existing call to btn_refresh_games_clicked will handle running all the checks again for whether or not the Mark Global button should be displayed, using the same refactored logic described above that we would use when the dialog opens.

Basically we move the logic to set whether the button is visible or not into update_game_list_ui, meaning we will update it regardless of launcher, because is_mark_global_available performs a launcher check for us. Then when we mark a tool as global, we update the value of self.is_mark_global_available. Finally our existing call to refresh the game list elements means we will pick up any changes to is_mark_global_available and show/hide the button.

The reason showing and hiding may be better is because we show/hide the button when the dialog opens.

[sonic2kk]

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.

• set_launcher_global_tool now returns a bool; False if all the checks fail or if the function ends without hitting a launcher-specific block, OR the value of whether set_steam_global_compat_tool succeeded (as it can fall into an except block and fail to write to the VDF). This means set_launcher_global_tool will only return True if it was able to write out an updated global tool value to a given config file; right now that's only Steam's config.vdf.
• When we click on the Mark Global button, we update self.is_mark_global_available to be the not set_launcher_global_tool. This is because if set_launcher_global_tool succeeds, then the current tool was marked as global and so is not available to be marked as global.
• Now that we update is_mark_global_available when the button is clicked, we can move our logic to show/hide the Mark Global button into update_game_list_ui. which is called as a result of our button click calling btn_refresh_games_clicked. This means we'll run through the same logic used when the dialog opens to determine if the Mark Global button should be shown/hidden, and we use the updated value of is_mark_global_available to do it.

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]

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.

[sonic2kk]

The reference to self.ctool is updated when the refresh button is clicked. This means self.ctool.is_global is still False. So when the tool is marked as global, or when the refresh button is generally clicked (i.e. if the global Steam compat tool was updated externally while the CtInfo dialog was open, and the refresh button was pressed after that) the dialog does not updated to reflect that the tool is global.

So if GE-Proton9-7's CtInfo dialog is open, and the global compat tool in config.vdf is updated to be GE-Proton9-7, pressing the refresh button will not show/hide the Mark Global button, and it will not update the game list to display "Tool is Global".

However, it does update the compat tool list on the Main Menu to display (global) because the compat tool name!

And re-opening the dialog of course works as expected.

---

I'm not sure how to fix this one, it is a bit of an edge-case and at worst something we can update in a later PR.

[sonic2kk]

I think the last remaining issue is that when we refresh the CtInfo dialog, the self.ctool reference is not updated. This is a problem because after marking as global (which calls the Refresh method) we don't have an updated reference to self.ctool.is_global, so many UI show/hide functions still act as though we don't have a global compat tool.

We will need a way to update our reference to self.ctool to be more up-to-date. In update_game_list_steam we emit to batch_update_completed which correctly updates the display on the Main Menu to mark the tool as "(global)" (as it will re-parse the config file and so pick up that the tool is now global). However to my understanding Qt signals are asynchronous, so there is no way to know when the data is actually updated. This also only applies to Steam, meaning that for other launchers,

Perhaps the best way to do this in a launcher-agnostic way is to return the ctool object in set_launcher_global_tool alongside the boolean success value.

• This would be straightforward but would not allow us to update the UI for other launchers to display that a tool is global. We can do this for Steam already because we unconditionally emit to batch_update_completed when we call update_game_list_steam.

We could emit to a slot that calls MainWindow#update_ui and then have MainWindow#update_ui emit to a slot that we listen for on CtInfo that gives us an updated compat_tool_index_map which we could then parse our ctool out of. This would allow us to:

• Update the UI for all launchers when we mark a tool as global, so they will all display (global). If we are able to mark a tool as global from CtInfo, the launcher should always be able to display a tool as global, otherwise we wouldn't be able to mark it as global in the first place.
• Remove the unconditional call to the Batch Update Completed signal when we update the Steam Game List
• But this all has the drawback of update_ui returning us a compat_tool_index_map that we have to parse. We could get around this by making a util method to find a given BasicCompatTool object from a compat_tool_index_map. We would also have to block and wait for this.

---

The overall problem is when we mark a tool as global or when we refresh the ctinfo dialog (which would pick up externally-modified global tools such as those updates from the launcher itself) that we need to:

• Update the compat_tool_index_map for all launchers, to properly display that a tool is "(global)" in the name
  • This works for Steam right now only because the batch update complete signal is called when we update the Steam game list, which is called when we refresh the UI, which is called on refresh button click AND on marking a tool as global.
  • But this won't work once marking global is expanded to other launchers like Lutris
• Update the CtInfo dialog's self.ctool so that it is fully refreshed, and make sure all UI elements that depend on attributes from self.ctool (such as self.ctool.is_global for show/hide logic) get updated properly.

I am unsure how to accomplish this.

EDIT: I wonder if Qt's Property would fit here? We could store self.ctool as a Property. Then we could hook up the Refresh logic to when the ctool changes. We could emit to a new slot on the Main Window to update its tool list. Something like self.update_tool_list.emit(self.ctool) and then the Main Window would hook into this in a separate method that first calls its self.update_ui method, then it would find the ctool that the current CtInfo dialog is using based on the ctool parameter given. This has a couple of problems:

• The question I can't answer here is how we then update the CtInfo's self.ctool. Can we pass self like self.update_tool_list.emit(self.ctool, self)?
  • This may have to be optional in case we ever want to call this from anywhere else where we may not have an object passed in which has a self.ctool parameter.

This seems very convoluted and so possibly the wrong approach...