Skip to content

Fix retain cycles in Site Creation#21066

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

Fix retain cycles in Site Creation#21066
crazytonyli merged 1 commit intotrunkfrom
weak-delegate-violation-fixes-7

Conversation

@crazytonyli
Copy link
Copy Markdown
Contributor

@crazytonyli crazytonyli commented Jul 11, 2023

Relates to #21050.

I'll add PR comments regarding the two retain cycles.

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 23:13
@crazytonyli crazytonyli self-assigned this Jul 11, 2023
crazytonyli
crazytonyli commented Jul 11, 2023
View reviewed changes
...asses/ViewRelated/Site Creation/Design Selection/Preview/TemplatePreviewViewController.swift
@IBOutlet weak var progressBar: UIProgressView!

internal var delegate: TemplatePreviewViewDelegate?
internal weak var delegate: TemplatePreviewViewDelegate?
Copy link
Copy Markdown
Contributor Author

@crazytonyli crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

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

LayoutPreviewViewController retains self: delegate = self

WordPress-iOS/WordPress/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPreviewViewController.swift

Lines 3 to 11 in 48b8a39

class LayoutPreviewViewController: TemplatePreviewViewController {
let completion: PageCoordinator.TemplateSelectionCompletion
let layout: PageTemplateLayout
init(layout: PageTemplateLayout, selectedPreviewDevice: PreviewDevice?, onDismissWithDeviceSelected: ((PreviewDevice) -> ())?, completion: @escaping PageCoordinator.TemplateSelectionCompletion) {
self.layout = layout
self.completion = completion
super.init(demoURL: layout.demoUrl, selectedPreviewDevice: selectedPreviewDevice, onDismissWithDeviceSelected: onDismissWithDeviceSelected)
delegate = self

crazytonyli
crazytonyli commented Jul 11, 2023
View reviewed changes
WordPress/Classes/ViewRelated/Site Creation/Site Segments/SiteSegmentsStep.swift
}()

var delegate: WizardDelegate?
weak var delegate: WizardDelegate?
Copy link
Copy Markdown
Contributor Author

@crazytonyli crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

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

A retain cycle between WizardNavigation and SiteSegmentsStep

WordPress-iOS/WordPress/Classes/ViewRelated/Site Creation/Wizard/WizardNavigation.swift

Lines 47 to 51 in 48b8a39

private func configureSteps() {
for var step in steps {
step.delegate = self
}
}

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Jul 11, 2023

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 Numberpr21066-1ecffdc
Version22.7
Bundle IDorg.wordpress.alpha
Commit1ecffdc
App Center BuildWPiOS - One-Offs #6310
 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

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 Numberpr21066-1ecffdc
Version22.7
Bundle IDcom.jetpack.alpha
Commit1ecffdc
App Center Buildjetpack-installable-builds #5333
 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 17, 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.

The code changes look good.

  • Wizard delegate during side creation continues to be working ✅
  • TemplatePreview delegate during page creation continues to be working ✅

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