steamutil: Move shortcuts check inside try block, add logging for VDF file loading

[sonic2kk]

Related to #226.

This moves the if not no_shortcuts check inside the try block to prevent a crash if c is not assigned. If something inside the try block fails early on, the libraryfolders VDF (v) or config VDF (c) variables may not be set appropriately and so a crash will occur when trying to reference c before it is assigned a value - If v throws an exception then c will never be set, or c is not loaded properly it could also be None. If the VDF library can read it though it should always return an empty dict, but if any of the gets fail it could end up being None I think (not sure, never had an exception get thrown with chained gets, not sure if it will just be None, take on the last return value, or if it will be unassigned thus triggering the crash).

This also adds some logging for the VDF file loading. Since it's come up a couple of times now that these files aren't parsed correctly it would be useful to see which file can't be loaded. This log message will get triggered each time the user switches to the Steam utility but for most users this should be invisible, it might just end up being "noisy" for those of us that run from the command line often. So perhaps the logging is less desirable but I can remove it. If it is desired I figured it would be better to catch that in this PR too.

---

The root cause of #226 is still likely that a Steam file is corrupted or otherwise invalid, but this should prevent the crash in such a scenario (the Steam games list will probably still be blank though).

[DavidoTek]

Thanks and also thanks for doing the investigation for # 226! :tada:

This moves the if not no_shortcuts check inside the try block

I wonder if we should put the if not no_shortcuts: ... inside the else: part of the try-except. I think it is the correct way of doing it in Python:

try:
    c = ...
except:
    ...
else:
    if not no_shortcuts:
        ...

(PS: I noticed I've accidentally used finally instead of else in some places. I will correct that :) see #229 )

Since it's come up a couple of times now that these files aren't parsed correctly it would be useful to see which file can't be loaded

Can we change it to print a message only if loading fails?

[sonic2kk]

Moved the shortcuts check into the else block - Just looked it up and I agree, didn't even realise Python had this syntax :sweat_smile:

Can we change it to print a message only if loading fails?

So I thought about this and realised a couple of things:

• To do this we'd probably have to initialize v and c to None so we can check it in the else/exception block, but then that itself would fix the referenced-before-assignment problem, though it still doesn't hint at when v or c might be None. I am not sure how best to go about this, I would rather a solution that avoids initialising them both to None
• open() will already log if the file couldn't be opened, see #228 (No such file or directory: '$HOME/.local/share/Steam/config/libraryfolders.vdf)

So for now I removed it altogether, but I am open to improvements on where/how to display this :-) (A Python debugger could do it I guess...)

[sonic2kk]

As mentioned in https://github.com/flathub/net.davidotek.pupgui2/pull/13, it may be worth adding .get('Valve', 'valve') for cases where the key has a lowercase v for some reason i.e. #226. On all my machines it is Valve, but this user has reported the games list being blank in previous ProtonUp-Qt versions, so this is not new behaviour for them. And Steam overwrites any change made to that file to have a lowercase v.

I am not sure if the rest of the config.vdf file format may be different, though the user in #226 reported that changing the key fixed the issue until Steam overwrote the value.

[DavidoTek]

So for now I removed it altogether, but I am open to improvements on where/how to display this :-)

Okay. If there is a key error, it should print the exception anyway. With the approach below, we should see in which half of the get-chain the error is.

it may be worth adding .get('Valve', 'valve')

I don't think that would work as the second parameter is the fallback value. What we could do is:

c1 = vdf.load(open(config_vdf_file)).get('InstallConfigStore').get('Software')
if 'Valve' in c1:
    c = c1.get('Valve').get('Steam').get('CompatToolMapping')
elif 'value' in c1:
    c = c1.get('valve').get('Steam').get('CompatToolMapping')
else:
    raise Exception('Error! config.vdf/InstallConfigStore/Software neither contains key "Valve", nor "valve"')

[sonic2kk]

I don't think that would work as the second parameter is the fallback value.

You're right, that was silly of me 😅

The snippet you posted would work, I wonder if there's a better way just in general, as mentioned in another issue perhaps there's a better way to use get. Maybe creating some dedicated class to manage VDF data and giving it a get method but that's probably way overkill for simply managing a dict.

We may even just be able to call the variables c instead of using c1 but I'm not sure, didn't test yet :-)

[DavidoTek]

You're right, that was silly of me sweat_smile

I guess the syntax of try-except-else-finally is quite strange in comparison to other languages. Even I accidentally confused else with finally and needed to fix that in #229.

Maybe creating some dedicated class to manage VDF data

The simplest thing to do would be to add a nested_get function to util.py. I would accept a path like InstallConfigStore/Software/... and an optional parameter ignore_case which would match the first key ignoring the case, though setting that one to True probably would increase the time it takes to find and return a value drastically (The only solution I can think of to do this is to loop through all dict key and do key.lower() == searched_key.lower()).

We may even just be able to call the variables c instead of using c1 but I'm not sure, didn't test yet :-)

That should work, I just did it this way to make it more obvious what it does.

[sonic2kk]

Hmm, when I was looking into adding the block mentioned, I noticed that there are a few places where the chained gets are used to get the CompatToolMapping, which is under the Valve key. So I tried to write a generic function to do what we discussed:

def get_steam_vdf_compat_tool_mapping(vdf_file: dict):

    c = vdf_file.get('InstallConfigStore').get('Software')

    # Sometimes the key is 'Valve', sometimes 'valve', see #226
    c = c.get('Valve') or c.get('valve')
    if not c:
        raise KeyError('Error! config.vdf InstallConfigStore.Software neither contains key "Valve" nor "valve" - config.vdf file may be invalid!')

    return c.get('Steam').get('CompatToolMapping')

Since get does not raise an exception, we can use an or here to check for Valve or valve as a fallback. After this or, if c is None, we can raise the error. If the key does exist, we go through the rest of the gets.

I broke the gets down in this way because I thought it would be easier to manage, should the format of this file ever change (which I would guess it won't drastically change, but I also don't work for the big V :sweat_smile:). It would be possible to change it to vdf_file.get('InstallConfigStore').get('Software').get('Valve') or vdf_file.get('InstallConfigStore').get('Software').get('valve') though.

It should be possible to use this function like so:

v = vdf.load(open(libraryfolders_vdf_file))
c = get_steam_vdf_compat_tool_mapping(vdf.load(open(config_vdf_file)))

I have done some light testing on my PC and behaviour appears to be the same, though further testing is a good idea as this is a fairly significant change to how the config.vdf is loaded.

[DavidoTek]

I thought it would be easier to manage, should the format of this file ever change

Yes, that makes it more flexible indeed.

It would be possible to change it to vdf_file.get('InstallConfigStore').get('Software').get('Valve') or

I think it is good as you did. That should also make it more clear what the problem is if they should ever change the format again.

I have done some light testing on my PC and behaviour appears to be the same, though further testing is a good idea as this is a fairly significant change to how the config.vdf is loaded.

I will test it and then merge.

Thanks! :tada: