-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add FXiOS-15127 [AI Kill Switch] Delete models when translation is toggled off #32995
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: main
Are you sure you want to change the base?
Changes from all commits
15c06df
c1245c8
5089bfd
d91fb2c
c1c15b1
f5ef8e5
31374e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,8 @@ final class TranslationSettingsMiddleware { | |
|
|
||
| case TranslationSettingsViewActionType.toggleTranslationsEnabled: | ||
| let current = prefs.boolForKey(PrefsKeys.Settings.translationsFeature) ?? true | ||
| let newValue = !current | ||
| // TODO: FXIOS-15421 Always configure new setting value instead of toggling pref | ||
| let newValue = action.newSettingValue ?? !current | ||
| prefs.setBool(newValue, forKey: PrefsKeys.Settings.translationsFeature) | ||
| SettingsTelemetry().changedSetting( | ||
| PrefsKeys.Settings.translationsFeature, | ||
|
|
@@ -70,7 +71,16 @@ final class TranslationSettingsMiddleware { | |
| windowUUID: action.windowUUID, | ||
| actionType: TranslationSettingsMiddlewareActionType.didUpdateSettings | ||
| )) | ||
|
|
||
| if !newValue { | ||
| Task { | ||
| await modelsFetcher.resetStorage() | ||
| store.dispatch(TranslationSettingsMiddlewareAction( | ||
| isTranslationsEnabled: newValue, | ||
| windowUUID: action.windowUUID, | ||
| actionType: TranslationSettingsMiddlewareActionType.didResetStorage | ||
| )) | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jumping in with less context on reset storage portion: I may be missing it, but I couldn't find where this was being observed. Sometimes It feels a bit weird to dispatch this action here and have it heavily tied to the settings side of things, maybe we want it closer to the did reset storage method instead? Might be helpful to understand why we need this dispatch though, but I couldn't find it. Also realize, we are probably missing a comment here on why we have the 2 dispatched actions. Discussed on one of @razvanlitianu previous PR is due to an issue with how our toolbar configuration is heavily tied to Toolbar Actions and not general Redux actions. For redux, I was under the impression that we perform the action and anyone that listens would care. So Instead of resetting storage being tied to translation settings, it seems like more related to the model fetcher. Maybe ideally, we have a model fetch middleware that observes the didUpdateSettings action and it calls Sorry for the rambling, just some thoughts. Not a blocker for me if already approved. cc: @razvanlitianu |
||
| case TranslationSettingsViewActionType.toggleAutoTranslate: | ||
| let newValue = !isAutoTranslateEnabled | ||
| prefs.setBool(newValue, forKey: PrefsKeys.Settings.translationAutoTranslate) | ||
|
|
||
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 think we might have an edge case that breaks: user manually turns translations off (pref = false) → then turns kill switch ON → middleware reads pref (false) → sets to true. The pref ends up opposite to what the kill switch intended. In
TranslationSettingsViewActionType.toggleTranslationsEnabledactionlet newValue = !currentjust flips whatever is currently in prefs.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.
yep you are right. I found this logic a bit strange but was just trying to follow the existing pattern. I think it makes more sense to just pass in the bool value we expect to set it to. What do you think?
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.
Yes. That makes sense.