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

Add vdf_safe_load function #455

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add vdf_safe_load function #455

wants to merge 2 commits into from

Conversation

DavidoTek
Copy link
Owner

Fix #424

Adds a new function vdf_safe_load(vdf_file: str) -> dict to steamutil.py

It opens and parses vdf files using the vdf library. Defective UTF-8 characters are replaced with a unicode question mark symbol. The error is printed in case of other Exceptions and an empty dict is returned.

@sonic2kk
Copy link
Contributor

Is this a bug upstream out of interest? Why exactly is this needed? Just wondering as I have never ran into this issue, and have some games with emojis etc in their names.

I remember a similar issue was related to incorrectly configured system locales. In the past I have also fallen victim to application issues caused by locale problems.

I saw the discussion in the linked issue but I still don't really understand why this is needed.

@DavidoTek
Copy link
Owner Author

There was a similar issue upstream where defective UTF-8 characters would cause the problem. The fix didn't help with vdf.load though. I don't think there is an upstream issue for that yet.

I remember a similar issue was related to incorrectly configured system locales. In the past I have also fallen victim to application issues caused by locale problems.
I saw the discussion in the linked issue but I still don't really understand why this is needed.

It's most likely caused by a misconfigured system and not directly the vdf dependency. The problem is that the vdf file contains invalid UTF-8 characters that cannot be parsed for some reason. The vdf parser will throw an error and crash ProtonUp-Qt. This patch will replace the invalid characters with a valid character (I think �).

There are multiple people who reported the issue, so it seems like a good idea to ensure that it won't crash ProtonUp-Qt.

@sonic2kk
Copy link
Contributor

sonic2kk commented Sep 15, 2024

Okay, I think I understand. So this isn't strictly a problem caused by us or the VDF library, but is probably some system issue. That's okay then, I just wanted to make sure there wasn't a deeper problem.

Is this something we should report upstream? Other applications using the VDF library are probably going to bump into this.

@DavidoTek
Copy link
Owner Author

DavidoTek commented Sep 15, 2024

Is this something we should report upstream? Other applications using the VDF library are probably going to bump into this.

I don't think upstream can do much about it because the unicode-error handling takes place in the open function which is called before the vdf library. vdf.load is passed the file object.
I'm thinking about opening an issue nevertheless in case others encounter a similar issue.


UPDATE: Upstream discussion/issue solsticegamestudios/vdf#2

@sonic2kk
Copy link
Contributor

Ohh you're right, I was thinking that this was called when the VDF library was trying to open it, but I guess this is actually to do with how Python tries to open the file, which it can't do on systems with locale issues.

A number of years ago I encountered locale issues with Protontricks, and some years ago I also encountered locale issues with SteamTinkerLaunch. I think the solution in this PR makes sense, and opening an issue upstream could lead to some discussion around the cause of the problem and some solutions.

@sonic2kk
Copy link
Contributor

I saw the upstream discussion and agree that this is a result of the open() function, but I really wonder why it seems to have only recently become an issue. Maybe Valve changed something, or suddenly more people have incorrect locales? I'm trying to understand if something broke here, but I don't know what could have broke.

How likely is it that Valve changed something that has suddenly made systems with incorrect locales run into this issue, I wonder.


I think this PR covers all of the VDF loads where we would need this from what I can tell. Will take a look at #407 closer to the time to see how it would integrate with this. :-)

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.

ProtonUp-Qt Flatpak ver. 2.9.2 - UnicodeDecodeError
2 participants