Skip to content

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

austincondiff
Copy link
Collaborator

@austincondiff austincondiff commented Jun 2, 2025

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

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Screen.Recording.2025-06-02.at.1.30.31.PM.mov

@thecoolwinter
Copy link
Collaborator

thecoolwinter commented Jun 3, 2025

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 a find.
  • 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 the String 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)

@thecoolwinter
Copy link
Collaborator

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

@austincondiff
Copy link
Collaborator Author

austincondiff commented Jun 3, 2025

@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.

@austincondiff austincondiff marked this pull request as draft June 3, 2025 22:22
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.

✨ Select Next/Previous Occurrence
2 participants