PR: Implement "Replace in selection" in the Editor #4638

spillz · 2017-06-22T13:44:02Z

Fixes #358

Supports replacing all instances of the search text with the replace text in a selection of text. Changes find/replace dialog to add buttons for "replace in selection" and "replace all" and removes the "replace all" checkbox. Tweaks behavior of automatically filling the searchbox depending on whether multiple lines were selected.

A refinement would be to either add shortcuts for the three buttons or combine the three buttons into one and use radio/combo to control scope.

Supports replacing all instances of the search text with the replace text in a selection of text. Changes find/replace dialog to add buttons for "replace in selection" and "replace all" and removes the "replace all" checkbox. Tweaks behavior of automatically filling the searchbox depending on whether multiple lines were selected.

ccordoba12 · 2017-06-22T13:49:24Z

@spillz, thanks for the extra effort. I'll let @rlaverde do the review.

rlaverde

I test it and works as expected, and I think the workflow is intuitive.

rlaverde · 2017-06-22T16:37:45Z

spyder/widgets/findreplace.py

@@ -226,32 +238,36 @@ def toggle_highlighting(self, state):
            else:
                self.clear_matches()

-    def show(self):
+    def show(self, hide_replace = True):


Remove spaces around keyword argument:
def show(self, hide_replace=True)

rlaverde · 2017-06-22T16:41:48Z

spyder/widgets/findreplace.py

@@ -267,7 +283,7 @@ def hide(self):

    def show_replace(self):
        """Show replace widgets"""
-        self.show()
+        self.show(hide_replace = False)


Remove spaces around keyword argument:
self.show(self, hide_replace=True)

rlaverde · 2017-06-22T16:44:39Z

spyder/widgets/findreplace.py

-                self.refresh()
-            else:
-                self.search_text.lineEdit().selectAll()
+            if hide_replace or len(text.splitlines())<=1:


Maybe a comment for explaining this behavior is missing: "When selecting several lines, and replace box is activated the text won't be replaced for the selection"

rlaverde · 2017-06-22T17:01:32Z

spyder/widgets/findreplace.py

+
+
+    @Slot()
+    def replace_find_selection(self, focus_replace_text=False):


I'm not sure if using a custom search is the best way, because the other searching methods use the findText function of QWebEngineView (they also use python regex when regex checkbox is activated).

Although I'm not sure if this could be implemented using QWebEngineView.findText because it doesn't support searching in a selection, also multiple selection should be managed adding more complexity.

True that there is a small risk that there are different results for the two regex implementations. I think the risk is relatively small because we don't highlight find results when the user chooses a selection. And there's always UNDO...

Actually a bigger inconsistency is that I didn't implement the whole word restriction. DOH! And it might ultimately be simpler to use the QT implementation because python's re doesn't handle whole words. Let me give this some thought over the weekend.

rlaverde · 2017-06-22T17:01:55Z

spyder/widgets/findreplace.py

@@ -329,7 +345,8 @@ def find_previous(self):
    def text_has_been_edited(self, text):
        """Find text has been edited (this slot won't be triggered when 
        setting the search pattern combo box text programmatically"""
-        self.find(changed=True, forward=True, start_highlight_timer=True)
+        if not self.replace_widgets[0].isVisible() or len(to_text_string(self.editor.get_selected_text()).splitlines())<=1:


Add a comment for explaining this behavior too: "When selecting several lines, and replace box is activated, dynamically search is deactivated, for avoid changing the selection."

Also, this line is way to long. Please break it using a \

rlaverde · 2017-06-22T17:22:14Z

@ccordoba12

I think this plugin need a refactoring (not as part of this PR) the different searching functions find replace_find replace_find_selection should use the same "backend", there is duplicated logic and functions use different ways to do something very similar.

ccordoba12 · 2017-06-22T22:37:21Z

Ok, but that's for Spyder 4.

ccordoba12 · 2017-06-22T22:39:49Z

spyder/widgets/findreplace.py

+            cursor.beginEditBlock()
+            seltxt = to_text_string(self.editor.get_selected_text())
+            if not pattern:
+                replacement = re.sub(re.escape(search_text), re.escape(replace_text), seltxt, flags=re_flags)


This line is also too long. Please make it in 79 columns

ccordoba12 · 2017-06-22T22:40:17Z

@rlaverde, did you test this locally to see if it's working as expected?

rlaverde · 2017-06-22T22:49:24Z

did you test this locally to see if it's working as expected?

yes

…ents)

Handles whole word restriction on matches and more fully deactivates dynamic search when user changes the find/replace options when the replace box is activated and a multiple lines in the editor are selected.

spillz · 2017-06-25T17:11:56Z

The latest changes handle the whole word option and take care of deactivating the dynamic search multiple line selections. In my testing I noticed a few little bugs in the way that the original implementation of "replace all" works. I haven't fixed that here, but that could be the subject of a different PR.

ccordoba12 · 2017-06-25T17:29:54Z

Our tests are failing because you left a print statement behind.

In my testing I noticed a few little bugs in the way that the original implementation of "replace all" works

Could you fix them here too? I mean, if they are simple enough, we could use the opportunity to fix them.

spillz · 2017-06-25T17:35:06Z

Could you fix them here too? I mean, if they are simple enough, we could use the opportunity to fix them.

They aren't trivial to fix. They stem from the mixed use of QT and python re in the original implementation.

Sorry about the print statement. Removed it.

ccordoba12 · 2017-06-25T17:47:42Z

They aren't trivial to fix. They stem from the mixed use of QT and python re in the original implementation.

Ok, no problem. Please open an issue about them so we don't forget to fix them.

Sorry about the print statement. Removed it.

No problem, that's why we have a test to check for no print's.

ccordoba12 · 2017-06-27T16:27:25Z

@rlaverde, please give a last review to @spillz changes to see if we can merge them after his last changes.

rlaverde · 2017-06-27T21:47:28Z

spyder/widgets/findreplace.py

+                #If whole words is checked we need to check that each match
+                #is actually a whole word before replacing
+                word_flags = 0 if case else re.IGNORECASE
+                replacement = re.sub(pattern, _word_replacer, seltxt, flags=re_flags)


Why _word_replacer is needed? I think this could be achieved changing the pattern, like other functions do:

if words: pattern = r"\b{}\b".format(pattern)

spillz · 2017-06-29T16:03:00Z

Good point. I think I mistakenly thought the original find implementation was rejecting any matches if there were spaces in the search term but it doesn't look like that's the case. It does seem to match the "\b...\b" But if someone puts in "\" as their regex then they will be replacing instances of "\b" that start with a whole word. But maybe it is enough to make sure the pattern compiles before enclosing it in \b? On Jun 27, 2017 5:47 PM, "Rafael Laverde" <[email protected]> wrote: *@rlaverde* commented on this pull request. ------------------------------ In spyder/widgets/findreplace.py <#4638 (comment)>:

+ pattern = search_text if self.re_button.isChecked() else None

+ case = self.case_button.isChecked() + words = self.words_button.isChecked() + re_flags = re.MULTILINE if case else re.IGNORECASE|re.MULTILINE + + cursor = self.editor.textCursor() + cursor.beginEditBlock() + seltxt = to_text_string(self.editor.get_selected_text()) + if not pattern: + pattern = re.escape(search_text) + replace_text = re.escape(replace_text) + if words: + #If whole words is checked we need to check that each match + #is actually a whole word before replacing + word_flags = 0 if case else re.IGNORECASE + replacement = re.sub(pattern, _word_replacer, seltxt, flags=re_flags) Why _word_replacer is needed? I think that this could be achieved changing the pattern, like other functions do: if words: pattern = r"\b{}\b".format(patern) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4638 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFd5YJdXoJasXlo9veB_W3s_ZhhS8l0ks5sIXhzgaJpZM4OCUNE> .

ccordoba12 · 2017-06-30T04:44:05Z

@rlaverde, please take a decision about this one. We could leave it for 3.2.1 if you prefer.

rlaverde · 2017-06-30T16:30:02Z

But if someone puts in "" as their regex then they will be replacing instances of "\b" that start with a whole word. But maybe it is enough to make sure the pattern compiles before enclosing it in \b?

Yes, I also think that will be enough, please implement it using "\b...\b"

@rlaverde, please take a decision about this one. We could leave it for 3.2.1 if you prefer.

I think this could be in 3.2, Only the change for whole words is missing.

ccordoba12 · 2017-07-04T21:01:53Z

@spillz, please address @rlaverde comment ASAP if you want to see this in 3.2. You have until tomorrow (Wednesday) to do that.

Else I'll move this for 3.2.1.

spillz · 2017-07-05T12:40:58Z

I should be able to do it this evening (US Eastern time).

…

On Jul 4, 2017 5:01 PM, "Carlos Cordoba" ***@***.***> wrote: @spillz <https://github.com/spillz>, please address @rlaverde <https://github.com/rlaverde> comment ASAP if you want to see this in 3.2. You have until tomorrow (Wednesday) to do that. Else I'll move this for 3.2.1. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4638 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFd5VKBp3b6NJeOA2by4nOanA2GBfg4ks5sKqhFgaJpZM4OCUNE> .

ccordoba12 · 2017-07-05T14:27:07Z

That's fine. Thanks a lot for taking the time to do it.

…lways show and focus replace box after pressing Ctrl+R

ccordoba12 · 2017-07-06T23:20:24Z

@rlaverde, please take a look at this one and approve it, if it's ready.

ccordoba12 · 2017-07-06T23:50:44Z

Thanks for your work @spillz!

Fixes #358

ccordoba12 mentioned this pull request Jun 22, 2017

PR: Implement replace in selection in the Editor #4635

Closed

ccordoba12 added this to the v3.2 milestone Jun 22, 2017

rlaverde reviewed Jun 22, 2017

View reviewed changes

ccordoba12 reviewed Jun 22, 2017

View reviewed changes

Damien Moore and others added 2 commits June 22, 2017 22:21

updated find replace in selection for review comments (style and comm…

2e904b3

…ents)

Additional find replace in selection changes

2ad8ed6

Handles whole word restriction on matches and more fully deactivates dynamic search when user changes the find/replace options when the replace box is activated and a multiple lines in the editor are selected.

Find replace in selection patch -- removed extraneous print statement

ef6698d

rlaverde reviewed Jun 27, 2017

View reviewed changes

Find/replace in selection: Simplify whole word replacement code and a…

9ee947f

…lways show and focus replace box after pressing Ctrl+R

ccordoba12 changed the title ~~Implement replace in selection in the Editor~~ PR: Implement replace in selection in the Editor Jul 6, 2017

rlaverde approved these changes Jul 6, 2017

View reviewed changes

ccordoba12 changed the title ~~PR: Implement replace in selection in the Editor~~ PR: Implement "Replace in selection" in the Editor Jul 6, 2017

ccordoba12 merged commit c109aff into spyder-ide:3.x Jul 6, 2017

ccordoba12 added a commit that referenced this pull request Jul 6, 2017

Merge from 3.x: PR #4638

8f72201

Fixes #358

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Implement "Replace in selection" in the Editor #4638

PR: Implement "Replace in selection" in the Editor #4638

spillz commented Jun 22, 2017 •

edited by ccordoba12

Loading

ccordoba12 commented Jun 22, 2017

rlaverde left a comment •

edited

Loading

rlaverde Jun 22, 2017

rlaverde Jun 22, 2017

rlaverde Jun 22, 2017

rlaverde Jun 22, 2017 •

edited

Loading

spillz Jun 23, 2017

spillz Jun 23, 2017

rlaverde Jun 22, 2017

ccordoba12 Jun 22, 2017

rlaverde commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

ccordoba12 Jun 22, 2017

ccordoba12 commented Jun 22, 2017

rlaverde commented Jun 22, 2017

spillz commented Jun 25, 2017

ccordoba12 commented Jun 25, 2017

spillz commented Jun 25, 2017

ccordoba12 commented Jun 25, 2017

ccordoba12 commented Jun 27, 2017

rlaverde Jun 27, 2017 •

edited

Loading

spillz commented Jun 29, 2017 via email

ccordoba12 commented Jun 30, 2017

rlaverde commented Jun 30, 2017

ccordoba12 commented Jul 4, 2017

spillz commented Jul 5, 2017 via email

ccordoba12 commented Jul 5, 2017

ccordoba12 commented Jul 6, 2017

ccordoba12 commented Jul 6, 2017



		@Slot()
		def replace_find_selection(self, focus_replace_text=False):

PR: Implement "Replace in selection" in the Editor #4638

PR: Implement "Replace in selection" in the Editor #4638

Conversation

spillz commented Jun 22, 2017 • edited by ccordoba12 Loading

ccordoba12 commented Jun 22, 2017

rlaverde left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlaverde Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlaverde commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

Choose a reason for hiding this comment

ccordoba12 commented Jun 22, 2017

rlaverde commented Jun 22, 2017

spillz commented Jun 25, 2017

ccordoba12 commented Jun 25, 2017

spillz commented Jun 25, 2017

ccordoba12 commented Jun 25, 2017

ccordoba12 commented Jun 27, 2017

rlaverde Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

spillz commented Jun 29, 2017 via email

ccordoba12 commented Jun 30, 2017

rlaverde commented Jun 30, 2017

ccordoba12 commented Jul 4, 2017

spillz commented Jul 5, 2017 via email

ccordoba12 commented Jul 5, 2017

ccordoba12 commented Jul 6, 2017

ccordoba12 commented Jul 6, 2017

spillz commented Jun 22, 2017 •

edited by ccordoba12

Loading

rlaverde left a comment •

edited

Loading

rlaverde Jun 22, 2017 •

edited

Loading

rlaverde Jun 27, 2017 •

edited

Loading