Refactored AppSettings.vue to enhance readability and the code structure #12
Conversation
Signed-off-by: julianbollig <julian.bollig@tngtech.com>
Signed-off-by: julianbollig <julian.bollig@tngtech.com>
| const modelSettingsWatcher = watchEffect(() => { | ||
| modelSettings.llm = globalSetup.modelSettings.llm; | ||
| modelSettings.sd_model = globalSetup.modelSettings.sd_model; | ||
| modelSettings.lora = globalSetup.modelSettings.lora; |
There was a problem hiding this comment.
Why do we need the modelSettings carbonCopy here at all? I do not see any usage
| }); | ||
|
|
||
| onMounted(() => { | ||
| cancelModelSettingsChange(); |
There was a problem hiding this comment.
I am also failing to understand, why we need to cancel onMounted and why we would need to activate the watcher. Just from dashing through docs I would assume the way the watcher is used looks like "faulty" to me
| cancelModelSettingsChange(); | ||
| }) | ||
|
|
||
| onUnmounted(() => { |
There was a problem hiding this comment.
in doubt, we should probably use onDecativated / onActivated, which are also called onMounted /onUnmounted, but works around some browser keep-alive cache, see https://vuejs.org/guide/built-ins/keep-alive#lifecycle-of-cached-instance
| const globalSetup = useGlobalSetup(); | ||
|
|
||
| const presetModel = reactive<StringKV>(Object.assign({}, toRaw(globalSetup.presetModel))); | ||
| const presetModelChange = ref(false); |
There was a problem hiding this comment.
this variable name alones raises confusion about the concept of changing a presetModel.
I however like that you operated the complete logic out of appSettings, leaving AppSettings much cleaner.
It probably would be beneficial, if we operated out the "modelSettingsState" into a dedicated class outside of global state, but given that we have so far questions about how this should be used (and the future of it) we should definetly not do it right now
Signed-off-by: Florian Esser <florian.esser@tngtech.com>
No description provided.