Port four low-risk fixes from upstream PR #1379#528
Merged
Conversation
…ained 'editor' was the only IBOutlet on the class declared unsafe_unretained; every sibling outlet (toolbar, splitView, editorContainer, preview, ...) is weak. unsafe_unretained is a raw, non-zeroing pointer: if the MPEditorView is ever deallocated and recreated (window/layout churn), 'editor' keeps pointing at freed memory, and the next message sent to it is a dangling-pointer access, i.e. EXC_BAD_ACCESS. weak makes the pointer nil itself out instead, turning a crash into a safe no-op. Ported from MacDownApp/macdown@05d5153 by Roberto Bissanti. Related to #1379.
The 1-arg -insertText: has been deprecated since macOS 10.11 in favor of the 2-arg -insertText:replacementRange:. Six call sites still used the old form (five in NSTextView+Autocomplete.m, one in MPDocument's -insertNewParagraph:), the last stragglers in a file that otherwise already uses the 2-arg API. Each now passes NSMakeRange(NSNotFound, 0), Apple's documented sentinel for "use the current selection, or the marked (IME composition) range if there is one" -- preserving the exact behavior the deprecated call had. Adds MPInsertTextReplacementRangeTests with six tests, one per converted site. On the current SDK (macOS 15.6.1 / Xcode 16.1) the deprecated 1-arg form funnels through our 2-arg override but passes the live selection as the replacement range rather than NSNotFound; the tests discriminate on that, failing before the fix and passing after. Ports MacDownApp/macdown@2b09e4e by Roberto Bissanti. Related to #1379
The `location` and `supportText` outlets in the Terminal preferences xib are leftovers from a code-side rename to `locationTextField`/ `supportTextField` that was never mirrored in the nib. Both pointed at the same destination views as the correctly-named outlets that are still wired, so they were harmless -- but each logged a "Failed to connect outlet ... missing setter or instance variable" warning on every nib load. Removing them silences that noise. Ports MacDownApp/macdown@604664b by Roberto Bissanti. Related to #1379
Two things, both rooted in the Terminal preferences pane's Homebrew check. First, the modernization ported from upstream: the controller used the pre-GCD -readToEndOfFileInBackgroundAndNotify + NSNotificationCenter pattern, whose background read is scheduled at init on a QoS the app doesn't control -- the kind of priority-inversion Xcode's Thread Performance Checker flags as a Hang Risk when a Preferences pane switch happens mid-flight. Rewrote it around NSTask.terminationHandler, dispatching completion back to the main queue (the caller mutates AppKit UI directly), and broke the resulting terminationHandler retain cycle by niling it as the block's first statement under a scoped -Warc-retain-cycles pragma. Second, a latent bug the modernization surfaced: launchPath was the bare relative string "brew". NSTask never consults PATH, so -launch threw NSInvalidArgumentException every time and the caller silently fell into its "Homebrew not installed" path -- detection has never actually worked. Resolve an absolute path (/opt/homebrew/bin/brew, then /usr/local/bin/brew) before launching, and adopt the non-deprecated executableURL + -launchAndReturnError: instead of the deprecated -launch/@try-catch. All three outcomes (not found, launch failure, success) now deliver the handler on the main queue. Adds MPHomebrewSubprocessControllerTests (success delivers output on the main thread, not-found delivers nil on the main thread even when invoked from a background queue, and the terminationHandler cycle-break holds) via a small test-only seam that overrides brew-path resolution with /bin/echo so the tests are deterministic without a real Homebrew install. Ports MacDownApp/macdown@9e62b44 and @c807ff7 by Roberto Bissanti; the launchPath fix is additional. Related to #526 Related to #1379
Documents the four fixes ported from upstream MacDownApp/macdown PR #1379: the weak-outlet crash fix, the -insertText:replacementRange: migration, the Homebrew-detection fix, and the stale Terminal-preferences outlet cleanup. Related to #1379
Contributor
Code Coverage ReportCurrent Coverage: 0.00% Coverage Details (Summary) |
Two test-only categories still redeclared `editor` as `unsafe_unretained`, left over from before the primary `MPDocument.editor` property became `weak`. Harmless at runtime (ARC synthesizes accessors from the primary declaration, and the tests hold their own strong references to the editor), but the stale attribute misrepresents the property's actual ownership. Match `(weak)`. Related to #1379
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports four low-risk fixes triaged from upstream MacDownApp/macdown#1379 — Roberto Bissanti's Sparkle-modernization branch (GitHub @wltb). Each fix was reviewed cold independently before landing; the full suite is green (1074 tests, 0 unexpected).
Fixes
6494c9005d5153MPDocument.editorIBOutlet wasunsafe_unretained; nowweak.806f6ef2b09e4e-insertText:→-insertText:replacementRange:at 6 call sites, with 6 new tests.20ba9169e62b44+c807ff7MPHomebrewSubprocessController(NSTask.terminationHandler, main-queue delivery, retain-cycle break) and fix a real bug — see note below.4300766604664blocation,supportText) that logged a "Failed to connect outlet" warning on every nib load.Fix 3 is scope-upgraded (with approval)
Reviewing the Homebrew modernization surfaced a latent bug:
launchPath = @"brew"is a bare relative path, andNSTasknever searchesPATH, so-launchthrewNSInvalidArgumentExceptionevery time — the Terminal preferences pane's Homebrew detection has never actually worked (it always reported "not installed"). Beyond the faithful port, this now resolves Homebrew's absolute path (/opt/homebrew/bin/brew, then/usr/local/bin/brew) and adopts the non-deprecatedexecutableURL+-launchAndReturnError:. All three completion paths (not-found, launch-failure, success) deliver the handler on the main queue, since the caller mutates AppKit UI directly.This is a user-visible behavior change: the Terminal preferences pane will now correctly detect an installed Homebrew.
Testing
XCTExpectFailurecases).-insertText:replacementRange:migration; 3 for the Homebrew controller (success delivers output on the main thread; not-found delivers nil on the main thread even when invoked off-main; the terminationHandler retain-cycle break holds) — the latter use a test-only seam stubbing brew resolution with/bin/echofor determinism without a real Homebrew install.Suggested follow-up (not in this PR)
Two test-only categories still redeclare
editorasunsafe_unretained(MacDownTests/MPDocumentLifecycleTests.m:27,MacDownTests/MPScrollSyncTests.m:29), stale since the weak-outlet fix. Confirmed harmless (ARC uses the primary class'sweaksynthesis and the tests hold strong locals) but misleading — worth a one-line cleanup.Ports by @wltb (Roberto Bissanti).
Related to #1379
Related to #526