-
-
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
Improvements to SteamTinkerLaunch support #125
Conversation
Informing the user that the path will be changed is the best in my opinion as we determine a standard installation folder this way.
Yes, we should keep our hands off system wide/package manager installations.
I think this is a rare special case. A message like
Agreed.
We need to work on the Flatpak stuff, otherwise looks good! |
ec616e5
to
ac020e0
Compare
ac020e0
to
ced6281
Compare
Thanks for the positive feedback 😄 I'm working on implementing the changes (bar Flatpak stuff), but I still need to test it some more. Still all WIP but it's coming along 👍 |
64b0dd7
to
7a42854
Compare
I'll keep working on this but the current plan of what I want to implement is this:
There are still a few things I need to iron out with this PR before I move it out of draft, should be ready in the next few days if all goes to plan 😄 |
So the core of what I'm trying to achieve seems to work, but I'm having some trouble implementing the dialog prompt that shows up when a user tries to install STL through PUPQT when an existing install is present. I tried implementing just a simple conditional in the ctmod's has_external_install = get_external_steamtinkerlaunch_intall(os.path.join(install_dir, 'SteamTinkerLaunch'))
if has_external_install:
print('Non-ProtonUp-Qt installation of SteamTinkerLaunch detected. Asking the user what they want to do...')
mb = QMessageBox()
mb.setWindowTitle('Existing SteamTinkerLaunch Installation Found')
mb.setText(f'It looks like you have an existing SteamTinkerLaunch installation at \'{has_external_install}\' that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to \'{constants.STEAM_STL_INSTALL_PATH}\'. Do you wish to continue? (This will not affect your SteamTinkerLaunch configuration.)')
cb = QCheckBox('Remove existing SteamTinkerLaunch installation')
cb.setChecked(True)
mb.setCheckBox(cb)
resp = mb.exec() But this causes a segfault (probably the same as #124). So, I tried implementing a signal, which is what the current code in this PR has (though I have it hardcoded to always stop the installation for development purposes). This worked and the dialog showed up but this didn't pause the actual installation process. It showed the dialog but it means nothing because the application should wait until the dialog is closed. # ctmod_steamtinkerlaunch.py
has_external_install = get_external_steamtinkerlaunch_intall(os.path.join(install_dir, 'SteamTinkerLaunch'))
if has_external_install:
print('Non-ProtonUp-Qt installation of SteamTinkerLaunch detected. Asking the user what they want to do...')
continue_install = self.install_question_box_message.emit(
# Title and Text
'Existing SteamTinkerLaunch Installation',
f'It looks like you have an existing SteamTinkerLaunch installation at \'{has_external_install}\' that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to \'{constants.STEAM_STL_INSTALL_PATH}\'. Do you wish to continue? (This will not affect your SteamTinkerLaunch configuration.)',
# Checkbox
True,
# Checkbox Text
"Remove existing SteamTinkerLaunch installation",
# Checkbox enabled
True,
# Icon
QMessageBox.Warning
) # pupgui2.py
@Slot(str, str, bool, str, bool, QMessageBox.Icon)
def show_question_msgbox(self, title: str, text: str, checkbox: bool = False, checkbox_text: str = '', checked: bool = False, icon = QMessageBox.NoIcon):
""" Shows a Yes/No Question Dialog """
mb = QMessageBox(parent=self.ui)
mb.setWindowTitle(title)
mb.setText(text)
mb.setIcon(icon)
mb.addButton(QMessageBox.No)
mb.addButton(QMessageBox.Yes)
if checkbox:
cb = QCheckBox(checkbox_text)
cb.setChecked(checked)
mb.setCheckBox(cb)
return mb.exec() I looked around this weekend to find some kind of solution to this, but no luck. Could you offer any advice on how to best go about this? The Perhaps this can/should be done using some kind of Qt threading classes but I don't know much about that, and I don't want to try learning and implementing threading if there's a quicker, simpler way to go about it. Once this is sorted I think I can start doing some final cleanup and then move this PR out of draft 😄 |
Looks good so far, thanks!
Probably can be done by converting the installer thread from |
I'm more than willing to try and help implement #127 into this PR. The idea seems to be very similar at a surface level at least. No rush though, and no problem if you'd prefer the work kept separate 😃 |
That would be great!
I'm a bit busy at the moment, but I still need to take a look at what's the best way to fix the dialogs then. No problem if we do it in this PR. We can call it "Improvments to STL" or something. |
Just a heads up, I'm now the project lead on SteamTinkerLaunch (sonic2kk/steamtinkerlaunch#632). Because of my STL fork there were a couple of complications but very soon the project will be transferred to me and so the URL for STL will likely change. Updating that might be a bit of a "priority" (not sure what |
🌲🪚🌳 "forstworx" :) |
I've updated the URLs to the new repo, but I think the old one will be redirected to the new one just fine. |
I've been meaning to mention here as it's been included for a few days now, but on Steam Deck STL now shows a notifier during installation. This is potentially a little bit unnecessary/intrusive with ProtonUp-Qt, so when implementing it I included a Maybe as part of these changes we can pass that flag when calling SteamTinkerLaunch on Steam Deck. Also, I saw commit 138b189, do you know if this has any impact on the mentioned issues with the dialogs? No problem if not and no rush on my side at all here, just pure curiosity :-) As an aside, I've seen a number of users installing STL through ProtonUp-Qt, so there should be no rush on these improvements as everything should be working currently. And it'll probably be a little while before I roll the next version of SteamTinkerLaunch so there's no rush to that end either. |
That is good, we should use it then. remind myself @DavidoTek
I was trying a few methods to get the return value of the question message box. I've changed the code a bit so a blocking call(signal) can be made from the installer thread to the main thread and wait for the user to press okay. But I still have no clue what is the "best"/least hacky way to get a return value from the message box function without changing the code structure too much... I'll update the code and write here if I've found a good method ;)
Okay, that's very good, I wasn't sure about that. |
I've finally managed to add functionality to create a message box with a checkbox from a ctmod in fde1778. # Under "message_box_message = Signal(str, str, QMessageBox.Icon)" add following:
cbquestion_box_message = Signal(str, str, str, QMessageBox.Icon)
# Inside "__init__" add following:
self.main_window = main_window
# Emit a signal to the blocking slot and retrieve the bool result using "get_msgcb_answer"
self.cbquestion_box_message.emit('Title', 'Message', 'Checkbox', QMessageBox.Warning) # will block until "okay" is pressed
print( self.main_window.get_msgcb_answer() ) PS: There still could be issues with the implementation, please tell me if you find any. No rush though, I will do some testing in the next days myself. |
Thank you, this looks awesome! I'll take a look at this over the next couple of days and pick up the work here again (and add in the extra stuff like the I'll also do a comprehensive run-through of installing STL on my Steam Deck and laptop and testing out different scenarios. One user recently reported the STL-git (specifically git only iirc) install hanging on their Steam Deck with the ProtonUp-Qt Flatpak. I couldn't reproduce on my laptop from PUPQT git, but looking at the commit history and from memory I also think there were some improvements and fixes for STL on the PUPQT side that aren't in a stable release yet. So I don't think it's an issue in the current main branch. However I will test thoroughly once these main changes are done and if there are any other fixes needed I'll include them before we merge this :-) Thanks so much for your work here, it is hugely appreciated! EDIT: Sorry, didn't get a chance to look at this today, got distracted sorting some Vortex stuff for STL. Hoping to take a look at this over the weekend though! |
Resolved the merge conflicts (hopefully 😅) correctly. Implementation looks good just from reviewing in more depth from resolving those conflicts. I haven't tested it locally yet but it should be reasonably straightforward to refactor and properly call your new implementation (which looks a heck of a lot cleaner than how I was trying to do it!) Since I just resolved merge conflicts and didn't test, this branch may not be in a working state right now (I resolved them online using GitHub's editor) but I will take a look into properly testing over the weekend and getting the messagebox stuff working as intended. Thanks a ton for all your patience with this and the STL integration in general. I can understand that it may have introduced a bit more of a maintenance burden than the existing integrations 🙂 |
Thank you soo much for you help!! |
Okay so the implementation looks pretty good! It functions exactly as expected in my testing. But there's one small thing I would like added, or if you would prefer not to add it we can discuss alternatives depending on what kind of design you want to go with 😃 Just to recap, the problem was that the dialog that appeared when a user wanted to install SteamTinkerLaunch with an existing installation present not managed by ProtonUp-Qt. This dialog did not actually "pause" anything, so the installation would continue anyway. ProtonUp-Qt does now actually pause! The only issue I'm facing now is that there is no option to "cancel" the install. Below is a screenshot of how the dialog looks (the text here is subject to change pending further feedback once the actual functionality works, sorry if it's a little rough): My idea here is that a user can press "OK" to continue installing, and "Cancel" if they realise "Oh shoot I actually don't want to install anymore". If there was a cancel button, and then a way to know if the user selected "OK" or "Cancel" as well as whether or not they selected the checkbox, I think that would be nice. If you don't want to add a cancel button for any reason, if there's a way to tell if they pressed the "X" button on the dialog prompt that would function similarly. If you don't like the sound of that idea, we can discuss alternatives 🙂 But I think having a cancel button would be the most straightforward for users. I haven't forgotten about #127 either, and I'm hoping to get that implemented with this PR now that the pausing has been implemented. |
While reading through the issue about shell modification i also remembered we discussed conditionally showing a checkbox. The current implementation doesn't allow for this but I add in this functionality: @Slot(str, str, str, QMessageBox.Icon)
def show_msgbox_cbquestion(self, title: str, text: str, checkbox_text: str, icon = QMessageBox.NoIcon) -> bool:
""" Show a message box with main window as parent """
mb = QMessageBox(parent=self.ui)
mb.setWindowTitle(title)
mb.setText(text)
mb.setIcon(icon)
cb = None
if len(checkbox_text.strip()) > 0:
cb = QCheckBox(checkbox_text)
mb.setCheckBox(cb)
mb.exec()
self.set_msgcb_answer(None if not cb else cb.isChecked()) A small concern is that add_to_path = self.main_window.get_msgcb_answer()
if not add_to_path and add_to_path is not None:
print('We showed a checkbox and it was False!')
else:
print('Either the checkbox was true or not shown at all!') Maybe Not sure what the best behaviour is here but just something I was investigating :-) Another potential problem is that here is no way to set if the checkbox is enabled or disabled by default. For dialogs like the one warning about shell configuration, it would be nicer to have this enabled by default. Even though the checkbox will only show up for users in advanced mode, having the default as checked would be better imo but I am open to feedback! |
Didn't get a chance to work on this today but thanks, your explanation and solution make sense :-) Will see about getting this finalised over the weekend (though I am planning a short break soon) 🙂 |
No rush. PS:
Well, actually this won't work as the uninstallation will continue afterwards... I think that would be a weird/hacky solution, but we could pass an optional variable "ctmod_object" with a reference to the CtInstaller object ( EDIT: Seems to work fine. Not sure about the Qt translations part though.. # steamutil.py, line 317
def remove_steamtinkerlaunch(compat_folder='', remove_config=True, ctmod_object=None) -> bool:
# steamutil.py, line 348-352
if ctmod_object:
if hasattr(ctmod_object, 'message_box_message'):
ctmod_object.message_box_message.emit(
ctmod_object.tr('Unable to Remove SteamTinkerLaunch'),
ctmod_object.tr('Access to SteamTinkerLaunch installation folder at {STL_SYMLINK_PATH} was denied, please remove this folder manually.\n\nThe uninstallation will continue.')
.format(STL_SYMLINK_PATH=stl_symlink_path),
QMessageBox.Icon.Warning
)
else:
mb = QMessageBox()
mb.setWindowTitle('Unable to Remove SteamTinkerLaunch')
mb.setText(f'Access to SteamTinkerLaunch installation folder at \'{stl_symlink_path}\' was denied, \
please remove this folder manually.\n\nThe uninstallation will continue.')
mb.exec() EDIT2: Translations will work using: EDIT3: I have added the changes with 6518e60. Please let me know if there is something I should change. |
Thanks a ton for your work here! After some testing, the PR should be ready for review (and discussion on handling this within Flatpak 😅) P.S. A few users now have reported that ProtonUp-Qt does not install SteamTinkerLaunch in Flatpak on Steam Deck. I can't reproduce from git and I think it was fixed with a8d7597 Once the next release rolls around it might be a good idea to call out SteamTinkerLaunch fixes :-) |
Okay, I did one final check and your improvements seem to fix it! Thank you so much! In my testing I did discover that the dialog when you press "Remove" was displaying the incorrect game count for SteamTinkerLaunch. I fixed this by changing the logic in We can talk about ways to improve that though 🙂 It still correctly counts ctmod usage for other compat tools so the change should hopefully be fine functionally, but it is slower and that is not great |
Okay, this PR is fully ready for review now. Given that the scope of this PR widened a bit during development, I updated the name of the PR to reflect this 😄 Below is a full changelog of what this PR does:
Of course there was a huge amount of work on your part to facilitate these changes, with big improvements to ctmod information passing and dialog threading too. Currently the ctmod dialogs and such for STL don't have any translations as far as I'm aware. If there is anything I need to change on my end for this please let me know :-) There may be some concerns here around Flatpak support, when it comes to checking for existing SteamTinkerLaunch installations. It is highly unlikely that a user will have SteamTinkerLaunch installed outside of their home folder. Most users will probably have it somewhere in their Downloads folder or in a custom Applications folder, both of these would likely be in their home folder. I am not sure if the Flatpak has general access to a user's home folder, and if it doesn't that is probably by design so I am not sure how we would want to handle this. This PR gets the location of existing installs by taking the path from the symlink using If ProtonUp-Qt can read the home folder, then I think there is no issue here :-) And on Steam Deck (where I would wager most Flatpak users come from) this should not really affect them in that case - Plus STL always goes to I am not sure if there are any other potential Flatpak concerns with these changes. But we can discuss all of that now that the changes are functionally complete 😄 And of course, feedback on code style and so on are always appreciated here 😄 Commits will be squashed once review is done if that's ok. Thank you so much for all of your support and collaboration with this PR, let's get it finalized and get it merged 🥳 |
Okay, thank you very much! 🥳 🎉 Looks very good.
Translations should work fine as the ctmods are loaded after the QApplication and QTranslator are initialized.
ProtonUp-Qt currently doesn't have full permission to the home folder. Reading the symlink is possible, but deleting the target folder may not. |
Ah, okay, awesome! 😄
If reading the symlink works i think that is fine, because:
I agree that Flatpaks which can only access their "required" folders are more secure. I think that's the "spirit" of Flatpaks, so I understand and agree with your decision here. And it does have execute permissions, but since Flatpaks aren't installed as root and since there are access limits, that mitigates the "risk" to some extent I think :-) At least, that is my armchair not-a-flatpak-dev understanding 😉
So this is kind of what I was thinking about yesterday and it's definitely a good point. However, I think this will be fine for a few reasons:
But on top of this, I will also add a section on STL's ProtonUp-Qt wiki page with information on replacing manual installs with ProtonUp-Qt, and that the Flatpak will need to be granted access to the install if it is installed outside of |
Good to hear that we agree on that.
Great!
# For ctmods
QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', 'blah blah')
# For other files I currently use the filename to group the translations.
# If it is used inside a class with a descriptive name, `self.tr('blah')` will use the class name to group automatically.
QCoreApplication.instance().translate('steamutil.py', 'blah blah')
# If there is a additional parameter, we can first translate it and add it using `.format` afterwards
folder = '/home/user/test'
QCoreApplication.instance().translate('util.py', 'The tool will be installed in {INSTALL_FOLDER}. Now you have {NUM_TOOLS} tools installed!').format(INSTALL_FOLDER=folder, NUM_TOOLS=5) PS: Let me know if there is something I should add/help with. |
Thanks for the examples, I think I get how to go about implementing the translations. Something like this: self.question_box_message.emit(
'Existing SteamTinkerLaunch Installation',
f'It looks like you have an existing SteamTinkerLaunch installation at \'{has_external_install}\' that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to \'{constants.STEAM_STL_INSTALL_PATH}\' if it has write access to this folder, otherwise it will continue installation as normal.\n\nDo you want to continue installing SteamTinkerLaunch? (This will not affect any existing SteamTinkerLaunch configuration.)',
'Remove existing SteamTinkerLaunch installation',
MsgBoxType.OK_CANCEL_CB,
QMessageBox.Warning
) Would become: self.question_box_message.emit(
QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', 'Existing SteamTinkerLaunch Installation'),
QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', f'It looks like you have an existing SteamTinkerLaunch installation at \'{has_external_install}\' that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to \'{constants.STEAM_STL_INSTALL_PATH}\' if it has write access to this folder, otherwise it will continue installation as normal.\n\nDo you want to continue installing SteamTinkerLaunch? (This will not affect any existing SteamTinkerLaunch configuration.)'),
QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', 'Remove existing SteamTinkerLaunch installation'),
MsgBoxType.OK_CANCEL_CB,
QMessageBox.Warning
) Forgot to use If this is correct, I can implement translations for the STL dialogs, and then get the commits squashed :-) |
Yes, almost. Except this line QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', f'It looks like you have an existing SteamTinkerLaunch installation at \'{has_external_install}\' that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to \'{constants.STEAM_STL_INSTALL_PATH}\' if it has write access to this folder, otherwise it will continue installation as normal.\n\nDo you want to continue installing SteamTinkerLaunch? (This will not affect any existing SteamTinkerLaunch configuration.)'), Formatting needs to be done after translation. This should work: QCoreApplication.instance().translate('ctmod_steamtinkerlaunch', 'It looks like you have an existing SteamTinkerLaunch installation at "{EXTERNAL_INSTALL_PATH}" that was not installed by ProtonUp-Qt.\n\nReinstalling SteamTinkerLaunch with ProtonUp-Qt will move your installation folder to "{STL_INSTALL_PATH}" if it has write access to this folder, otherwise it will continue installation as normal.\n\nDo you want to continue installing SteamTinkerLaunch? (This will not affect any existing SteamTinkerLaunch configuration.)').format(EXTERNAL_INSTALL_PATH=has_external_install, STL_INSTALL_PATH=constants.STEAM_STL_INSTALL_PATH), |
6908658
to
97197fb
Compare
Thanks! I pushed the translation updates. I believe I have done it correctly, I did a quick smoke test of the dialogs and they seem to be still showing the text and formatting correctly. Can't vouch for whether or not they actually translate but I think this is fine for now 😅 |
Thank you, looks good! 🎉 I will change one little thing regarding the code to count the number of games in use. After some final testing, I will squash and merge this PR. |
Heck yeah, awesome!! And yes any changes for the game counting would be great :-) Once merged and when the next PUPQT release rolls out, I'll include a note and upstream link on the changelog for StemTinkerLaunch v12 to mention that ProtonUp-Qt integration got some nice improvements :-) |
I've merged the changes into main and will prepare a new release with the new additions soon. Thank you again for the awesome collaboration! 🎉 🎉 🎉 |
It was a pleasure 🥳 I try to keep an eye out for issues but feel free to ping me if there is anything STL-related you need me to take a look at in future (even just a "cc @sonic2kk" is fine). I'm always glad to help out however I can :-) (and maybe I just a like a good excuse to use Python 😆) |
I create a new release 2.7.5. The AppImage is available here as usual: https://github.com/DavidoTek/ProtonUp-Qt/releases
Great to hear. Trying something out and maybe learning something new is the fun part 😄 |
Opened as a draft as I need to see how this impacts switching STL versions + some other extra testing, and I have some other concerns listed below.
I noticed on my PC that ProtonUp-Qt detects my Non-ProtonUp-Qt install of STL (as expected, not a bug or anything imo) but then had the idea that it could be neat if ProtonUp-Qt could see and manage non-root local installs that were not installed by ProtonUp-Qt. There's an arguably valid case that someone might've locally installed STL and would want it to be managed. Since ProtonUp-Qt can currently manage setting STL in the Games List no matter how a user has installed SteamTinkerLaunch in the past.
This PR in its current form makes an initial attempt at this. In the
remove_steamtinkerlaunch
method I addedstl_symlink_path
which is either the target for the SteamTInkerLaunch symlink in the compat folder, orNone
. Then when removing the SteamTinkerLaunch installation we check if that path exists and if it does and if its writable (avoids trying and failing to remove installs in/usr/bin
). The regularSTEAM_STL_INSTALL_PATH
value is moved to anelif
block.I need to do some more testing and I also have a few points I want to raise.
~/stl
. This makes sense to me and is what I would consider expected behaviour, but perhaps we should do one of two things:~/stl/prefix
).The latter has a few problems around ProtonUp-Qt always being able to "know" where STL is installed. I'm slightly less keen on this idea and I would opt to do the former if it's necessary to tell the user this. The number of users with an STL install like this is probably low, most users I see usually have STL at
/usr/bin
based on their logs/home/gaben/Downloads/steamtinkerlaunch-master
, or it could be 30 folders deep in another drive.Despite the last two, I would be in favour of keeping SteamTinkerLaunch listed no matter where it is installed on a user's system, as there is the useful case of setting the compatibility tool in the games list.
There is still some work to do but I'd like to hear any feedback. Thanks so much for your continued work on fixing up STL support, I was going to leave a comment about this in the STL discussion issue but I figured I'd actually try and give back a little. No rush on this PR though, we can work through any feedback as and when it comes through and go from there :-)
Thanks!