-
Notifications
You must be signed in to change notification settings - Fork 4
Config editor #35
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
base: development
Are you sure you want to change the base?
Config editor #35
Conversation
…ivesplit hotkey internally. removed YesOrNo enum. added app config default < config file < CLI order. added hotkey conversion for egui.
| let settings = Arc::new(RwLock::new(settings)); | ||
| let mut run = Run::default(); | ||
| run.push_segment(Segment::new("")); | ||
| let mut config = AppConfig::load_app_config(AppConfig::new()); // load saved config into default object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried to run the code in this PR yet, but it looks like you disabled command line parsing of options and also changed the default value for a lot of settings by switching from YesOrNo to bool. Bool defaults to false whereas YesOrNo defaults to Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it looks like you disabled command line parsing of options
command line options are loaded on the next line.
load_app_config creates a new default AppConfig then loads the settings from the config file if there is one.
update_from then replaces any config options with command line options.
So the result is default < config file < command line
also changed the default value for a lot of settings by switching from YesOrNo to bool. Bool defaults to false whereas YesOrNo defaults to Yes
I wasn't aware rust had a default for bool. I typically prefer not to reimplement primitives. Also I'd think false would be a more sane default most of the time. Not everyone is going to have an autosplitter for example.
I'm not committed to it though and can change that back. It does make the config consistent with everything being a quoted string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the result is default < config file < command line
Oh cool. I never realized clap has a way to do that. Yes, that's nicer than the messy hand written way I did it.
I wasn't aware rust had a default for bool.
Yeah, the standard lib defines default for a bunch of numeric types as their zero value. See here: https://doc.rust-lang.org/src/core/default.rs.html#164
I typically prefer not to reimplement primitives.
It's been quite a while since I wrote that code, but I believe I was motivated by not liking the way bool works in clap. I believe bool args are determined by their presence but I wanted to be able to turn them on/off using the precedence that you mentioned. Basically, you should be able to override the config file using the command line. I launch annelid in lots of different configurations using shell scripts, but I'm probably the only one doing that.
Not everyone is going to have an autosplitter for example.
If we need to, we can do a finer grained thing with the defaults by either hand writing the default impl for the AppConfig or maybe clap derive has a way to do it per field.
|
I just pushed to the PR restoring
That should make it easier to work with. I also added a |
Added config editor window. Standardizing on livesplit hotkeys within the app seems to have removed the need for the alt binding as far as I can tell