Skip to content

Change SharingAccountViewController.delegate to be a weak reference#21063

Merged
crazytonyli merged 1 commit intotrunkfrom
weak-delegate-violation-fixes-5
Jul 18, 2023
Merged

Change SharingAccountViewController.delegate to be a weak reference#21063
crazytonyli merged 1 commit intotrunkfrom
weak-delegate-violation-fixes-5

Conversation

@crazytonyli
Copy link
Copy Markdown
Contributor

@crazytonyli crazytonyli commented Jul 11, 2023

Relates to #21050.

The delegate property causes a retain cycle between SharingAuthorizationHelper and SharingAccountViewController:

WordPress-iOS/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationHelper.m

Lines 236 to 243 in 48b8a39

SharingAccountViewController *controller = [[SharingAccountViewController alloc] initWithService:self.publicizeService
connections:keyringConnections
existingConnections:[self connectionsForService]];
controller.delegate = self;
// Set the view controller stack vs push so there is no back button to contend with.
// There should be no reason for the user to click back to the authorization vc.
[self.navController setViewControllers:@[controller] animated:YES]; 

flowchart TD
  SharingAuthorizationHelper -->|navController| UINavigationController --> SharingAccountViewController -->|delegate| SharingAuthorizationHelper
Loading

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    None.

  3. 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 crazytonyli added this to the 22.9 milestone Jul 11, 2023
@crazytonyli crazytonyli requested review from jkmassel and mokagio July 11, 2023 22:56
@crazytonyli crazytonyli self-assigned this Jul 11, 2023
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Jul 11, 2023 •
edited
Loading

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21063-068c0a3
Version22.7
Bundle IDorg.wordpress.alpha
Commit068c0a3
App Center BuildWPiOS - One-Offs #6387
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Jul 11, 2023 •
edited
Loading

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21063-068c0a3
Version22.7
Bundle IDcom.jetpack.alpha
Commit068c0a3
App Center Buildjetpack-installable-builds #5412
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus mentioned this pull request Jul 14, 2023
Open
staskus
staskus approved these changes Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

👍 nothing to add

@crazytonyli crazytonyli merged commit a49bbe8 into trunk Jul 18, 2023
@crazytonyli crazytonyli deleted the weak-delegate-violation-fixes-5 branch July 18, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@staskus staskus staskus approved these changes

@jkmassel jkmassel Awaiting requested review from jkmassel

@mokagio mokagio Awaiting requested review from mokagio

Assignees

@crazytonyli crazytonyli

Labels

None yet

Projects

None yet

Milestone

22.9

Development

Successfully merging this pull request may close these issues.

3 participants

@crazytonyli @wpmobilebot @staskus