Conversation
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
from
c51deae to
f65da42
Compare
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
2 times, most recently
from
b3300d5 to
b3b8e7f
Compare
|
Whisper acceleration option is in experimental settings, must toggle it on in advanced |
|
Tested on M1 Pro. Works correctly when switching between CPU and GPU. |
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
2 times, most recently
from
3016654 to
18652f5
Compare
|
Updated retention history serialization to use An alternative solution would be to use snake_case everywhere and add aliases for lowercase to retain backwards compatibility. Depends on the migration path you want to take. |
|
#1085 will go in first then we will update this I will take a look and test and decide the best path forward. Thanks for the cleanup in general, it's much appreciated, just needs a solid testing. Don't want to break things in the name of cleanup |
|
@ruszabarov we just merged #1085, good to rebase now, ping me when you need some testers! |
|
@ruszabarov, could you do a test. mainly what I want to know, is does this change wipe out existing user settings if you could, go to the current release build, change a bunch of settings and then run this. just making sure everything is backwards compatible. ideally we change as little as possible with regards to the current string names. |
|
@cjpais I set the following settings in the release version, then launched this dev version and the settings were preserved. I used the following values
Also checked the raw Backwards compatibility is something I paid close attention to when making these changes. That is why I went with |
|
@VirenMohindra I could use another set of eyes on the Whisper Accelerator and Ort Accelerator option 🙂 |
src/stores/settingsStore.ts
Outdated
| commands.updateRecordingRetentionPeriod(value ?? "never"), | ||
| model_unload_timeout: (value) => | ||
| commands.setModelUnloadTimeout(value ?? "min5"), |
There was a problem hiding this comment.
nit: is it possible to pull these out and share them? if we ever deviate from never and min5 as defaults, we should have a single source of truth
There was a problem hiding this comment.
I decided to Exclude all the undefined values from AppSettings. undefined is not a legal option for value. The behavior should stay effectively the same since we were doing as boolean, as string, ?? "never", etc before
| let whisper_options = vec![ | ||
| WhisperAcceleratorSetting::Auto, | ||
| WhisperAcceleratorSetting::Cpu, | ||
| WhisperAcceleratorSetting::Gpu, | ||
| ]; |
There was a problem hiding this comment.
nit: if WhisperAcceleratorSetting gains a new variant (like CoreMl), this list won't update and the compiler won't warn. worth either mirroring the ort pattern or adding a comment explaining why these three are always the full set
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
from
f4aff38 to
58022bf
Compare
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
from
58022bf to
efe4979
Compare
|
@cjpais just rebased and fixed all the conflicts. Since we are building a combined list |
ruszabarov
force-pushed
the
unload-setting-typesafety
branch
from
efe4979 to
c348e91
Compare
3 participants
Before Submitting This PR
Human Written Description
Improving type safety of dropdowns with a generic on
DropdownOption, allowing option to be typed instead of plainstrings.Related Issues/Discussions
#1071 (comment)
Testing
Tested all dropdowns for interactivity. Tested some of simple options like overlay position -- works correctly. Biggest risks are the Whisper acceleration option (wasn't able to find it in the settings) and the clipboard options since I had to change rust code for those.
AI Assistance
Used codex 5.4 for
get_available_accelerators()rust function and to add the generic parameterTtoDropdownOptionin typescript.