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

Prepopulate replace input with current editor selection #1964

Merged
merged 3 commits into from
Oct 29, 2012
Merged

Prepopulate replace input with current editor selection #1964

merged 3 commits into from
Oct 29, 2012

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Oct 26, 2012

Prepopulates the replace input with the current editor selection.

This pull request replaces #1962 following @peterflynn suggestions.

@ghost ghost assigned peterflynn Oct 26, 2012
@peterflynn
Copy link
Member

This looks good, but one thing I noticed is that the replace begins on the next occurrence after the text you initially selected. It loops back around eventually, but the text you had selected will always be last to get replaced. This bug already existed, of course, but your change makes it easier to hit since there's now an incentive to have some text selected.

I think you can fix this by changing line 225 from cm.getCursor() to cm.getCursor(true) (i.e., always begin at the start of the selection, instead of whichever end the cursor was left at). Can you test this out and add it to your pull request if it works?

@jbalsas
Copy link
Contributor Author

jbalsas commented Oct 26, 2012

Yes, I noticed that as well, but as it was already happening in master and is stated in the comments at the top I thought about trying to solve it separately...

Something similar happens if you have the cursor in the middle of a word (no selection) which looks strange to me. However, I've been testing it and Sublime2 does exactly the same.

@peterflynn I've tested your solution and setting cm.getCursor(true) does fix this indeed. Do you also want me to take out or update the Replace works a little oddly [...] comment at the top?

@jbalsas
Copy link
Contributor Author

jbalsas commented Oct 28, 2012

@peterflynn I've already pushed the changes.

After reading it again, I think the comment I was quoting refers to the find-next in general, which still works like that. This could also be fixed by setting cm.getCursor(true) in line 151 inside doSearch. I could push it here as well, but maybe it makes more sense to create a differente pull request. What do you think?

@peterflynn
Copy link
Member

@jbalsas: I actually think that comment is no longer correct even before your change, so we should probably just remove it. It originates from a very old version of the file, where it looks like Find and Replace were much more entangled. In more recent versions of the code, findNext() never does any replacement.

But we can do that cleanup later -- I'm happy to merge this pull request now.

peterflynn added a commit that referenced this pull request Oct 29, 2012
Prepopulate Replace input with current editor selection (similar to Find)
@peterflynn peterflynn merged commit 80d4ac1 into adobe:master Oct 29, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants