Skip to content
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

pupgui2: Don't attempt to map install_directory if combo_install_location_index_map is empty #458

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

sonic2kk
Copy link
Contributor

Fixes #454.

Overview

This PR adds a check to MainWindow#combo_install_location_current_index_changed to ensure that self.combo_install_location_index_map is not empty. This self.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 forcing POSSIBLE_INSTALL_LOCATIONS = [] in constants.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 the index 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, in MainWindow#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:

  1. Open ProtonUp-Qt with no install directories found at all.
    a. This can be simulated by forcing POSSIBLE_INSTALL_LOCATIONS = [] in constants.py.
  2. Add a Custom Install Directory by clicking on the three dots button to open the Custom Install Directory dialog (...) and adding a valid path.
  3. Open the Custom Install Directory dialog again and click the "Default" button.
  4. The MainWindow will remove the Custom Install Directory successfully instead of crashing and will return to its initial state in Step 1.

image

image

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 an index argument whenever combo_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 the combo_install_location_index_map list is not empty.

Let me know if there are any concerns with this approach. Thanks!

@DavidoTek
Copy link
Owner

Thanks!

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

Yes, maybe we can add a warning text like No launchers detected. That could be extended with a tooltip text that explains the warning...

grafik

@DavidoTek DavidoTek merged commit d848220 into DavidoTek:main Sep 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IndexError: List index out of range
2 participants