Skip to content

Fix popup hotkey re-registration leak#1456

Open
GuangDai wants to merge 1 commit into
p0deje:masterfrom
GuangDai:fix/popup-hotkey-leak
Open

Fix popup hotkey re-registration leak#1456
GuangDai wants to merge 1 commit into
p0deje:masterfrom
GuangDai:fix/popup-hotkey-leak

Conversation

@GuangDai

@GuangDai GuangDai commented Jul 17, 2026

Copy link
Copy Markdown

Summary

  • keep the popup global shortcut registered for the process lifetime
  • route complete shortcut presses through the existing popup state machine
  • keep only the flagsChanged monitor needed for release-to-select behavior

Root cause

Opening the popup disabled .popup, and closing it enabled the shortcut again. In KeyboardShortcuts 2.0.2, that pair unregisters the Carbon hotkey and registers a new one. The Carbon backing allocations accumulated across popup open/close cycles.

Keeping the global registration alive removes that allocation cycle. Toggle, ordinary repeated presses, and release-to-select continue to use the existing popup state machine.

Known test difference

testOpenAndSelectThirdItemRepeatedPress posts multiple synthetic key-down events without intervening key-up events. With the Carbon hotkey left registered, those extra raw events do not reach the local AppKit monitor, so that test remains red. The minimal leak fix is intentionally kept here; compatibility with that synthetic repeat sequence can be handled separately if it reflects required runtime behavior.

Validation

  • ported from the fork's measured fix (04298ab2), which observed 783 live Carbon hotkey allocations using 43.5 MB after extended use
  • verified against the pinned KeyboardShortcuts 2.0.2 source: disable unregisters, enable calls RegisterEventHotKey, and handled hotkey events return noErr
  • independently AI-reviewed against current upstream master
  • git diff --check passes
  • the branch contains one production-code file and no docs or CI changes

@GuangDai

GuangDai commented Jul 17, 2026

Copy link
Copy Markdown
Author

While working on the preview path in my fork, I ran into three separate and important preview/UI performance issues. They remain out of scope for this PR, but I want to record them as possible follow-ups.

I rechecked the commit history through the final fork snapshot immediately before the Swift 6 / strict-concurrency migration. The actual final chains were:

  1. Large text can trigger a SwiftUI/CoreText render and layout storm. The final pre-migration state bounded preview text with a configurable limit (default 3,000 characters; 0 maps to effectively full text) (final text-limit implementation). One correction to my earlier wording: the experiment that removed the history-items transition (8700a5a3) was explicitly reverted (fc60568f) because it increased peak stalls. The layout-driving animation that ultimately had to go was the preview window's animated frame-resize path; the fork replaced it with one immediate frame update plus an opacity-only fade (463feb15).

  2. High-resolution images should be downsampled for the preview viewport. The final path used an ImageIO downsampling primitive (3a5241ac) through an off-main ImageProcessor actor and decorator integration (actor, integration, shared processor/cache). Preview decode was capped at an 800-pixel longest side by default (545d4d46) and then made configurable (36ab3b0d); 0 means screen-sized rather than decoding a huge native-resolution bitmap.

  3. Rapid mouse movement can make the preview chase the hovered selection and visibly jitter. The final solution was layered: hover selection stopped driving scrollTo / LazyVStack layout invalidation (03f8f67a); selection changes cancelled stale preview decodes (3599b2fb); the displayed previewedItem was decoupled from the live leadHistoryItem, with cancel-on-change retargeting/debounce (ee2cd34d); and preview geometry changed instantly with only a composited opacity fade (463feb15). The final default retarget delay was 200 ms and remained configurable down to 0 (a9ddf3fb).

The fork has diverged substantially since these changes landed. These commits document behavior that survived to the final pre-migration snapshot; they are references and evidence, not patches that can be cherry-picked. Each issue would need to be revisited independently and adapted into a small upstream-compatible change.

@GuangDai
GuangDai force-pushed the fix/popup-hotkey-leak branch 3 times, most recently from e550b6a to be842f4 Compare July 17, 2026 11:25
Avoid disabling and re-enabling the popup shortcut for every open/close cycle. KeyboardShortcuts registers a new Carbon hotkey when re-enabled, causing the backing allocations to accumulate. Route repeated popup shortcut presses through the existing popup state machine while keeping the process-wide registration alive.
@GuangDai
GuangDai force-pushed the fix/popup-hotkey-leak branch from be842f4 to 29d4454 Compare July 17, 2026 11:34
@GuangDai

Copy link
Copy Markdown
Author
CleanShot 2026-07-17 at 19 53 49

If we can deal with these three problems, we can make maccy much smoother like my branch.

@GuangDai
GuangDai marked this pull request as ready for review July 17, 2026 11:58
@GuangDai

Copy link
Copy Markdown
Author

One more candid note after checking the current upstream path: OCR is eager, not search-triggered. Every accepted clipboard entry is passed through generateTitle() (clipboard ingest); if the item contains an image, that immediately schedules a .fast VNRecognizeTextRequest (image branch, Vision request). This happens before history deduplication, so duplicate image copies can also start OCR, and changing showSpecialSymbols calls generateTitle() again for every current item (settings update path). There is no OCR enable/disable setting or lazy search-time trigger.

Honestly, I do not think OCR earns its cost as an always-on built-in clipboard feature. My expectation—not a measured statistic—is that in well over 90% of image clipboard usage, users will never need to find those images by recognized text. Spending compute and energy on every image copy for a rarely used capability looks like a poor resource trade-off to me. For that reason I removed the Vision OCR image-title feature from my fork. If OCR remains upstream, I think it should at least be opt-in and/or lazy rather than running for every copied image.

@weisJ

weisJ commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Do you have any data indicating that hotkey registration causes actual significant memory usage?

  1. Large text can trigger a SwiftUI/CoreText render and layout storm. [...]

In my experience there is no significant lag during search/delete/pin etc. Can you give an example, where this is the case for you. The animation exists very deliberately.

  1. High-resolution images should be downsampled for the preview viewport. [...]

What is the rational behind this proposed change? SwiftUI already handles downsampling images under the hood.

  1. Rapid mouse movement can make the preview chase the hovered selection and visibly jitter. [...]

Again, do you have actual evidence that this is a problem? Usually SwiftUI already handles coalescing layout invalidation quite well.

One more candid note after checking the current upstream path: [...]

Honestly, I do not think OCR earns its cost as an always-on built-in clipboard feature. [...]

OCR happens on a background thread. Having OCR run before deduplication is also not a very urgent issue as images in the clipboard are near impossible to be deduplicated anyways. As above, do you have data supporting image OCR taking up significant resources? With the next update the OCR text from images can directly be copied from the UI, which - in my opinion - makes this a valuable feature.
Though, if you provide a PR adding a toggle to the settings, I generally don't see a problem why it couldn't be merged. If you do please make sure to provide translations for any added strings appearing in the UI.

@GuangDai

Copy link
Copy Markdown
Author
CleanShot 2026-07-18 at 08 39 18

large text cause freeze.

large text: you can generate any large text using chatgpt.

example file:

large text.txt

sample file here

Maccy.sample.txt

And memory leak. If you used for a long time (10+ hours) and always trigger with shortcuts, then run leak $PID(Maccy's PID). You will find the evidence.

@GuangDai

Copy link
Copy Markdown
Author

And my fork.

CleanShot 2026-07-18 at 08 48 30

Maccy.sample.txt

@GuangDai

GuangDai commented Jul 18, 2026

Copy link
Copy Markdown
Author

Then there's the issue of downsampling. I'm not familiar with Swift features; I just followed my previous development approach and did it directly. So it might be incorrect.

But I implemented multilayer cache. So I store downsampled image in memory, and leave origin image in disk to save memory.

Next, regarding OCR, I prefer that if a feature isn't used but runs frequently, then it's unnecessary. So I removed it. Of course, this is just a personal habit of mine and my friend's; I'm not sure how many people actually use OCR. But I still maintain my opinion that this feature is unnecessary.

@GuangDai

GuangDai commented Jul 18, 2026

Copy link
Copy Markdown
Author

Problem 3 and Problem 1 are directly related; improving Problem 1 will greatly alleviate Problem 3.

However, the behavior will differ from my current fork. In my fork, I made the following modifications based on my personal habits:

  1. Original: As long as you trigger Preview, moving the mouse, even briefly skipping an unrelated item, will still trigger Preview.

  2. Mine: Triggering Preview and then moving the mouse will only refresh the preview area if it hovers over an item for more than the configured preview time (I set 200ms as default).

@GuangDai

GuangDai commented Jul 18, 2026

Copy link
Copy Markdown
Author

And the memory leak issue ,as you've seen, the app performs terribly with large text, and I frequently need to use ChatGPT for my studies. Furthermore, verifying a clear memory leak requires a long run, so I'm temporarily unwilling to provide this evidence myself, as it significantly impacts usability.

@GuangDai

GuangDai commented Jul 18, 2026

Copy link
Copy Markdown
Author

Maccy-dry-run-818f03d-2.6.1-60.zip

my fork app. If you worry about any safety, security issue with this app. (I know there was a fake maccy) You can fork my fork, check it with AI, and run github workflow to build yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants