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#8310

Closed
marcelgerber wants to merge 2 commits into
adobe:masterfrom
marcelgerber:find-whole-word-2
Closed

[REVIEW ONLY] Add Find Whole Word functionality#8310
marcelgerber wants to merge 2 commits into
adobe:masterfrom
marcelgerber:find-whole-word-2

Conversation

@marcelgerber
Copy link
Copy Markdown
Contributor

New version of #7604.

This PR implements https://trello.com/c/xabJJVcu/1078-find-replace-whole-word-toggle.
It doesn't have an icon yet, but you can use it without restrictions.

To be done:

  • Unit tests
  • Only allow either Whole word or Regular expression toggle to be active at once
  • Icon (@larz0?)

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 ("abc")
  2. Find Whole Word: abc

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

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

Thanks @SAplayer, working on the icon now.

@TomMalbran
Copy link
Copy Markdown
Contributor

@larz0 @SAplayer Should we have another icon, or should the current Regular expression toggle button become a dropdown button so that we don't use more space with new buttons? (we should get more in the future)

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

@TomMalbran @SAplayer that got me thinking. When "Match Case" and "Regex" are both on would "Match Case" affect the regex results?

@TomMalbran
Copy link
Copy Markdown
Contributor

@larz0 You can't have both on. That is why a dropdown will work well here.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

@TomMalbran I'll need to think about it. If we want to use a drop down then we'll need to figure out a way to show the options that are being turned on.

@TomMalbran
Copy link
Copy Markdown
Contributor

@larz0 Sure. Maybe it can be a dropdown with icons. The currently selected one is both the displayed one and with the lightblue background and the one with a checkmark in the list. The card linked here also includes toggles for prefix of word and suffix of word, no implemented here but that could be eventually added to the same dropdown.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

@TomMalbran sounds like we can definitely group "Whole word", "Prefix" and "Suffix" into a drop down :)

@TomMalbran
Copy link
Copy Markdown
Contributor

@larz0 That can work too. So for now we can use an icon and we can add the dropdown once the others are implemented.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

Here you go @SAplayer http://cl.ly/0T3C1d0H3M1Y

Should just work once you replace the old sprite with this one.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 7, 2014

@TomMalbran cool sounds good.

@redmunds
Copy link
Copy Markdown
Contributor

redmunds commented Jul 8, 2014

@TomMalbran

You can't have both ("Match Case" and "Regex") on.

Yes, you can. The "Match Case" setting remove the i flag from the "Regex".

But, "Find Whole Word" and "Regex" are mutually exclusive. Maybe that's what you meant.

@TomMalbran
Copy link
Copy Markdown
Contributor

@redmunds Yes, that is what I mean. I just read it as "Whole Word" and "Regex" since we are in this PR

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 8, 2014

@redmunds @TomMalbran In that case then it makes sense to make the "Regex" button a drop down button with "whole word", "prefix" and "suffix" in the menu.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

@larz0 So should I wait until we've got these extra two buttons (prefix/suffix) or implement the dropdown right now?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 8, 2014

@SAplayer we could implement the drop-down now. I thought about it last night and realized that we need a third option that's the default option. We could call it "Any" for now but let me know if you can think of a better label. What do you think @TomMalbran?

@marcelgerber
Copy link
Copy Markdown
Contributor Author

I'd imagine it like this:

// dropdown button
Further Options
None
.* Regular Expression
“” Whole Word
“… Prefix
…” Suffix

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 8, 2014

@SAplayer what's "Further Options"? Maybe we can use a Filter icon for "None". Looks good!

@marcelgerber
Copy link
Copy Markdown
Contributor Author

I thought of "Further Options" as a header for the menu (bold and delimited).

@larz0
Copy link
Copy Markdown
Member

larz0 commented Jul 8, 2014

I see. Maybe it's ok to leave that out if the drop-down's default icon is obvious. I'll work on the additional icons that we'll need.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

Note to self: The "Whole Word" toggle should influence the "Find All and select" (Alt+F3 currently) behaviour, too. See #8473 (comment)

@JeffryBooher
Copy link
Copy Markdown
Contributor

Seems to work. Can you merge master into this branch. Let us know when there are unit tests for this.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

@JeffryBooher This PR is not yet ready. We need a dropdown to contain all those new flags.
Sorry for not working on this, but I had other stuff to do.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

Closing in favor of #9526.

@marcelgerber marcelgerber deleted the find-whole-word-2 branch October 11, 2014 08:53
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.

5 participants