Skip to content
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

#42382 Preserve case while search and replace #60311

Merged
merged 6 commits into from Jul 25, 2019

Conversation

@JulienMalige
Copy link
Contributor

commented Oct 9, 2018

Ex JetBrains IntelliJ user, I was looking for a "preserve case" option in the "search and replace" box.

capture d ecran 2018-10-09 a 14 20 04

#42382, #9798 are linked to my use case.

Replacing with case preservation can uses different strategies. I chose the Emacs one :

replaces a lower case ‘foo’ with a lower case ‘bar’, an all-caps ‘FOO’ with ‘BAR’, and a capitalized ‘Foo’ with ‘Bar’.

https://www.gnu.org/software/emacs/manual/html_node/emacs/Replacement-and-Lax-Matches.html#Replacement-and-Lax-Matches


There is no test regression but it's still a WIP :

  • add a new test suite
  • add a keyboard shortcut (can be a next PR)
  • add to global search (can be a next PR)
@msftclas

This comment has been minimized.

Copy link

commented Oct 9, 2018

CLA assistant check
All CLA requirements met.

@JulienMalige JulienMalige force-pushed the JulienMalige:preserve-case branch 3 times, most recently from 86f6fe1 to 4362e07 Oct 18, 2018

@JulienMalige JulienMalige force-pushed the JulienMalige:preserve-case branch from 4362e07 to ea97d58 Oct 19, 2018

@JulienMalige JulienMalige force-pushed the JulienMalige:preserve-case branch from ea97d58 to 73af37c Oct 19, 2018

@rebornix

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

@JulienMalige thanks for taking this, I like the idea of case preserving and glad that you can help with this feature.

I don't worry too much about how this feature is implemented, either IntelliJ way or Emacs way. But I'm not sure where the option/flag should be placed, the find input box is already crowded. cc @misolori and @roblourens , this one looks like a generic feature for both Find Widget and Search Viewlet and we share the same UI component.

@misolori

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I think this feature is quite useful, but I also agree that I'm not sure where this toggle should be placed. It seems like this would go well with the rest of the replace/all, which could go into a context dropdown:

image

@rebornix thoughts? we'd need to revisit this for the sidebar since we already use the context icon to expand the search details, but that's a poor choice of an icon to begin with since we use the chevron icon to expand content.

@rebornix

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@misolori the dropdown in your mockup looks neat, I like it. The only concern here is we don't want to make things hard for users who only do replace and replace all, introducing a dropdown always will add one more step.

One solution is we can probably keep the existing UI and hide advanced features like Preserve Case or others behind flags, when they are enabled, we start to show the dropdown icon.

Another way is trying to show all icons when possible but display a dropdown when the find widget is not wide enough for all of them.

Right now there are a few feature requests that we can't easily implement as we lack of space in find widget, but once we have a good UX for it, they can be unblocked.

@misolori

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I do like the idea of showing certain toggles contextually, however I agree that we don't have scalable solution for adding additional toggles. I'd vote for this option.

While we're talking other ideas, we could also explore moving the replace actions into the input field like the find input:
image

@roblourens

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

What's wrong with the original proposal? I think it makes sense and doesn't (yet) feel crowded if it's by itself in the replace box. It's consistent in that the other toggle buttons in the search input also control "modes" for your find/replace. I think the replace buttons would be out of place in the replace input because they are actions, not toggle buttons.

@roblourens

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

And we do also need to do this for search, I think it's ok to be a separate PR but should go out in the same month as in the find widget.

@misolori

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I think the issue with the original proposal is that it's adding to an already cluttered UI, look at the positioning of the toggles (red) and actions (blue):

image

And this brings up an interesting scenario for the find widget where we've maximized the real estate for it. I also think this is a good candidate to bring up in our UX Sync this week.

@JulienMalige

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

My initial idea was actually to preserve the differentiation between toggles and actions.

For me, the toggles must be close to the actions they affect. My toggle is directly related to the content of the input, so I think it has to be inside of it (like the toggles of the search). The "isolated" toggle on the right is not disturbing me, because it affects the buttons close to him (previous/next).

@roblourens, are you speaking about the global search ?
if so, I could and would like to develop this feature (can be done in November).

@rebornix rebornix added this to the December 2018 milestone Dec 4, 2018

@rebornix

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Let's visit this in December and have it merged, having the toggle button inside the replace input box is good enough.

@rdhelms

This comment has been minimized.

Copy link

commented Apr 24, 2019

This looks like a great feature to have! Looking forward to it someday!

@JulienMalige

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

I will try to finish it on May 🚀

@johnculviner

This comment has been minimized.

Copy link

commented Apr 25, 2019

Please merge soon. This would have saved me lots of time today already!!

@rebornix rebornix added this to the On Deck milestone Jun 13, 2019

@rebornix rebornix self-requested a review Jul 24, 2019

@rebornix rebornix changed the title [WIP] #42382 Preserve case while search and replace #42382 Preserve case while search and replace Jul 24, 2019

@rebornix rebornix merged commit 5f8d2ef into microsoft:master Jul 25, 2019

5 checks passed

VS Code Build #20190725.3 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details
@rebornix

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Finally got this in a good shape and merged. Thanks for your contribution and everyone's patience ;)

With this change and in tomorrow's Insiders, you should be able to preserve case when doing a single replacement. Currently we support

  • all lower case
  • ALL UPPER CASE
  • Title Case

Feel free to contribute more patterns for this feature.

@gmosx

This comment has been minimized.

Copy link

commented Jul 27, 2019

Btw, this search-replace 'popup' is too small and crowded. Any thoughts about making this bigger and maybe horizontally-centered within the editor window?

@rebornix

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@gmosx you can drag the left border of the find widget to resize it.

@rebornix rebornix referenced this pull request Jul 29, 2019
2 of 2 tasks complete

@roblourens roblourens modified the milestones: On Deck, July 2019 Aug 2, 2019

@gmosx

This comment has been minimized.

Copy link

commented Aug 2, 2019

@gmosx you can drag the left border of the find widget to resize it.

Hm, didn't notice that. Still the popup is very small. If it was centered, I believe it would 'feel' better.
Anw, thanks for the tip :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.