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.

Sign up for GitHub

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

Change a few delegate properties in Signup to be weak references #21064

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Change a few delegate properties in Signup to be weak references #21064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions WordPress/Classes/ViewRelated/NUX/SignupEpilogueCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import UIKit
import WordPressAuthenticator


protocol SignupEpilogueCellDelegate {
protocol SignupEpilogueCellDelegate: AnyObject {
func updated(value: String, forType: EpilogueCellType)
func changed(value: String, forType: EpilogueCellType)
}
Expand Down Expand Up @@ -37,7 +37,7 @@ class SignupEpilogueCell: UITableViewCell {
private let passwordTopMargin: CGFloat = 16

private var cellType: EpilogueCellType?
open var delegate: SignupEpilogueCellDelegate?
open weak var delegate: SignupEpilogueCellDelegate?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a retain cycle between the SignupEpilogueTableViewController and this SignupEpilogueCell.

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueTableViewController.swift

Line 214 in 48b8a39

cell.delegate = self


// MARK: - UITableViewCell

Expand Down
8 changes: 4 additions & 4 deletions WordPress/Classes/ViewRelated/NUX/SignupEpilogueTableViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import UIKit
import WordPressAuthenticator


protocol SignupEpilogueTableViewControllerDelegate {
protocol SignupEpilogueTableViewControllerDelegate: AnyObject {
func displayNameUpdated(newDisplayName: String)
func displayNameAutoGenerated(newDisplayName: String)
func passwordUpdated(newPassword: String)
Expand All @@ -11,7 +11,7 @@ protocol SignupEpilogueTableViewControllerDelegate {

/// Data source to get the temporary user info, not yet saved in the user account.
///
protocol SignupEpilogueTableViewControllerDataSource {
protocol SignupEpilogueTableViewControllerDataSource: AnyObject {
var customDisplayName: String? { get }
var password: String? { get }
var username: String? { get }
Expand All @@ -21,8 +21,8 @@ class SignupEpilogueTableViewController: UITableViewController, EpilogueUserInfo

// MARK: - Properties

open var dataSource: SignupEpilogueTableViewControllerDataSource?
open var delegate: SignupEpilogueTableViewControllerDelegate?
open weak var dataSource: SignupEpilogueTableViewControllerDataSource?
open weak var delegate: SignupEpilogueTableViewControllerDelegate?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueViewController.swift

Lines 71 to 79 in 48b8a39

override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
super.prepare(for: segue, sender: sender)
if let vc = segue.destination as? SignupEpilogueTableViewController {
vc.credentials = credentials
vc.socialService = socialService
vc.dataSource = self
vc.delegate = self
}

I'm not sure if a view controller would hold strong references to segue instances, which would cause a retain cycle in our case here. But considering how this delegate and dataSource are used, they should be weak references anyways.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I jumped on this memory leak in other investigations.

open var credentials: AuthenticatorCredentials?
open var socialService: SocialService?

Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/ViewRelated/NUX/SignupUsernameTableViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import WordPressAuthenticator
class SignupUsernameTableViewController: UITableViewController, SearchTableViewCellDelegate {
open var currentUsername: String?
open var displayName: String?
open var delegate: SignupUsernameViewControllerDelegate?
open weak var delegate: SignupUsernameViewControllerDelegate?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a retain cycle between SignupUsernameViewController and SignupUsernameTableViewController.

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupUsernameViewController.swift

Lines 31 to 36 in 48b8a39

if let vc = segue.destination as? SignupUsernameTableViewController {
usernamesTableViewController = vc
vc.delegate = self
vc.displayName = displayName
vc.currentUsername = currentUsername
}

open var suggestions: [String] = []
private var service: AccountSettingsService?
private var isSearching: Bool = false
Expand Down
4 changes: 2 additions & 2 deletions WordPress/Classes/ViewRelated/NUX/SignupUsernameViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import SVProgressHUD
import WordPressAuthenticator


protocol SignupUsernameViewControllerDelegate {
protocol SignupUsernameViewControllerDelegate: AnyObject {
func usernameSelected(_ username: String)
}

Expand All @@ -12,7 +12,7 @@ class SignupUsernameViewController: UIViewController {

open var currentUsername: String?
open var displayName: String?
open var delegate: SignupUsernameViewControllerDelegate?
open weak var delegate: SignupUsernameViewControllerDelegate?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This delegate is used in a similar way as #21064 (comment)

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueViewController.swift

Lines 80 to 88 in 48b8a39

if let vc = segue.destination as? SignupUsernameViewController {
vc.currentUsername = updatedUsername ?? epilogueUserInfo?.username
vc.displayName = updatedDisplayName ?? epilogueUserInfo?.fullName
vc.delegate = self
// Empty Back Button
navigationItem.backBarButtonItem = UIBarButtonItem(title: String(), style: .plain, target: nil, action: nil)
}
}

private var usernamesTableViewController: SignupUsernameTableViewController?

// MARK: - View
Expand Down