Skip to content

Commit

Permalink
Bugfix FXIOS-9174 [Multi-window] Fix theme bugs (#20373)
Browse files Browse the repository at this point in the history
* [9174] Fix several bugs with theme changes across multiple iPad windows.

* [9174] Comment

* [9174] Minor cleanup for readability

* Fix missing import

* [9174] Cleanup

* [9174] Cleanup and comments

* [9174] Comments
  • Loading branch information
mattreaganmozilla committed May 22, 2024
1 parent d75cea1 commit f97204a
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 27 deletions.
17 changes: 15 additions & 2 deletions BrowserKit/Sources/Common/Theming/DefaultThemeManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,15 @@ public final class DefaultThemeManager: ThemeManager, Notifiable {
guard currentTheme(for: window).type != newTheme else { return }

updateSavedTheme(to: newTheme)
updateCurrentTheme(to: fetchSavedThemeType(for: window), for: window)

// Although we may have only explicitly changed the state on one specific window,
// we want to be sure we update all windows in case the Light/Dark theme changed.
allWindowUUIDs.forEach {
updateCurrentTheme(to: fetchSavedThemeType(for: $0), for: $0, notify: false)
}
// After updating all windows, notify (once). We send the UUID of the window for
// which the change originated though more than 1 window may ultimately update its UI.
notifyCurrentThemeDidChange(for: window)
}

public func reloadTheme(for window: WindowUUID) {
Expand Down Expand Up @@ -215,15 +223,20 @@ public final class DefaultThemeManager: ThemeManager, Notifiable {
userDefaults.set(newTheme.rawValue, forKey: ThemeKeys.themeName)
}

private func updateCurrentTheme(to newTheme: ThemeType, for window: WindowUUID) {
private func updateCurrentTheme(to newTheme: ThemeType, for window: WindowUUID, notify: Bool = true) {
windowThemeState[window] = newThemeForType(newTheme)

// Overwrite the user interface style on the window attached to our scene
// once we have multiple scenes we need to update all of them

let style = self.currentTheme(for: window).type.getInterfaceStyle()
self.windows[window]?.overrideUserInterfaceStyle = style
if notify {
notifyCurrentThemeDidChange(for: window)
}
}

private func notifyCurrentThemeDidChange(for window: WindowUUID) {
mainQueue.ensureMainThread { [weak self] in
self?.notificationCenter.post(name: .ThemeDidChange, withUserInfo: window.userInfo)
}
Expand Down
17 changes: 13 additions & 4 deletions BrowserKit/Sources/Common/Theming/Themeable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,32 @@ public protocol Themeable: ThemeUUIDIdentifiable, AnyObject {
func applyTheme()
}

/// Protocol for views to identify which iPad window (UUID) the view is associated with.
/// By default, all UIViews conform to this automatically. See: UIView+ThemeUUIDIdentifiable.swift.
public protocol ThemeUUIDIdentifiable: AnyObject {
var currentWindowUUID: WindowUUID? { get }
}

/// Protocol that views or controllers may optionally adopt when they provide an explicit (typically, injected)
/// window UUID. This is used by our convenience extensions to allow UIViews to automatically detect their
/// associated iPad window UUID, even if the view is not immediately installed in a window or view hierarchy.
public protocol InjectedThemeUUIDIdentifiable: AnyObject {
var windowUUID: WindowUUID { get }
}

extension Themeable {
public func listenForThemeChange(_ subview: UIView) {
let mainQueue = OperationQueue.main
themeObserver = notificationCenter.addObserver(name: .ThemeDidChange,
queue: mainQueue) { [weak self] _ in
self?.applyTheme()
guard let uuidIdentifiable = subview as? ThemeUUIDIdentifiable else { return }
self?.updateThemeApplicableSubviews(subview, for: uuidIdentifiable.currentWindowUUID)
self?.updateThemeApplicableSubviews(subview)
}
}

public func updateThemeApplicableSubviews(_ view: UIView, for window: WindowUUID?) {
guard let window else { return }
public func updateThemeApplicableSubviews(_ view: UIView) {
guard let window = (view as? ThemeUUIDIdentifiable)?.currentWindowUUID else { return }
assert(window != .unavailable, "Theme applicable view has `unavailable` window UUID. Unexpected.")
let theme = themeManager.currentTheme(for: window)
let themeViews = getAllSubviews(for: view, ofType: ThemeApplicable.self)
themeViews.forEach { $0.applyTheme(theme: theme) }
Expand Down
8 changes: 4 additions & 4 deletions firefox-ios/Client.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
1D2F68AD2ACB266300524B92 /* RemoteTabsPanelState.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68AC2ACB266300524B92 /* RemoteTabsPanelState.swift */; };
1D2F68AF2ACB272500524B92 /* RemoteTabsTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68AE2ACB272500524B92 /* RemoteTabsTableViewController.swift */; };
1D2F68B12ACCA22000524B92 /* RemoteTabsEmptyView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68B02ACCA22000524B92 /* RemoteTabsEmptyView.swift */; };
1D3822E92BAB99250046BC5E /* UIView+Multiwindow.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */; };
1D3822E92BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */; };
1D3C90882ACE1AF400304C87 /* RemoteTabPanelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3C90872ACE1AF400304C87 /* RemoteTabPanelTests.swift */; };
1D4D79462BF2F4E7007C6796 /* SimpleTab.swift in Sources */ = {isa = PBXBuildFile; fileRef = 43E69EAF254D064E00B591C2 /* SimpleTab.swift */; };
1D4D79472BF2F4FD007C6796 /* Throttler.swift in Sources */ = {isa = PBXBuildFile; fileRef = 96D95015270238500079D39D /* Throttler.swift */; };
Expand Down Expand Up @@ -2263,7 +2263,7 @@
1D2F68AC2ACB266300524B92 /* RemoteTabsPanelState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsPanelState.swift; sourceTree = "<group>"; };
1D2F68AE2ACB272500524B92 /* RemoteTabsTableViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsTableViewController.swift; sourceTree = "<group>"; };
1D2F68B02ACCA22000524B92 /* RemoteTabsEmptyView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsEmptyView.swift; sourceTree = "<group>"; };
1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIView+Multiwindow.swift"; sourceTree = "<group>"; };
1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIView+ThemeUUIDIdentifiable.swift"; sourceTree = "<group>"; };
1D3C90872ACE1AF400304C87 /* RemoteTabPanelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabPanelTests.swift; sourceTree = "<group>"; };
1D558A562BED7ECB001EF527 /* MockWindowManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockWindowManager.swift; sourceTree = "<group>"; };
1D558A592BEE7D07001EF527 /* WindowSimpleTabsCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WindowSimpleTabsCoordinator.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -8696,7 +8696,7 @@
E1442FCB294782D8003680B0 /* UIView+Extension.swift */,
E1442FC4294782D7003680B0 /* UIView+Screenshot.swift */,
E1442FCA294782D8003680B0 /* UIView+SnapKit.swift */,
1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */,
1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */,
E1442FC7294782D7003680B0 /* UIViewController+Extension.swift */,
C81A8F2426D3ED1900EBA539 /* UIWindow+Extension.swift */,
E1442FC9294782D8003680B0 /* URL+Mail.swift */,
Expand Down Expand Up @@ -14465,7 +14465,7 @@
2137785F297F3B1B00D01309 /* DownloadsPanelViewModel.swift in Sources */,
EBE26B57220C959D00D1D99A /* BrowserViewController+TabToolbarDelegate.swift in Sources */,
8A3EF7F02A2FCF3100796E3A /* HiddenSettings.swift in Sources */,
1D3822E92BAB99250046BC5E /* UIView+Multiwindow.swift in Sources */,
1D3822E92BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift in Sources */,
B2FEA6912B4661BE0058E616 /* AddressAutofillToggle.swift in Sources */,
B2FEA68F2B460D9E0058E616 /* AddressAutofillSettingsViewModel.swift in Sources */,
AB03032B2AB47AF300DCD8EF /* FakespotOptInCardView.swift in Sources */,
Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/AccessoryViewProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ enum AccessoryType {
case standard, creditCard, address, login
}

class AccessoryViewProvider: UIView, Themeable {
class AccessoryViewProvider: UIView, Themeable, InjectedThemeUUIDIdentifiable {
// MARK: - Constants
private struct UX {
static let toolbarHeight: CGFloat = 50
Expand Down
15 changes: 0 additions & 15 deletions firefox-ios/Client/Extensions/UIView+Multiwindow.swift

This file was deleted.

33 changes: 33 additions & 0 deletions firefox-ios/Client/Extensions/UIView+ThemeUUIDIdentifiable.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import UIKit
import Common
import Shared

extension UIView: ThemeUUIDIdentifiable {
/// Convenience that allows most UIViews to automatically reference their parent window UUID.
/// If the view is installed in a view hierarchy that is part of a window, or is a type
/// that has its UUID injected, this returns the window UUID. If not, returns nil.
public var currentWindowUUID: WindowUUID? {
// If the view has opted in to InjectedThemeUUIDIdentifiable, we prefer that UUID
// since it is injected and should always be correct even if the view isn't installed
// in a window or view hierarchy.
if let injectedUUID = (self as? InjectedThemeUUIDIdentifiable)?.windowUUID {
return injectedUUID
}

// Edge-case: if the view is a UITableView, allow delegates to opt-in to InjectedThemeUUIDIdentifiable.
if let tableView = self as? UITableView,
let uuidProvider = tableView.delegate as? InjectedThemeUUIDIdentifiable {
return uuidProvider.windowUUID
}

// Finally, for most views, we can simply return the associated UUID of the window the
// view is installed in. This covers 99% of use cases, however care needs to be used with
// UI that is not always inserted into a view hierarchy. In those situations, the view
// can opt-in to `InjectedThemeUUIDIdentifiable`.
return (self.window as? BrowserWindow)?.uuid
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2364,6 +2364,8 @@ class BrowserViewController: UIViewController,
urlBar.applyUIMode(isPrivate: isPrivate, theme: currentTheme)
}

toolbar.applyTheme(theme: currentTheme)

guard let contentScript = tabManager.selectedTab?.getContentScript(name: ReaderMode.name()) else { return }
applyThemeForPreferences(profile.prefs, contentScript: contentScript)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import UIKit
import Shared
import Common

class ThemedTableViewController: UITableViewController, Themeable {
class ThemedTableViewController: UITableViewController, Themeable, InjectedThemeUUIDIdentifiable {
var themeManager: ThemeManager
@objc var notificationCenter: NotificationProtocol
var themeObserver: NSObjectProtocol?
Expand Down

0 comments on commit f97204a

Please sign in to comment.