Fix text change notification observer in ChangeUsernameViewController #21003

crazytonyli · 2023-07-04T04:20:05Z

Relates to #20994.

When the user wants to change their username from ChangeUsernameViewController, the app shows a confirmation alert which has a text field to ask the user to type the new username. The confirmation button in the alert will be disabled first and turn into enabled only when the user types the new username correctly.

Current implementation adds notification observer at the time of creating the alert and unregisters the observer when alert is dismissed. There are a few issues with current implementation, I'll post an inline comments to highlight them. This PR refactors the observing notification approach to register an observer once the view controller is loaded and let iOS SDK to unregister it when the view controller is released.

Test Instructions

Modify this code in "Account Settings" to always be editableUsername, so that we can enter the "change username" screen.
Launch the app from Xcode.
Navigate to "Avatar" -> "Account Settings" -> "Username".
Select or type a new username.
Tap "Save". This is where you'll see the confirmation alert.

The "Change username" alert button should be disabled. The button should become enabled when the text in the text field matches the new username, and should become disable if it doesn't.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
What's described in the test instruction section.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A

crazytonyli · 2023-07-04T04:22:19Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

            DDLogInfo("User cancelled alert")
        })
-        changeUsernameAction = alertController.addDefaultActionWithTitle(Constants.Alert.change, handler: { [weak alertController] _ in


Here is a memory leak in the original implementation: self, the view controller, strongly references changeUsernameAction; The action instance strongly holds onto this closure, this closure strongly captures self.

crazytonyli · 2023-07-04T04:23:27Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

@@ -135,44 +145,50 @@ private extension ChangeUsernameViewController {
                                                preferredStyle: .alert)
        alertController.addAttributeMessage(String(format: Constants.Alert.message, viewModel.selectedUsername),
                                            highlighted: viewModel.selectedUsername)
-        alertController.addCancelActionWithTitle(Constants.Alert.cancel, handler: { [weak alertController] _ in
-            if let textField = alertController?.textFields?.first {
-                NotificationCenter.default.removeObserver(textField, name: UITextField.textDidChangeNotification, object: nil)


I believe the intention is removing the observer that's added when creating this alert. But this line doesn't actually do that: textField is not registered as an observer, not by this view controller at least.

crazytonyli · 2023-07-04T04:24:08Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

                textField.text == self.viewModel.selectedUsername else {
                    DDLogInfo("Username confirmation failed")
                    return
            }
            DDLogInfo("User changes username")
-            NotificationCenter.default.removeObserver(textField, name: UITextField.textDidChangeNotification, object: nil)


This is the same unregistering observer issue as the above comment.

crazytonyli · 2023-07-04T04:24:35Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

-            NotificationCenter.default.addObserver(forName: UITextField.textDidChangeNotification,
-                                                   object: textField,
-                                                   queue: .main) {_ in
-                                                    if let text = textField.text,


This notification observer, which is never unregistered, strongly holds onto textField.

wpmobilebot · 2023-07-04T04:42:26Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21003-69b707a
	Version	22.7
	Bundle ID	org.wordpress.alpha
	Commit	`69b707a`
	App Center Build	WPiOS - One-Offs #6200

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-07-04T04:42:56Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21003-69b707a
	Version	22.7
	Bundle ID	com.jetpack.alpha
	Commit	`69b707a`
	App Center Build	jetpack-installable-builds #5226

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio

I started looking at adding UI tests for this behavior, but it's quite the slog.

Approving in the meantime.

mokagio · 2023-07-04T05:09:48Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

+        NotificationCenter.default.addObserver(
+            self,
+            selector: #selector(handleTextDidChangeNotification(_:)),
+            name: UITextField.textDidChangeNotification,
+            object: nil
+        )


This PR refactors the observing notification approach to register an observer once the view controller is loaded and let iOS SDK to unregister it when the view controller is released.

+1 for using addObserver since, as you point out, iOS handles removing it. As far as I know, the only instance in which one needs to manually remove the observers in the context of an object lifecycle is when they are using blocks

If your app targets iOS 9.0 and later or macOS 10.11 and later, you do not need to unregister an observer that you created with this function. If you forget or are unable to remove an observer, the system cleans up the next time it would have posted to it.

https://developer.apple.com/documentation/foundation/nsnotificationcenter/1415360-addobserver

You must invoke removeObserver: or removeObserver:name:object: before the system deallocates any object that addObserverForName:object:queue:usingBlock: specifies.

https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname

This feels somewhat fragile to me in the context of UIScene, because (IINM) we're capturing the textDidChangeNotification for every UITextField in the currently running app. Is that correct?

If so, this is a good bug fix, but adopts a different kind of technical debt, so I wonder if we ought to approach it slightly differently? 🤔

Good point. There is no need to observe all text change during the lifecyle of this view controller, even though the notification handler does nothing for notifications that it doesn't care about.

I have changed the approach to only observe the text field in the alert: 69b707a.

mokagio · 2023-07-04T05:10:58Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

@@ -31,6 +32,13 @@ class ChangeUsernameViewController: SignupUsernameTableViewController {
        super.viewDidLoad()
        setupViewModel()
        setupUI()
+
+        NotificationCenter.default.addObserver(


What do you think of injecting the NotificationCenter instance? (init(... notificationCenter: NotificationCenter = .default).

Much of a muchness?

Ah, the never-ending "do we DI the iOS SDK's singletons" debate 😂

FWIW I vote we do so that we can inject a NotificationCenter under test – ensures we don't rely on shared resources (and thus can parallelize our tests)

There is no existing unit test for this test, nor the view model class. I had a look at the view model implementation, thinking maybe I move this text observation logic into the view model if that's a sensible thing to do in the current structure. But I couldn't quite figure out the designed interactions between the view controller, view model, and the store object.

jkmassel · 2023-07-04T18:52:13Z

WordPress/Classes/ViewRelated/Me/My Profile/Change Username/ChangeUsernameViewController.swift

@@ -15,7 +15,8 @@ class ChangeUsernameViewController: SignupUsernameTableViewController {
        }
        return saveItem
    }()
-    private var changeUsernameAction: UIAlertAction?
+
+    private weak var confirmationController: UIAlertController?


It's unfortunate that UIAlertController is such a weird bit of API – it'd be great to push down the TextViewDidChange stuff into a UIAlertController subclass (if such a thing were allowed, which it's not for some reason).

I really wonder if adjusting the UI (to present a full-screen page instead of a jammed-up confirmation step) would make this a lot simpler code-wise? Seems like a win on both fronts, tbh.

I created #21006 as an alternative to this PR, using a static function instead of a subclass. But I think the idea is similar: encapsulating the logic within UIAlertController.

mokagio · 2023-07-09T10:50:53Z

@crazytonyli there hasn't been a choice on whether to merge this or #21006.

In the interested of moving forward with the code freeze, I'd suggest merging this implementation and eventually update to get the #21006 changes if so decided—or vice versa. Feel free to choose the version you favor.

cc @oguzkocer as the release manager for 22.8

crazytonyli · 2023-07-09T21:51:15Z

@mokagio Sounds good! These two PRs in their current state don't have too much difference in terms of runtime behaviour.

crazytonyli added 5 commits July 4, 2023 15:40

Move notification handler to its own function

c7b76d9

Update notification handler function

6d286e5

Fix incorrectly unregistered notification observer

6784a64

Fix a retain cycle between view controller and alert action

549f96e

Delete changeUsernameAction property

17c3f69

crazytonyli added the [Type] Bug label Jul 4, 2023

crazytonyli added this to the 22.8 milestone Jul 4, 2023

crazytonyli requested review from jkmassel and mokagio July 4, 2023 04:20

crazytonyli self-assigned this Jul 4, 2023

crazytonyli commented Jul 4, 2023

View reviewed changes

staskus self-requested a review July 4, 2023 07:38

mokagio approved these changes Jul 4, 2023

View reviewed changes

jkmassel reviewed Jul 4, 2023

View reviewed changes

Only observe text field change notification from the alert

69b707a

crazytonyli mentioned this pull request Jul 5, 2023

Move confirmation prompt to a static function #21006

Closed

4 tasks

crazytonyli merged commit db15a47 into trunk Jul 9, 2023

crazytonyli deleted the fix-memory-leak-in-notification-observers-7 branch July 9, 2023 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix text change notification observer in ChangeUsernameViewController #21003

Fix text change notification observer in ChangeUsernameViewController #21003

crazytonyli commented Jul 4, 2023

crazytonyli Jul 4, 2023

crazytonyli Jul 4, 2023

crazytonyli Jul 4, 2023

crazytonyli Jul 4, 2023

wpmobilebot commented Jul 4, 2023 •

edited

Loading

wpmobilebot commented Jul 4, 2023 •

edited

Loading

mokagio left a comment

mokagio Jul 4, 2023

jkmassel Jul 4, 2023

crazytonyli Jul 4, 2023

mokagio Jul 4, 2023

jkmassel Jul 4, 2023

crazytonyli Jul 4, 2023

jkmassel Jul 4, 2023

crazytonyli Jul 5, 2023

mokagio commented Jul 9, 2023 •

edited

Loading

crazytonyli commented Jul 9, 2023

Fix text change notification observer in ChangeUsernameViewController #21003

Fix text change notification observer in ChangeUsernameViewController #21003

Conversation

crazytonyli commented Jul 4, 2023

Test Instructions

Regression Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Jul 4, 2023 • edited Loading

wpmobilebot commented Jul 4, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Jul 9, 2023 • edited Loading

crazytonyli commented Jul 9, 2023

wpmobilebot commented Jul 4, 2023 •

edited

Loading

wpmobilebot commented Jul 4, 2023 •

edited

Loading

mokagio commented Jul 9, 2023 •

edited

Loading