DuplicateOptionsError (crash on startup)

[B4rabbas]

Describe the bug
ProtonUpQt crash on startup

To Reproduce
Steps to reproduce the behavior:

1. Launch the app
2. See error

Expected behavior
Launching the app witheout crash.

Screenshots
Sorry, there is nothing more than the shell output.

Desktop (please complete the following information):

• Platform: standard amd X64 desktop computer
• System: Linux Mint 21.3 x86_64
• Version: v2.9.1
• How did you install ProtonUp-Qt?: Flatpak

Additional context
Happen after installing proton in native steam (for my lutris library)

Terminal output

❯ /usr/bin/flatpak run --branch=stable --arch=x86_64 --command=net.davidotek.pupgui2 net.davidotek.pupgui2
ProtonUp-Qt 2.9.1 by DavidoTek. Build Info: DavidoTek Flathub build.
Python 3.10.13 (main, Nov 10 2011, 15:00:00) [GCC 12.2.0], PySide 6.5.2
Platform: KDE Flatpak runtime 6.5 Linux-5.15.0-97-generic-x86_64-with-glibc2.35
Gtk-Message: 19:03:44.820: Failed to load module "xapp-gtk3-module"
Qt: Session management error: Could not open network socket
Loading locale fr / fr_FR
Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/app/lib/python3.10/site-packages/pupgui2/__main__.py", line 2, in <module>
    main()
  File "/app/lib/python3.10/site-packages/pupgui2/pupgui2.py", line 569, in main
    apply_dark_theme(app)
  File "/app/lib/python3.10/site-packages/pupgui2/util.py", line 88, in apply_dark_theme
    theme = config_theme()
  File "/app/lib/python3.10/site-packages/pupgui2/util.py", line 152, in config_theme
    return read_update_config_value('theme', theme, section='pupgui2')
  File "/app/lib/python3.10/site-packages/pupgui2/util.py", line 138, in read_update_config_value
    config.read(config_file)
  File "/usr/lib/python3.10/configparser.py", line 699, in read
    self._read(fp, filename)
  File "/usr/lib/python3.10/configparser.py", line 1098, in _read
    raise DuplicateOptionError(sectname, optname,
configparser.DuplicateOptionError: While reading from '/home/harry/.var/app/net.davidotek.pupgui2/config/pupgui/config.ini' [line  5]: option 'github_api_token' in section 'pupgui' already exists

[DavidoTek]

configparser.DuplicateOptionError: While reading from '/home/harry/.var/app/net.davidotek.pupgui2/config/pupgui/config.ini' [line 5]: option 'github_api_token' in section 'pupgui' already exists

Did you configure the GitHub API access token? It seems like it is configured twice.

Please edit /home/harry/.var/app/net.davidotek.pupgui2/config/pupgui/config.ini and remove the second github_api_token entry.

Maybe you meant gitlab_api_token instead? (GitLab not GitHub)

[B4rabbas]

Yes, it's fixed.

[sonic2kk]

Is there a (straightforward) way we could manage this better? For example removing duplicate entries, or ignoring them and using the latest?

Assuming the issue above was that github_api_token was defined twice when it was meant to be gitlab_api_token, this would simply mask an issue, as a user would expect the GitLab token to be defined and it wouldn't be. But maybe handling it in some way is better than a crash? This crash afaict comes from the library we use but maybe there's something we can configure?

Then again , maybe not worth it since the long-term plan is to allow this to be configurable from the UI.

[sonic2kk]

I had a look into this, we could catch configparser.DuplicateOptionError, but we'd have to do this in a few places.

Mostly when we read from the config file in util.py, we use read_update_config_value (introduced as part of #306). There was, as far as I recall, an explicit decision to not use read_update_config_value in certain places, because a refactor could end up being a bit unwieldy. So there are two places where we still interact with the config file directly:

• install_directory
• config_custom_install_location

In order to catch this error, we would need to make adjustments in the following places (how we want to handle the exception is a different story):

• read_update_config_value - This one is straightforward, we can just wrap the function in a try/except DuplicateOptionError block.
• install_directory - Also straightforward, the same idea, and we return an empty string if the install directory is not found anyway, so we have a fallback return value
• config_custom_install_location - The tricky one, as this expects to always return a dictionary. Depending on how we want to handle the exception, i.e. if we want to just warn and silently continue, we'll have to be careful here.

So that covers where we need to make the changes, and how we coulld do it. Basically just wrap these interactions with the config file in a try/except that catches DuplicateOperationError.

If we go forward with this, the question becomes how we want to handle this. There are some ideas I have:

• Show a slightly more user-friendly error. Instead of a stack trace, we could show a dialog and print that some key in the config file is duplicated. In read_update_config_value, we have the section and option, so we could offer a guess to the user as to what key is duplicated in what section.
  • Continuing might be tricky, because I think if there's a duplicate key, every read of the config file will give us this error. Maybe having a global instance of ConfigFile is a solution, but maybe each time we write we'll still get pestered about this, not sure...
• Same as above, but still exit. This might be less user-friendly as a user might just click to make the error dialog go away, and then try to re-run the program, think it doesn't work, and just never use it. There is no "automated" path to resolution. Should there be? Eh... probably not... I'll get to that in my last point.
• Print to the console (and/or show a dialog as above) to inform of the duplicate key, but continue anyway, taking the value of the first or last occurance of the option (i.e. if github_api_token is defined 5 times, take either the 1st or 5th occurrence).
• I don't know if this one is possible, but if we could read and remove duplicate keys, that might be an option to do after warning about the duplicate values. If we, for example, show the warning dialog, the user will see it everytime they open ProtonUp-Qt. This is good because it means if they read it, they can get an idea on how to fix it in a human-readable way. If they don't though, they're going to see this dialog that they won't do anything about on each launch of ProtonUp-Qt.
  • If we have a button on the dialog to "resolve" this error by removing the duplicate key, it might be helpful to the user. However, this is tricky! We'd have to do this using File I/O instead of ConfigParser, which feels dirty to me (we can't read with ConfigParser because then it'll error about the duplicate key, and so the cycle continues). This also means we'd have to manage the logic of parsing the sections and such ourselves. Finally, the responsibility would be on us to decide which key to remove. How do we decide this? Do we remove the first, or last instance?

Personally, I think showing a more human-readable dialog to override the existing dialog, which just pulls in the stacktrace (#313), to better inform the user of what actually went wrong, is a step in the right direction. The two problems here are:

• Do we error out still, or continue, and if we choose to continue, are there any potential issues with being alerted to the duplicate key each time we try to interact with the config file?
• Is this even worth exploring? Maybe we'd be better putting effort into refactoring the "About" menu into an "Options" menu, and exposing the config options on the UI. That way, we should be able to avoid duplicate entries in the config file in the first place. Although there's no reason we can't do both.
  • We may want to avoid doing anything UI-heavy until we figure out something for #360, then we'll have a go-forward template for how to do UI stuff. Maybe the About dialog refactor could be our proof-of-concept for moving away from fixed-size dialogs. I should really get around to opening an issue for an Options dialog with a mockup...

[DavidoTek]

This one is straightforward, we can just wrap the function in a try/except DuplicateOptionError block. Show a slightly more user-friendly error. Instead of a stack trace, we could show a dialog and print that some key in the config file is duplicated. In read_update_config_value, we have the section and option, so we could offer a guess to the user as to what key is duplicated in what section.

Feels a bit sketchy, but we could do something like this:

try:
    # config code
except DuplicateOperationError:
    raise Exception("Hello, this is a user friendly error message")

Or ideally just show a message box and then load a default config from constants.py or something.

If we go forward with this, the question becomes how we want to handle this

I should note that the user should know how to edit the config as duplicate keys were introduced by the user before.

but if we could read and remove duplicate keys, that might be an option to do after warning about the duplicate values

Removing the duplicate key would probably be the most user friendly option although we would need to remove both instances as we don't know which one is correct and then let the user know.

Alternatively, there could be a dialog with the buttons: Okay, just delete the config for me and Exit, I will fix it myself.

Is this even worth exploring? Maybe we'd be better putting effort into refactoring the "About" menu into an "Options" menu, and exposing the config options on the UI. That way, we should be able to avoid duplicate entries in the config file in the first place. Although there's no reason we can't do both.

Seems to the the most logical option...

---

I feel like the way to go is:

• Add a user friendly error message as you described
• For now, show a dialog with Delete config (default choice) and I'll fix it myself.
• Later, add UI options for all config entries

[sonic2kk]

I like the "delete" option. This kind of issue should never happen from our side (we always use ConfigParser, and it's unlikely to cause a problem) so I think having the option to just start over or let the user investigate is good.