search

atom / find-and-replace

Find and replace in a single buffer and in the project
MIT License
242 stars 219 forks source link

add find and replace previous buttons #1112

Closed UziTech closed 3 years ago

UziTech commented 4 years ago

Description of the Change

Add Find Previous and Replace Previous buttons to view

image

Alternate Designs

We could actually write words on the buttons but I feel like it is intuitive with the arrows. There is also tooltips on each button that says what it does so someone could hover over the button to see what it does.

image

Benefits

Easily find/replace previous from view. I know shift + Find finds the previous element but it is not documented anywhere and there is no equivilant shift + Replace

Possible Drawbacks

none

Applicable Issues

closes #897

fixes #269 fixes #453 fixes #467 fixes #800

UziTech commented 4 years ago

I could also add a setting to enable the buttons.

find-and-replace

UziTech commented 4 years ago

The buttons could be arrows or any other icon from octicons

image

diveshkr-code commented 4 years ago

Hey I feel 'replace next' and 'replace previous' seems not very useful and dangerous options, for example, suppose I mistakenly clicked 'replace previous' arrow and didn't realize that I have done so how will I get it back. Hence I suggest that only 'find next' and 'find previous' should be added.

UziTech commented 4 years ago

@diveshkr-code Ctrl + z will undo the replace

UziTech commented 4 years ago

@atom @darangi @nathansobo any feedback on this?

diveshkr-code commented 4 years ago

@diveshkr-code kr-code Ctrl + z will undo the replace

Surely @UziTech but what if the user clicks 'replace previous' without realizing he has done it and he doesn't see it change as the previous occurrence was not visible on the screen. Since the code is changed with replace option I think it's better if replace is done only on the current selection. Just my opinion. Cheers.

UziTech commented 4 years ago

@diveshkr-code There are many things the user can do to accidentally change something, but I don't think we should remove the functionality.

For example, my cat walked on my keyboard the other day and entered a bunch of random text as well as pushing the page down button so I couldn't see what was changed. That doesn't mean we should remove the ability to move down a page with the page down button.

The "Replace Previous" and "Replace Next" buttons are extremely useful when renaming a variables or replacing other text only in certain places. Some of the found text you might want to not replace so you can click "Find Next" and some you will want to replace so you can click "Replace Next".

UziTech commented 4 years ago

@diveshkr-code This PR isn't adding any new functionality. The current "Replace" button does a replace and finds the next occurrence (the same thing the replace next button will do in this PR), and the ability to replace previous is in the command palette. This PR just adds buttons for Find/Replace Previous

diveshkr-code commented 4 years ago

@diveshkr-code This PR isn't adding any new functionality. The current "Replace" button does a replace and finds the next occurrence (the same thing the replace next button will do in this PR), and the ability the replace previous is in the command palette. This PR just adds buttons for Find/Replace Previous

@UziTech I think you have me pretty convinced cheers.

UziTech commented 4 years ago

@atom @darangi @rafeca @nathansobo Who is reviewing this?

darangi commented 3 years ago

@atom @darangi @rafeca @nathansobo Who is reviewing this?

@UziTech, this has been added to our radar, the team will get back to you soonest. Thanks for the contribution 🙇‍♂️

sadick254 commented 3 years ago

Hey @UziTech, This PR adds a UI to a functionality that is already present within atom. When user opens find-and-replace settings, the user can see the shortcuts to find/ replace next and previous matches. The benefits we get from having a UI behind that functionality is very minimal. Thank you for the effort to improve find and replace, unfortunately we will have to close this one out.

UziTech commented 3 years ago

The benefits we get from having a UI behind that functionality is very minimal.

That is very subjective which is why we could place this behind a setting checkbox.

If you don't think Atom will allow this type of UI you should also close the issues this is PR would have fixed so people don't go there thinking this is something that will be allowed.

897, #269, #453, #467, #800

Blosberg commented 3 years ago

I'm struggling to understand your rationale here @sadick254.

The fact that there is, by default, no "Find previous" button is just baffling, and @UziTech has done the work to implement it, so there's no workload issue here. I can't understand the conscious decision to omit such an absolutely fundamental piece of functionality when it costs nothing.

The tragedy here is that I love everything else about Atom, but this is a deal-breaker. It's like you've built a Ferrari, and then removed the "reverse" gear, telling people they need to memorize some strange code just to back up. You say "the benefits are minimal" (to which I disagree, I think the benefits are minimal, absolutely critical, functionality), but what exactly is the cost? The work is done, there's plenty of space, there are clearly a lot of people who want this functionality. Is there any actual reason to not implement this?