Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

[REVIEW ONLY] Add Find whole word functionality#7604

Closed
marcelgerber wants to merge 1 commit into
adobe:masterfrom
marcelgerber:find-whole-word
Closed

[REVIEW ONLY] Add Find whole word functionality#7604
marcelgerber wants to merge 1 commit into
adobe:masterfrom
marcelgerber:find-whole-word

Conversation

@marcelgerber
Copy link
Copy Markdown
Contributor

This PR implements https://trello.com/c/xabJJVcu/1078-find-replace-whole-word-toggle
It has not yet an icon, but you can use it.

Tasks to be done:

The things I tested worked the same as in Sublime Text, but I found one scenario where it didn't work the way it works in Visual Studio:

  1. Open a file with the content if ("\\b")
  2. Find (whole word, of course) \\b

Nor Brackets neither Sublime find anything, but VS does. We'd probably need regexp lookbefores in order to fix this, but those are not supported in JavaScript.

Comment thread src/search/FindInFiles.js
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a fallback error here, but this shouldn't ever happen (at least if StringUtils.regexEscape is sane). Should I remove it, translate the error message (we could probably use it more often), or just leave it as is?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to do something -- we can't just not search. Can we just do showErrorMessage (e.message); like we do for the case insensitive search?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the code :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SAplayer I'd actually argue we shouldn't bother with a try/catch here. This is equivalent to the next case below, where we also construct a pre-escaped regexp.

Any unexpected internal errors like this will get logged to the console, break into the debugger if you have dev tools open, etc. That seems like the correct thing to do here and in the existing case below too. I don't see any particular reason why this case would need to be guarded more than the one below.

@JeffryBooher JeffryBooher self-assigned this Apr 28, 2014
@JeffryBooher
Copy link
Copy Markdown
Contributor

I'll take a look

@njx
Copy link
Copy Markdown

njx commented Apr 28, 2014

Hey - FYI, I'm planning to embark on a refactoring of the FindReplace/FindInFiles code to unify the Find bar UIs, along with possibly some other refactorings on the engine side. That should make it easier to implement this feature, but it will likely involve disruptive code changes that would make this PR hard to merge as-is.

I think it might be better to wait until that's done, because implementing this will be cleaner in the new version, rather than trying to merge this now and then clean it up during the refactoring.

@SAplayer is that ok with you? We can leave this PR open for now until that's done.

@njx
Copy link
Copy Markdown

njx commented Apr 28, 2014

(Looks like we can unify the code that creates the queries too. So pretty much everything in this PR will be easier to do after the code gets cleaned up.)

@marcelgerber
Copy link
Copy Markdown
Contributor Author

No problem to me.

Comment thread src/search/FindInFiles.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should return null; to keep it consistent

@JeffryBooher
Copy link
Copy Markdown
Contributor

@SAplayer This is a review only pr so I am going to close it. You should make the changes using the replace across files branch as a base plus add unit tests then open a new PR.

@marcelgerber marcelgerber deleted the find-whole-word branch June 17, 2014 10:50
@marcelgerber
Copy link
Copy Markdown
Contributor Author

@JeffryBooher I'll probably wait until the branch is merged into master.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants