Skip to content

Change scanningDelegate to a weak reference#21053

Merged
crazytonyli merged 3 commits intotrunkfrom
weak-delegate-violation-fixes-3
Jul 18, 2023
Merged

Change scanningDelegate to a weak reference#21053
crazytonyli merged 3 commits intotrunkfrom
weak-delegate-violation-fixes-3

Conversation

@crazytonyli
Copy link
Copy Markdown
Contributor

@crazytonyli crazytonyli commented Jul 11, 2023

Relates to #21050.

This PR fixes a retain cycle between QRLoginScanningCoordinator and QRLoginCameraSession:

WordPress-iOS/WordPress/Classes/ViewRelated/QR Login/Coordinators/QRLoginScanningCoordinator.swift

Lines 8 to 15 in 09143b4

init(view: QRLoginScanningView, parentCoordinator: QRLoginParentCoordinator, cameraSession: QRCodeScanningSession = QRLoginCameraSession()) {
self.view = view
self.parentCoordinator = parentCoordinator
self.cameraSession = cameraSession
}
func start() {
cameraSession.scanningDelegate = self

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 04:24
@crazytonyli crazytonyli self-assigned this Jul 11, 2023
@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 Numberpr21053-f210ed6
Version22.7
Bundle IDcom.jetpack.alpha
Commitf210ed6
App Center Buildjetpack-installable-builds #5334
 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

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 Numberpr21053-f210ed6
Version22.7
Bundle IDorg.wordpress.alpha
Commitf210ed6
App Center BuildWPiOS - One-Offs #6311
 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.

👍 Code changes look good, the scanning delegate methods continue to be triggered and working

@crazytonyli crazytonyli merged commit 00c97fd into trunk Jul 18, 2023
@crazytonyli crazytonyli deleted the weak-delegate-violation-fixes-3 branch July 18, 2023 22:35
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