pupgui2: Don't attempt to map install_directory if combo_install_location_index_map
is empty
#458
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #454.
Overview
This PR adds a check to
MainWindow#combo_install_location_current_index_changed
to ensure thatself.combo_install_location_index_map
is not empty. Thisself.combo_install_location_index_map
could be empty if there are no launchers found by ProtonUp-Qt, and attempting to "Default" when no launchers are found results in the crash. This can be simulated by forcingPOSSIBLE_INSTALL_LOCATIONS = []
inconstants.py
.If it is empty, then any attempt to index it will result in the error in #454, where we try to index this based on
self.ui.comboInstallLocation.currentIndex()
. This check avoids indexing when the list is empty, fixing the crash.Problem
Whenever we press the "Default" button for Custom Install Directory, we attempt to update the
comboInstallLocations
dropdown so that it can return to the index for the launcher we want to default back to. For example with Steam, we would want to default back to the regular Steam install location and not the overridden Custom Install Directory. If no launcher for Steam was ever found, we would just default to the next one (i.e. Lutris).The logic to do this tries to index a list that contains a mapping of launchers and install directories. We index this based on the index of the
comboInstallLocation
dropdown. However if this is empty, which it could be if no launchers were found at all and the Custom Install Directory was used to set a path, then trying to default back results in trying to default back to an empty mapping. This results in theindex out of range
crash.Note that if a launcher is installed, ProtonUp-Qt should always find it. Typically misconfigured packages, missing Flatpak permissions, library issues, or no launchers installed cause the issue if no launchers found. But the last case, no launchers installed, could still be valid; a user might, for whatever reason, want to use ProtonUp-Qt with no launchers installed and download them to a set location (or may have configured the Flatpak to have extremely limited permissions, for security, and wants to keep the application very isolated and simply use it to download tools into one folder it has access to). In that case, defaulting back makes little sense, but a user might still want to do it. 🙂
Solution
To fix this, we should not try to index
self.combo_install_location_index_map
if the map is empty, i.e.len(self.combo_install_location_index_map) <= 0
, inMainWindow#combo_install_location_current_index_changed
. This is essentially the same kind of check one might do before indexing any list, to make sure it is not empty before indexing. But I wanted to explain above why this mapping may be empty, as that is equally as important.This check means that when we default back and remove the Custom Install Directory, we just update the MainWindow UI and return from
combo_install_location_current_index_changed
. There's nothing else for us to do, so we can just skip this function and return. This allows us to go from the UI having a Custom Install Directory, back to having no install directories found at all and displaying a blank UI.The flow can be tested on this PR as follows:
a. This can be simulated by forcing
POSSIBLE_INSTALL_LOCATIONS = []
inconstants.py
....
) and adding a valid path.Future Work
It has been discussed before, but we probably want to improve the UI a bit when no install directories are found. We could add some visual indication that no launchers were found, or possibly even show a dialog box if we wanted to and give the user some troubleshooting steps (validate Flatpak permissions, etc).
I think that is out-of-scope for this PR, but is something we should look into doing in future. 🙂
I went through a couple of iterations, checking the
comboInstallLocations.currentIndex
value, accepting anindex
argument whenevercombo_install_location_current_index_changed
is called (this is-1
from PySide6 if the dropdown is blank), but eventually landed on a simple check to ensure thecombo_install_location_index_map
list is not empty.Let me know if there are any concerns with this approach. Thanks!