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

Overwrite config.json with player configuration #138

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Dec 4, 2024

We are currently in the process of migrating vegastrike.config (old, XML) to config.json (new, JSON).
As a result, the structure of the new file changes regularly and overwrites whatever changes the player has introduced.
As the first thing we ported was the graphics (resolution, monitor),
this requires the player to manually edit the file after every relevant pull.

Solution

There is a script, conveniently named overwrite_config_defaults, to do this automatically.
Copy all your changes to config.bak, preserving the relevant structure.

Code Changes:

Issues:

  • None

Does not fix Engine/issues/921

@royfalk royfalk requested a review from evertvorster December 4, 2024 09:16
@royfalk royfalk self-assigned this Dec 4, 2024
@evertvorster
Copy link
Contributor

evertvorster commented Dec 4, 2024

Hi there!

Thanks for looking into this. However, this script is still set to operate in a region of the system which is not under the user's control. So, in order to run, it still needs superuser privileges.

As so:

[evert@Evert vegastrike]$ python python/utilities/overwrite_config_defaults.py 
Traceback (most recent call last):
  File "/usr/share/vegastrike/python/utilities/overwrite_config_defaults.py", line 30, in <module>
    with open('config.json', 'w') as json_file: 
         ^^^^^^^^^^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: 'config.json'
[evert@Evert vegastrike]$ 

As with vegastrike.config, would it not make more sense to have the engine check for a config.json in the same places and hierarchy as with vegastrike.config?

If a config.json is found in the user's home directory, then just use the config.json in the user's home directory rather than the one in the system.

This solves a couple of problems:

  1. No elevated user privileges are required.
  2. Since it is not part of the Assets package, it does not get overwritten when Assets are updated.

If you want to be fancy about it, you can put a version number inside the config.json, so that when the items in the config.json changes it would be possible to warn the user that the config file is of an older version and needs to be merged with the newer version from /usr/share/vegastrike.

If you want to be really, really fancy about it, the game engine can check for the existence of ~/.vegastrike/config.json, and if not present, create it from /usr/share/vegastrike/config.json. With the versioning method mentioned before, if the version in ~/.vegastrike is out of date, merge in the newer configuration options and save that. Also, post a message to the console to state that it was updated.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 4, 2024

OK. This is somewhat more complicated than I bargained for.
I believe the issue is fixed for people with sudo (specifically you).
I've detached this PR from the issue.

If this solves the issue for you, let's merge this and revisit the issue again in a few weeks.

@evertvorster
Copy link
Contributor

Hmm, for me it is easier to just have a copy of the config.json somewhere else and just sudo cp it back into position.
Maybe it is better to just forget about this PR and focus on something else.
We can revisit this issue when it becomes an issue for more people.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 5, 2024

Hmm, for me it is easier to just have a copy of the config.json somewhere else and just sudo cp it back into position.

It is now. But in a month or two, you'll have multiple changes to a much larger file.

Don't forget, config.json is supposed to grow rapidly in size.

@evertvorster
Copy link
Contributor

For now, I'll just copy over the file manually. If it changes significantly, I'll copy the new one and make the changes needed in my copy.
At some point in the future the game engine should save the config file in a destination which is not a system directory, and prefer that copy over the system one. However, we can cross that bridge when we get to it.

@BenjamenMeyer BenjamenMeyer added this to the 0.10.x milestone Dec 7, 2024
if isinstance(config_bak[key], dict) or isinstance(config_bak[key], list):
recursive_overwrite(config_json[key], config_bak[key])
else:
config_json[key] = config_bak[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be able to use a Python dict merge here:

dict_a = {}
dict_b = {}
a.update(b)

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.

3 participants