Refactor FXIOS-9331 [Theming] Fix ThemeManager issues #20667
Conversation
adudenamedruby
had a problem deploying
to
github-pages
GitHub Actions
Failure
Client.app: Coverage: 30.74
Generated by 🚫 Danger Swift against e366b5f |
| private func migrateSingleWindowPrivateDefaultsToMultiWindow(for window: WindowUUID) { | ||
| // Migrate old private setting to our window-based settings |
There was a problem hiding this comment.
I think we may be able to remove this migration code and persisted state entirely? IIRC this persisted state was to keep track of private mode between app launches, but now I think we've updated the iOS client to always clear private tabs on launch. And I believe the related setting for that has also been removed. (Though we should double-check, maybe all of this is still behind an experiment. I also have seen several user reviews already complaining about that change so we'd want to confirm with Product whether it's permanent or not.)
But if we have decided 100% to remove all of that, then I think we can also remove this migration and persisted state, since the app will never launch with any windows in Private mode.
I'd definitely like to get confirmation on that from someone who is a bit more in-the-loop on that however. (cc @OrlaM)
There was a problem hiding this comment.
Good point. I can make a note to fully look into this afterward, but I feel like this shouldn't be a blocker for merging this PR. It's easily removed later.
|
This pull request has conflicts when rebasing. Could you fix it @adudenamedruby? 🙏 |
adudenamedruby
force-pushed
the
rgb/9312_themingBugs
branch
from
39d3a2b to
e366b5f
Compare
adudenamedruby
had a problem deploying
to
github-pages
GitHub Actions
Failure
|
When this is approved., could you also merge it, please, as I'm gone for the wknd now. |
There was a problem hiding this comment.
@adudenamedruby I did a quick smoke test and only saw one issue which is the flash of Private theme during launch, but that's already happening on main also AFAICS. Removing the use of user defaults might be a nice follow-up if we can confirm it's no longer needed.
Probably wouldn't hurt to get additional eyes on this just as a sanity check, but otherwise LGTM. 
|
|
||
| var settings: KeyedPrivateModeFlags | ||
| = userDefaults.object(forKey: ThemeKeys.PrivateMode.byWindowUUID) as? KeyedPrivateModeFlags ?? [:] | ||
|
|
||
| settings[window.uuidString] = NSNumber(value: isOn) | ||
| userDefaults.set(settings, forKey: ThemeKeys.PrivateMode.byWindowUUID) | ||
|
|
||
| updateCurrentTheme(to: fetchSavedThemeType(for: window), for: window) | ||
| applyThemeChanges(for: window, using: determineThemeType(for: window)) | ||
| } | ||
|
|
||
| public func getPrivateThemeIsOn(for window: WindowUUID) -> Bool { | ||
| let settings = userDefaults.object(forKey: ThemeKeys.PrivateMode.byWindowUUID) as? KeyedPrivateModeFlags |
There was a problem hiding this comment.
Doesn't need to block the PR but we probably don't need to store this in UserDefaults any longer, and changing it might also fix this issue where the windows are rendering briefly in Private theme incorrectly during launch:
launch_flash.mov
📜 Tickets
Jira ticket
Github issue
💡 Description
OK. So, I was fixing a bug, and then another bug would pop up. I got tired of it, and decided to fix everything in one go by fixing the underlying logic of the theming system.
Changes of note:
determineThemeType(for...is now the heart of the theming system when it comes to deciding what theme needs to be shown instead of managing that in a bunch of different placeswindowThemeState🎉windowthroughout the theming system as much as possible.windowis really only needed for private mode stuff, but it started getting introduced in a bunch of places because of the chain calls. I've just simplified this to only the stuff needed 🥳For review:
The bulk of the meaningful changes are in
DefaultThemeManagerand inThemeMiddleware. Everything else is mostly updates from naming and such.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)