Change QSettings behavior to work with executables #159
Conversation
Adds `-c/--config-file <filename/dir>` argument to tag_studio.py
|
Changes Necessary to provided expected behavior for executable files when saving/loading QSettings to disk. Defaults QSettings to default behavior as specified in QSettings Docs A config file can be specified with the -c/--config-file argument with the following behavior.
In all cases the location of the settings file used is logged to the console. |
Loran425
changed the title
tagstudio/src/qt/ts_qt.py
Outdated
| if path.is_dir(): | ||
| path = path / "TagStudio.ini" | ||
| self.settings = QSettings(str(path), QSettings.IniFormat) | ||
| logging.info( | ||
| f"[QT DRIVER] Directory provided defaulting to TagStudio.ini in directory, using {self.settings.fileName()}" | ||
| ) |
There was a problem hiding this comment.
I wouldnt do this. The user should provide valid path to existing config file, or it should fail loudly. Imagine the user makes a typo in the path (wrong autocomplete choice etc.) and instead of /path/to/TagStudio.ini, they would type /path/to/Tarkov/, so it would create new empty config there. Most likely not desired behaviour.
There was a problem hiding this comment.
I think the only case that needs to be handled is how to create a .ini file from the command line argument then. If I want to use a new config location then I shouldn't need to go there and manually create the config file, I should be able to point the program at where I want it to save the config and then it creates the blank config file there.
tagstudio/src/qt/ts_qt.py
Outdated
| f"[QT DRIVER] Config File exists, using {self.settings.fileName()}" | ||
| ) | ||
| else: | ||
| if path.suffix == ".ini" and path.parent.is_dir(): |
There was a problem hiding this comment.
I dont understand this condition - path.parent.is_dir() will be always true, no?
But I'd say the same as in comment above - just handle the case where a path to existing file is provided, fail loudly otherwise.
There was a problem hiding this comment.
Not necessarily,
If config_file is passed a path like /home/Loran425/projects/TagStudio/TagStudio.ini the path will always be setup properly from the string but if any of the folders in that chain don't exist then the folder wouldn't exist.
There was a problem hiding this comment.
I see. In that case .exists() would be probably better choice.
There was a problem hiding this comment.
If we cut out the guess work and the file is expected to already exists yes .exists() would 100% be the proper call.
This still cuts out the ability to specify a new config file location from the command line arguments which means a user trying to specify a non-standard location would have to go touch the file prior to running TagStudio with a config flag pointing at that location, this doesn't seem ideal.
I think at this point, I'd agree the guess work needs to go, but I'm not sold on the config file existence requirement.
|
I wouldnt be doing any of the guessing games what the user maybe wants to do when the path is specified but the config doesnt exist there. But maybe other people have different stance on this. |
Removes support for directory as a `--config-file` argument
|
New behavior would be this A config file can be specified with the -c/--config-file argument with the following behavior.
In all cases the location of the config file used is logged to the console. |
I think as long as there's fair warning of there being no config file at the specified path then this should be alright. I think it would strike a balance between not failing silently (even if it isn't loud) as well as keeping the expected behavior of creating a config file at the specified location on a first run or not letting the program fail if the config was removed at some point, in the same way I would expect it to not fail if the config in the default userscope location was removed. |
| QSettings.IniFormat, QSettings.UserScope, "TagStudio", "TagStudio" | ||
| ) | ||
| logging.info( | ||
| f"[QT DRIVER] Config File not specified, defaulting to {self.settings.fileName()}" |
There was a problem hiding this comment.
This should probably have better wording, right now it sounds like the user did something wrong if they didn't specify a config file
| f"[QT DRIVER] Config File not specified, defaulting to {self.settings.fileName()}" | |
| f"[QT DRIVER] Using local config stored in {self.settings.fileName()}" |
see below