Skip to content

Commit 5359135

Browse files
ih-codesih-codes
authored
Bugfix FXIOS-14329 The SettingsTableViewController should use the Notifiable protocol to avoid crashing (#31073)
* Separate Notifiable into its own file. * Update SettingsTableViewController and its inheriting types to use Notifiable properly. Inherit from parent implementation as needed.
1 parent 4425a47 commit 5359135

File tree

5 files changed

+96
-87
lines changed

5 files changed

+96
-87
lines changed

‎BrowserKit/Sources/Common/Utilities/Notifiable.swift‎

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -4,62 +4,6 @@
44

55
import Foundation
66

7-
public protocol NotificationProtocol: Sendable {
8-
/// Posts a notification with an optional object and/or userInfo dictionary.
9-
nonisolated func post(name: NSNotification.Name, withObject: Any?, withUserInfo: [AnyHashable: Any]?)
10-
11-
/// Adds an observer.
12-
/// **NOTE**: Do not call this method directly. Use the `Notifiable` helper method `startObservingNotifications` instead.
13-
///
14-
/// Our `Notifiable` protocol relies on this method to provide default implementations.
15-
nonisolated func addObserver(
16-
_ observer: Any,
17-
selector aSelector: Selector,
18-
name aName: NSNotification.Name?,
19-
object anObject: Any?
20-
)
21-
22-
/// Removes an observer.
23-
/// **NOTE**: Do not call this method directly. Use the `Notifiable` helper method `stopObservingNotifications` instead.
24-
///
25-
/// Our `Notifiable` protocol relies on this method to provide default implementations.
26-
nonisolated func removeObserver(_ observer: Any, name aName: NSNotification.Name?, object anObject: Any?)
27-
28-
/// Removes an observer.
29-
///
30-
/// Our `Notifiable` protocol relies on this method to provide default implementations.
31-
nonisolated func removeObserver(_ observer: Any)
32-
33-
/// Combine-based variant for listening to posted notifications from `NSNotificationCenter`.
34-
///
35-
/// Our `Themeable` protocol relies on this method to provide default implementations that automatically clean up
36-
/// observers without having to remove them in the `deinit`, or worry about `nonisolated` handler methods.
37-
func publisher(for name: Notification.Name, object: AnyObject?) -> NotificationCenter.Publisher
38-
}
39-
40-
/// Provides default implementation for `NotificationCenter` conformance to `NotificationProtocol` with default params.
41-
extension NotificationProtocol {
42-
public func post(
43-
name: NSNotification.Name,
44-
withObject object: Any? = nil,
45-
withUserInfo userInfo: [AnyHashable: Any]? = nil
46-
) {
47-
self.post(name: name, withObject: object, withUserInfo: userInfo)
48-
}
49-
}
50-
51-
/// Make NotificationCenter conform to our `NotificationProtocol` protocol. This will allow us to mock the notification
52-
/// center in our tests.
53-
extension NotificationCenter: NotificationProtocol {
54-
public func post(
55-
name: NSNotification.Name,
56-
withObject object: Any? = nil,
57-
withUserInfo userInfo: [AnyHashable: Any]? = nil
58-
) {
59-
self.post(name: name, object: object, userInfo: userInfo)
60-
}
61-
}
62-
637
@objc
648
public protocol Notifiable: AnyObject {
659
/// We must treat `@objc` methods as `nonisolated`, as we cannot guarantee actor isolation when called outside

‎BrowserKit/Sources/Common/Utilities/NotificationProtocol.swift‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at http://mozilla.org/MPL/2.0/
4+
5+
import Foundation
6+
7+
public protocol NotificationProtocol: Sendable {
8+
/// Posts a notification with an optional object and/or userInfo dictionary.
9+
nonisolated func post(name: NSNotification.Name, withObject: Any?, withUserInfo: [AnyHashable: Any]?)
10+
11+
/// Adds an observer.
12+
/// **NOTE**: Do not call this method directly. Use the `Notifiable` helper method `startObservingNotifications` instead.
13+
///
14+
/// Our `Notifiable` protocol relies on this method to provide default implementations.
15+
nonisolated func addObserver(
16+
_ observer: Any,
17+
selector aSelector: Selector,
18+
name aName: NSNotification.Name?,
19+
object anObject: Any?
20+
)
21+
22+
/// Removes an observer.
23+
/// **NOTE**: Do not call this method directly. Use the `Notifiable` helper method `stopObservingNotifications` instead.
24+
///
25+
/// Our `Notifiable` protocol relies on this method to provide default implementations.
26+
nonisolated func removeObserver(_ observer: Any, name aName: NSNotification.Name?, object anObject: Any?)
27+
28+
/// Removes an observer.
29+
///
30+
/// Our `Notifiable` protocol relies on this method to provide default implementations.
31+
nonisolated func removeObserver(_ observer: Any)
32+
33+
/// Combine-based variant for listening to posted notifications from `NSNotificationCenter`.
34+
///
35+
/// Our `Themeable` protocol relies on this method to provide default implementations that automatically clean up
36+
/// observers without having to remove them in the `deinit`, or worry about `nonisolated` handler methods.
37+
func publisher(for name: Notification.Name, object: AnyObject?) -> NotificationCenter.Publisher
38+
}
39+
40+
/// Provides default implementation for `NotificationCenter` conformance to `NotificationProtocol` with default params.
41+
extension NotificationProtocol {
42+
public func post(
43+
name: NSNotification.Name,
44+
withObject object: Any? = nil,
45+
withUserInfo userInfo: [AnyHashable: Any]? = nil
46+
) {
47+
self.post(name: name, withObject: object, withUserInfo: userInfo)
48+
}
49+
}
50+
51+
/// Make NotificationCenter conform to our `NotificationProtocol` protocol. This will allow us to mock the notification
52+
/// center in our tests.
53+
extension NotificationCenter: NotificationProtocol {
54+
public func post(
55+
name: NSNotification.Name,
56+
withObject object: Any? = nil,
57+
withUserInfo userInfo: [AnyHashable: Any]? = nil
58+
) {
59+
self.post(name: name, object: object, userInfo: userInfo)
60+
}
61+
}

‎firefox-ios/Client/Frontend/Settings/ContentBlockerSettingViewController.swift‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ import Foundation
77
import Shared
88
import ComponentLibrary
99

10-
class ContentBlockerSettingViewController: SettingsTableViewController,
11-
Notifiable {
10+
class ContentBlockerSettingViewController: SettingsTableViewController {
1211
private struct UX {
1312
static let buttonContentInsets = NSDirectionalEdgeInsets(top: 12, leading: 0, bottom: 12, trailing: 0)
1413
}
@@ -214,7 +213,9 @@ class ContentBlockerSettingViewController: SettingsTableViewController,
214213
}
215214

216215
// MARK: - Notifiable
217-
func handleNotifications(_ notification: Notification) {
216+
override func handleNotifications(_ notification: Notification) {
217+
super.handleNotifications(notification)
218+
218219
switch notification.name {
219220
case UIContentSizeCategory.didChangeNotification:
220221
ensureMainThread {

‎firefox-ios/Client/Frontend/Settings/NotificationsSettingsViewController.swift‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -158,9 +158,8 @@ final class NotificationsSettingsViewController: SettingsTableViewController, Fe
158158
accessDenied.addAction(settingsAction)
159159
return accessDenied
160160
}
161-
}
162161

163-
extension NotificationsSettingsViewController: Notifiable {
162+
// MARK: Notifiable
164163
func addObservers() {
165164
startObservingNotifications(
166165
withNotificationCenter: notificationCenter,
@@ -169,7 +168,9 @@ extension NotificationsSettingsViewController: Notifiable {
169168
)
170169
}
171170

172-
func handleNotifications(_ notification: Notification) {
171+
override func handleNotifications(_ notification: Notification) {
172+
super.handleNotifications(notification)
173+
173174
switch notification.name {
174175
case UIApplication.willEnterForegroundNotification:
175176
Task {

‎firefox-ios/Client/Frontend/Settings/SettingsTableViewController.swift‎

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -813,7 +813,6 @@ class WithoutAccountSetting: AccountSetting {
813813
}
814814
}
815815

816-
@objc
817816
protocol SettingsDelegate: AnyObject {
818817
@MainActor
819818
func settingsOpenURLInNewTab(_ url: URL)
@@ -823,7 +822,7 @@ protocol SettingsDelegate: AnyObject {
823822
}
824823

825824
// The base settings view controller.
826-
class SettingsTableViewController: ThemedTableViewController {
825+
class SettingsTableViewController: ThemedTableViewController, Notifiable {
827826
private struct UX {
828827
static let tableViewFooterHeight: CGFloat = 30
829828
static let estimatedRowHeight: CGFloat = 44
@@ -864,23 +863,14 @@ class SettingsTableViewController: ThemedTableViewController {
864863
super.viewWillAppear(animated)
865864

866865
settings = generateSettings()
867-
NotificationCenter.default.addObserver(
868-
self,
869-
selector: #selector(syncDidChangeState),
870-
name: .ProfileDidStartSyncing,
871-
object: nil
872-
)
873-
NotificationCenter.default.addObserver(
874-
self,
875-
selector: #selector(syncDidChangeState),
876-
name: .ProfileDidFinishSyncing,
877-
object: nil
878-
)
879-
NotificationCenter.default.addObserver(
880-
self,
881-
selector: #selector(firefoxAccountDidChange),
882-
name: .FirefoxAccountChanged,
883-
object: nil
866+
startObservingNotifications(
867+
withNotificationCenter: NotificationCenter.default,
868+
forObserver: self,
869+
observing: [
870+
.ProfileDidStartSyncing,
871+
.ProfileDidFinishSyncing,
872+
.FirefoxAccountChanged
873+
]
884874
)
885875

886876
applyTheme()
@@ -915,22 +905,17 @@ class SettingsTableViewController: ThemedTableViewController {
915905
return []
916906
}
917907

918-
@objc
919908
private func syncDidChangeState() {
920-
DispatchQueue.main.async {
921-
self.tableView.reloadData()
922-
}
909+
self.tableView.reloadData()
923910
}
924911

925-
@objc
926912
private func refresh() {
927913
// Through-out, be aware that modifying the control while a refresh is in progress is /not/ supported
928914
// and will likely crash the app.
929915
// self.profile.rustAccount.refreshProfile()
930916
// TODO [rustfxa] listen to notification and refresh profile
931917
}
932918

933-
@objc
934919
func firefoxAccountDidChange() {
935920
self.tableView.reloadData()
936921
}
@@ -1085,4 +1070,21 @@ class SettingsTableViewController: ThemedTableViewController {
10851070
setting.accessoryButtonTapped()
10861071
}
10871072
}
1073+
1074+
// MARK: Notifiable
1075+
1076+
func handleNotifications(_ notification: Notification) {
1077+
switch notification.name {
1078+
case .ProfileDidStartSyncing, .ProfileDidFinishSyncing:
1079+
ensureMainThread {
1080+
self.syncDidChangeState()
1081+
}
1082+
case .FirefoxAccountChanged:
1083+
ensureMainThread {
1084+
self.firefoxAccountDidChange()
1085+
}
1086+
default:
1087+
break
1088+
}
1089+
}
10881090
}

0 commit comments

Comments
 (0)