-
Notifications
You must be signed in to change notification settings - Fork 105
Added Select Next/Previous Occurrence commands #330
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
Conversation
...ect previous occurrence.
Okay a few things here after some review:
- Those huge new methods on the find model are not great, they try and misuse the model for something the rest of the model does not expect. On top of that they're not doing much more than what
moveMatch
is doing after performing afind
. - The
moveMatch
changes are confusing imo. They have cyclomatic complexity errors and don't need the extra paths you've added. You're also setting the selection ranges twice, once with the textView and once with the view controller. - We don't do any menu validation, and don't provide any custom menus so I'm not sure what the
NSMenuItemValidation
addition is doing here. It will be required eventually when we do commands, but right now it's just causing a lint error because the file is too long. Without that, the command handlers can be non-objc and moved to loadView with the other command handler methods.
In the end, I got this mostly working after spending a while on it but I don't think this is the path we should go. This is mixing concerns with the find panel and this feature and just causing a headache to try and sift through the extra logic this introduces to the find model.
It also introduces some odd behaviors like:
- What if the user has
beginsWith
selected, should that be cleared for finding the next occurrence? - If the user has the find panel open, should it really clear the find text for finding the next occurrence?
I feel like this could be a smaller change, I think it should(?) require:
- Creating a
find
method on theString
type that takes a regular expression and a range to search.See the existing find method, where we can specify the range in which to query:
// FindPanelViewModel+Find.swift let text = target.textView.string let range = target.textView.documentRange let matches = regex.matches(in: text, range: range).filter { !0ドル.range.isEmpty } self.findMatches = matches.map(\.range)
- This could be updated to only find the next match, or to collect all the matches (re: find next occurrence).
- A separate algorithm for finding the next/previous occurrence of the selected text:
- When selecting the next occurrence, find the next match of the current selection in the document range after the current selection.
- Reverse in the opposite direction.
- Emphasize the new match if it's found.
- Add the new match to the text selection (
textView.textSelectionManager.addSelection
)
If we do need to keep around some tracked state to make this work, I think it should be in a new class. I did end up getting it to work but it was hacky and made the find model unnecessarily complex
@thecoolwinter I see your points and they are all valid. I first wanted to see if I could get it to work so I rushed through a lot of this and didn't take the time to clean it up, so thats on me.
Let me clean it up and I'll let you know where I end up with this. I'll put this PR in draft status in the mean time.
Uh oh!
There was an error while loading. Please reload this page.
Description
Adds Select Next Occurrence (⇧⌥⌘E) and Select Previous Occurrence (⇧⌥⌘E) commands.
Respects Wrap Around and Match Case settings in the Find panel.
Related Issues
Checklist
Screenshots
Screen.Recording.2025年06月02日.at.1.30.31.PM.mov