Consider using a config file to load the github token from
txtsd opened this issue · 7 comments
Is your feature request related to a problem? Please describe.
I don't want to have to prepend an env var and launch from a shell.
Describe the solution you'd like
I want to launch protonup-qt from my application launcher.
Describe alternatives you've considered
I've considered exporting the env var from my .bashrc/.zshrc/.zshenv but this pollutes the config space of my shell instead of adding to protonup-qt's own config space.
Additional context
Basically, check for the existence of $XDG_CONFIG_HOME/protonup-qt/config (.txt or .yaml or .json or whichever) and load the token from there.
You can prepend a token as an environment variable from your application launcher in the vast majority of instances (usually under some form of "Edit Applications" list), and you can also configure Flatpak environment variables using Flatseal or KDE's built-in Flatpak permissions editor, however I agree that having a setting for this would be great.
I mentioned in another issue that replacing "About" with a settings menu (and having "About" as a tab here) would be a good idea, so we could have an option for GitHub and GitLab tokens on the main menu. GitLab ctmods are not in a stable release yet but there was a refactor to allow downloads from GitLab to accommodate #302.
We already store the current theme in a config file so I don't think it would be much more effort to store two strings.
I am not sure how the config file is structured but as a stop-gap to save us from refactoring a dialog to implement this, we could just check for and try to load this token from the config file initially, for users who manually place it there. Then in future we can expose it on the UI.
I think that's what this proposal was actually getting at, and that shouldn't be too tricky :-) I'll look into that a little bit tonight.
I do have some concerns over implementation details, such as should this take priority over the environment variable, or should this replace the environment variable option altogether? If the latter, we could document on the Readme that this should be set in the ini file and update the GitHub rate limit warning (or, hopefully, before the next release we can have a refactored "Settings" dialog).
I don't think this feature per-se shouldd have to wait on the settings dialog though.
Excellent! You went all out!
Regarding the order of preference for configuration values, this answers it well:
https://stackoverflow.com/questions/11077223/what-order-of-reading-configuration-values
But we don't have to support all of those scenarios.
I would also suggest, contrary to how ProtonUp-Qt works currently, that all versions of the various flavors of wine and proton be fetched as soon as a token is detected. This way there won't be an unresponsive GUI when changing the flavors of wine in the dropdowns.
Is this still feasible to do when we have a token? I forget the exact number, but let's say we fetch 15 versions, and each of the five launchers (Steam, Lutris, Heroicwine, Heroicproton, Bottles) have an average of 6 compatibility tools. That's quite a lot to fetch. With Advanced Mode this can get even longer.
I just think it's quite a lot of fetching. Even if we only do this when we switch launchers (i.e. only fetch Steam launchers when that's the current one) it's still a lot of calls to make.
I don't think I agree that we should fetch all the tools ahead of time, especially because a user may only want one tool. The UI also isn't really "unresponsive", it's intentionally disabled while versions are being fetched.
Plus, even though it might be feasible to refactor, the logic to fetch versions is called by the install dialog. Moving it to the main window, and then storing a reference to all of these tags, wouldn't necessarily be that easy. Plus, fetching the versions dynamically each time means the version list is always refreshed when a user goes to install a tool, so we avoid any sort of caching issues.
I don't think I agree with this specific part of the change. Fetching the compatibility tool versions as they are needed is by design I think and is more desirable for the reasons listed above. I don't think the UI being greyed out for a brief period of time is that much of an issue, and the status bar notes that versions are being fetched anyway, so I think it's ok to leave this as-is :-)
If DavidoTek feels differently though it could be incorporated in a separate PR.
I'll look into how to better handle the environment variables as per your link, I think at the very least I'll mention something about this on the PR as well. Though it's worth nothing that ProtonUp-Qt doesn't really have a pattern of allowing its settings to be overridden by environment variables. We don't have many, so it could be done, but that's just something I wanted to mention, it isn't really done elsewhere yet so this would be a slight shift.
I'll leave the priority order / keeping environment variables around at all up to DavidoTek as well, I'm not that strongly opinionated one way or the other, though I do think if we allow this for the tokens, maybe we should apply it to other config options - though I'm not sure how much sense it makes to do that. Right now, yes, it makes sense for the tokens, but once they're on the UI it may make less sense.
Always open to further discussion on either of these points though 😄
The PR was updated to prefer environment variables over the config file values, so it will use both. When setting PUPGUI_GHA_TOKEN or PUPGUI_GLA_TOKEN, these will be preferred over their config file counterparts, which are now named github_api_token and gitlab_api_token respectively.
I merged PR #306 now. The ability to set api tokens in the config file will be available with the next release of ProtonUp-Qt. A GUI option to configure the keys may follow.
See #161 (comment) for details about the configuration file format.
PS: As of now, a full ProtonUp.Qt config.ini may look like this:
[pupgui]
installdir = /path/to/compatibilitytools.d/
[pupgui2]
advancedmode = enabled
custom_install_dir = /path/to/custom/lutris/runners/wine/
custom_install_launcher = lutris
github_api_token = my-github.com-api-token
gitlab_api_token = my-gitlab.com-api-tokenAwesome!
Thanks, @sonic2kk and @DavidoTek for the quick discussion, implementation, and testing.
@sonic2kk The verbosity in your explanations is absolutely bonkers!
I would also like to thank @BrodieRobertson for talking about this tool in his video, which led me to use it, get a bit frustrated, and make a QoL suggestion.
(I used to download and unpack GE and tkg manually before this 🤢 )