Skip to content

refactor!: rename MediaUploadDelegate to MediaProcessor - #630

Open
jkmassel wants to merge 3 commits into
feat/remove-upload-file-hookfrom
refactor/media-processor-rename
Open

refactor!: rename MediaUploadDelegate to MediaProcessor#630
jkmassel wants to merge 3 commits into
feat/remove-upload-file-hookfrom
refactor/media-processor-rename

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Stacked on #629. Seventh of ten PRs splitting #621. A rename, plus the class-bound relaxation that follows from it — see Breaking change.

What?

MediaUploadDelegateMediaProcessor, mediaUploadDelegatemediaProcessor, the server parameter → processor, the file → MediaHandlers.swift, and Android's demo → DemoMediaProcessor.

Why?

The protocol no longer uploads anything — #629 removed uploadFile, leaving handlesFile and processFile. "UploadDelegate" now describes the one thing it can't do, and next to MediaUploader the two names read as variations on the same job rather than the two halves of a deliberate split.

MediaProcessor says what is left: it transforms bytes, GutenbergKit delivers them.

How?

A rename sweep, ~83 sites across both platforms including the demo apps. Prose in doc comments follows the types, including the property abstracts Xcode Quick Help and Android Studio hover actually render. MediaHandlers.swift because the file holds both protocols now.

The weak_delegate suppression added in #625 goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. The rule keys on the identifier suffix, so it cannot fire on mediaProcessor — the suppression and the paragraph arguing with it are both dead, and nothing in CI would ever have said so.

Following that through: MediaProcessor and MediaUploader also drop : AnyObject. Nothing needed class-boundness — there is no weak, ===, or ObjectIdentifier use against either protocol anywhere in the tree — and EditorViewController holds both strongly, so a class-bound protocol was quietly steering hosts toward a conformer that holds the view controller back and closes a retain cycle ARC cannot break. Dropping it lets a host conform with a value type capturing only what the work needs. Every existing conformer is a class and is unaffected.

One test change rides along. retainsDelegateForServerLifetime named two properties and tested one. The processor was bound to a strong local for the whole do block, so #expect(weakDelegate != nil) was satisfied by that local and never by the server. We could not make it fail against a server that holds nothing: with UploadContext(processor: nil, …) the test passed. It now drops the host's reference before asserting, the way processesForHostReleasedDelegate already does, and the same mutation fails it. The release half was always live and is unchanged.

Testing Instructions

  • iOS Simulator xcodebuild test — 585 + 395 tests green
  • Android :Gutenberg:test green (re-run with --rerun-tasks); Android and iOS demo apps compile
  • SwiftLint 0 violations across 152 files; Detekt clean
  • retainsDelegateForServerLifetime mutation-checked both ways — UploadContext(processor: nil, …) passed the old assertion and fails the new one; no-op'ing releaseConnectionHandler() still fails the release assertion, so that half is untouched

Breaking change

mediaUploadDelegate is now mediaProcessor, and MediaUploadDelegate is MediaProcessor. Conformances need no changes beyond the name.

MediaProcessor and MediaUploader are also no longer AnyObject-bound. Class conformers are unaffected; a host that declared its own weak reference to one of these existentials would need to hold it strongly instead.

Note this relaxes a constraint rather than fixing the cycle outright — a struct that stores the EditorViewController cycles just the same. The doc comments say so rather than implying the type system settles it.

A value-type conformer also carries a caveat a class did not: it is copied on assignment and captured once when the editor begins loading, so mutating your own instance afterwards changes nothing the editor will run, and re-assigning the property to push the new value traps — in release as well as debug. Configure a struct conformer at init and treat it as frozen; if you need settings the host can change mid-session, read them inside processFile through a reference the conformer captures. Documented on both protocols.

@wpmobilebot

wpmobilebot commented Sep 5, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/630")

Built from 43806eb

@jkmassel
jkmassel force-pushed the refactor/media-processor-rename branch from f6a9bac to 2a835c1 Compare September 8, 2026 16:12
@jkmassel
jkmassel force-pushed the refactor/media-processor-rename branch 2 times, most recently from 976ade8 to 3910919 Compare September 9, 2026 16:48
The protocol no longer uploads anything — the previous commit removed
`uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now
describes the one thing it can't do, and next to `MediaUploader` the two
names read as variations on the same job rather than the two halves of a
deliberate split.

`MediaProcessor` says what is left: it transforms bytes, GutenbergKit
delivers them. Mechanical throughout — the property becomes
`mediaProcessor`, the server parameter `processor`, the file
`MediaHandlers.swift` (it holds both protocols now), and Android's demo
`DemoMediaProcessor`. Prose follows the types.

The `weak_delegate` suppression added when the property became strong goes
away with the name: the rule was arguably right that a strongly-held
"delegate" is a smell, and the answer was that this was never a delegate.

BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and
`MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes
beyond the name.
`MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and
`EditorViewController` holds both strongly. A conformer that holds the view
controller back therefore closes a retain cycle ARC cannot break: the editor
is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only
caller — never runs either, and a bound loopback `NWListener` outlives the
editing session.

Nothing needed class-boundness. There is no `weak`, `===`, or
`ObjectIdentifier` use against either protocol anywhere in the tree, and
every existing conformer is a class, which conforms unchanged. Dropping the
requirement lets a host conform with a value type capturing only what the
work needs — the shape that avoids the cycle, and the one a class-bound
`Delegate` discouraged.

This does not make the cycle impossible: a struct that stores the view
controller cycles just the same. The docs say so rather than implying the
type system settles it.
`retainsDelegateForServerLifetime` names two properties and only tested one.
The processor was bound to a strong local for the whole `do` block, so
`#expect(weakDelegate != nil)` was satisfied by that local — the server's
ownership was never what the assertion depended on.

Confirmed by mutation. With `UploadContext(processor: nil, ...)`, so the server
holds no reference to the processor at all, the test **passed**. Nil the host's
reference before the assert — the way `processesForHostReleasedDelegate` at
:557 already does — and the same mutation fails it.

The release half was always live and is unchanged: no-op'ing
`releaseConnectionHandler()` still fails the trailing
`#expect(weakDelegate == nil)`, which is the regression 7124457 added it for.

Pre-existing, from 8827ba4 — the commit that introduced the test to pin the
strong-ownership fix it could not actually detect.
@jkmassel
jkmassel force-pushed the refactor/media-processor-rename branch from 3910919 to 43806eb Compare September 9, 2026 18:40
@jkmassel
jkmassel marked this pull request as ready for review September 9, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android iOS [Type] Breaking Change For PRs that introduce a change that will break existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants