Skip to content
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

Update app's theme when system theme is updated #3211

Closed
wants to merge 14 commits into from
Closed

Conversation

natharateh
Copy link
Contributor

@natharateh natharateh commented Aug 23, 2019

https://phabricator.wikimedia.org/T227123

  • When system theme changes, we're notified about the change in multiple different methods - traitCollectionDidChange(_:), updateViewConstraints(), viewWillLayoutSubviews() and viewDidLayoutSubviews() - I chose to use traitCollectionDidChange(_:)
  • When we're notified of the change, I update* our theme if 1) the new system theme is different from the previous theme 2) the new app theme would be different from the one that's currently set
    *I don't update the theme right away, instead I broadcast our own Theme.didChangeNotification (we do the same thing when the theme change is initiated by the user in Settings or via Article reading controls) and VCs adjust their themes

Known issues:

  • When the app goes into the background, it gets hit with system notifications for some reason - for example, if the current theme is set to dark, as you background the app, you'll see a notification about a system theme change - as if the theme changed from dark to default and then from default back to dark. Break in adjustTheme, after the guard, to reproduce.
  • When I background the app, go to Settings to change the theme, then look at the app without making it active, all the native views are already themed (even though traitCollectionDidChange wasn't called). The same is not true for webViews - the theme gets fully applied only when the app becomes active again.

@tonisevener tonisevener removed the Beta label Aug 29, 2019
@natharateh natharateh removed the Hold label Aug 30, 2019
@natharateh natharateh changed the base branch from xcode_11 to develop August 30, 2019 14:26
@natharateh natharateh removed the WIP label Aug 30, 2019
@natharateh natharateh added this to the 6.4 milestone Aug 30, 2019
@natharateh
Copy link
Contributor Author

Updated the base branch and the description, ready for review

@joewalsh
Copy link
Contributor

joewalsh commented Sep 3, 2019

WMFAppViewController is able to walk the VC hierarchy and apply the theme to all themeable view controllers (in applyTheme: toNavigationControllers: called by applyTheme:). This way, if applyTheme is called on WMFAppViewController, the whole app gets themed and only one place needs to handle listening for the trait collection change. It looks like since WMFThemeableNavigationController is the rootVC it's the only VC that always gets the trait collection change. It could pass that theme along to WMFAppViewController or the theme theme logic from WMFAppViewController could move into WMFThemeableNavigationController. I have a working proof of concept on the trait_theme branch.

It seems like this should also be a theme setting - either a separate "match system" theme in the list or something that automatically switches to the black theme from whatever the selected theme is. Probably worth discussing with the rest of the team.

@natharateh
Copy link
Contributor Author

Good idea, closing this in favor of trait_theme. A couple things that are missing - adjusting theme on launch and making the initial onboarding themeable.

@natharateh natharateh closed this Sep 3, 2019
@natharateh natharateh mentioned this pull request Sep 3, 2019
@tonisevener tonisevener deleted the dark_mode branch June 15, 2020 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants