-
Notifications
You must be signed in to change notification settings - Fork 17
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 FileSelect Widget #47
Conversation
b33d5ca
to
4a38dec
Compare
src/lib.rs
Outdated
Some(_) => { | ||
warn!("Unknown / unimplemented value type"); | ||
} |
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.
This should probably be the same case as None
(and even then it seems like both remaining cases should probably be deduplicated. You always have to consider that the settings map may contain a bunch of garbage. The widgets always have higher precedence than what garbage is in the settings map. So you would default to an empty string (or in this case, no path at all).
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.
Changed in the Remove Some(_) case, merge with None case
commit.
src/lib.rs
Outdated
let value = obs_data_get_string(settings, SETTINGS_AUTO_SPLITTER_SETTINGS_FILE_SELECT); | ||
|
||
obs_data_set_string(settings, SETTINGS_AUTO_SPLITTER_SETTINGS_FILE_SELECT, value); |
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.
What does this do? It looks like it sets back the value that we just read?
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 don't really know. I just saw a similar line in the boolean / checkbox case and copied it. I can delete it and re-test stuff.
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 have a branch that changes that here: https://github.com/AlexKnauth/obs-livesplit-one/tree/file-select-test
At some point in the next day or so I'll find time to test it.
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.
In the Remove redundant set=get
I removed this line in both the file-select case and the bool case. I tested it with a contrived auto-splitter that put the "wrong" type in, and it seemed to work just fine, as long as this change goes together with the Remove Some(_) case, merge with None case
commit.
A companion PR to LiveSplit/livesplit-core#748, depends on #46.
Implements the FileSelect widget as a path in the OBS properties. The user can select a file, and the WASI-compatible path to that file gets stored in the settings map.