-
Notifications
You must be signed in to change notification settings - Fork 38
Move pendingInput code to OnitPanelState #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move pendingInput code to OnitPanelState #369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR refactors the architecture for handling highlighted text by introducing a delegate pattern to decouple the HighlightedTextManager from OnitPanelState. Previously, HighlightedTextManager directly accessed PanelStateCoordinator.shared.state to update pendingInput properties, creating tight coupling between components.
The changes introduce a new HighlightedTextDelegate protocol that defines a single method highlightedTextDidChange(selectedText:application:). The HighlightedTextManager now maintains a list of delegates and notifies them when highlighted text changes, rather than directly manipulating panel states. Each OnitPanelState instance implements this delegate protocol and registers itself during initialization via setupHighlightedTextDelegate().
When highlighted text changes, the OnitPanelState receives the notification and handles its own input state management logic. It creates an Input object from the selected text and updates either pendingInput or trackedPendingInput based on the user's autoAddHighlightedTextToContext preference setting. This moves the responsibility for input state management from the text manager to the panel state itself.
The QuickEditManager was also updated to use highlightedTextManager.selectedText directly rather than accessing the derived state through PanelStateCoordinator, simplifying the logic and removing another coupling point.
This architectural improvement follows better separation of concerns principles, making the HighlightedTextManager focused solely on text selection detection while each OnitPanelState owns its input management logic. The refactoring also enables highlighted text to be available across all panel instances in tethered mode, which the developer considers better UX.
Confidence score: 3/5
• This refactoring improves architecture but has a potential memory leak issue that needs attention
• The delegate registration in OnitPanelState initializers lacks corresponding cleanup in deinit, which could cause memory leaks or stale references as panel states are created and destroyed
• The HighlightedTextManager.swift file needs closer review for proper delegate lifecycle management
5 files reviewed, no comments
4f44d03 to
4a585a0
Compare
macos/Onit/StateManagers/HighlightedText/HighlightedTextManager.swift
Outdated
Show resolved
Hide resolved
65fa8db to
e85110a
Compare
4a585a0 to
6b9d25c
Compare
lk340
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Loving this refactor series so far! 💪
Part 4 of my refactoring series..
This PR pulls the code for updating the PendingInput to the OnitPanelState. We introduce a HighlightedTextDelegate that allows the HighlightedTextManager to send notifications when the highlighted text changes. The OnitPanelState can then subscribe to these notifications and own its logic for updating the PendingInput, disentangling the two.
One note: this might create some weird behavior in Tethered mode, since it will update ALL of the OnitPanelStates, instead of only the one in PanelStateCoordinator's state object. However, I think this is the better solution? If you highlight text, I would expect it to be available in all of the OnitPanels.
Next up: removing the Caret management code from the HighlightedTextManager.