Replace bug fix

[oBFusCATed]

Currently there is one very annoying bug in C::B.

Steps to reproduce:
1. Select something
2. Search replace
3. Type something for replace string
4. Click Replace
5. Click yes in the next dialog

The result is that the word after the selection is replace, even though the selection matches the search term.

This patch tries to fix this, any comments?

Code

diff --git a/src/sdk/editormanager.cpp b/src/sdk/editormanager.cpp
index 73e1956..28f6fec 100644
--- a/src/sdk/editormanager.cpp
+++ b/src/sdk/editormanager.cpp
@@ -1479,8 +1479,13 @@ void EditorManager::CalculateFindReplaceStartEnd(cbStyledTextCtrl* control, cbFi
         // there can be a selection, the last found match)
         if ((data->scope == 0) && data->NewSearch && (ssta != cpos || send != cpos))
         {
-            ssta = cpos;
-            send = cpos;
+            // Don't do this in replace mode, because we want to start the replacement
+            // with the current selection, not the first match after the selection.
+            if (!replace)
+            {
+                ssta = cpos;
+                send = cpos;
+            }
         }


@@ -1587,7 +1592,7 @@ int EditorManager::Replace(cbStyledTextCtrl* control, cbFindReplaceData* data)
     }
     control->BeginUndoAction(); // The undo is set at this point in case we need to convert the EOLs.

-    CalculateFindReplaceStartEnd(control, data);
+    CalculateFindReplaceStartEnd(control, data, true);

     if (data->matchWord)
         flags |= wxSCI_FIND_WHOLEWORD;

[ollydbg]

Go ahead, I think this bug is quite annoying for a long time.

[oBFusCATed]

There is another problem with this dialog.
Here it is:
1. Install MouseSap plugin
2. Open a source file
3. Select something
4. Press ctrl+f, ctrl+r
5. See that the string in the field "Text search for" is not selected
6. If you start to type you won't override it, but you start to append to it, which is very annoying.

This problem started some time ago and it was not present before.
It happens on both up-to-date gentoo and up-to-date centos5. I've not tried to reproduce it on windows.

I've tried to find why it happens. What I've found is that it happens because of this line:
MouseSap.cpp line 630

Code

            gtk_clipboard_set_text(
                gtk_clipboard_get(GDK_SELECTION_PRIMARY),
                selectedText.mb_str(wxConvUTF8),
                selectedText.Length() );

If I comment it or remove the call to focus the text field the bug disappears.

Any ideas and comments?

[thomas]

My 8541 build does not have this problem, or I'm too stupid to reproduce.

If I select something and then search/replace, it only ever replaces occurrences inside the selection, as I would expect.

Are you sure this is not some broken plugin that adds "extra smartness" and doesn't work?

[oBFusCATed]

If I select something and then search/replace, it only ever replaces occurrences inside the selection, as I would expect.

The idea is that you don't search inside the selection, but you select something (single word), fire up the replace dialog and don't change the search term.
It should match the selection and also the selection should be still active in the editor.

Are you sure this is not some broken plugin that adds "extra smartness" and doesn't work?

Reproduces with just the core built from inside C::B.

[thomas]

Ah, now I get it. The issue is that the search/replace starts at the first occurrence after the highlighted one. Yes, that's what it does here too.

[oBFusCATed]

The posted patch is committed. If you see any problems don't hesitate to report them.

I'm looking for answers about the second problem...

[oBFusCATed]

Can you tell me what variations of this patch work or doesn't on your machines (windows other linuxes)?

Code

diff --git a/src/sdk/findreplacedlg.cpp b/src/sdk/findreplacedlg.cpp
index 50da448..3dd4e38 100644
--- a/src/sdk/findreplacedlg.cpp
+++ b/src/sdk/findreplacedlg.cpp
@@ -229,6 +229,7 @@ FindReplaceDlg::FindReplaceDlg(wxWindow* parent, const wxString& initial, bool h
         XRCCTRL(*this, "chkDelOldSearchRes2",    wxCheckBox)->Show();
     }

+    wxComboBox *findCombo = nullptr;
     m_findPage = 0;
     if (findReplaceInFilesOnly)
     {
@@ -236,16 +237,22 @@ FindReplaceDlg::FindReplaceDlg(wxWindow* parent, const wxString& initial, bool h
         XRCCTRL(*this,  "nbReplace", wxNotebook)->SetSelection(1);
         m_findPage = (XRCCTRL(*this, "nbReplace", wxNotebook)->GetPage(0)); // no active editor, so only replace-in-files
         (XRCCTRL(*this, "nbReplace", wxNotebook)->RemovePage(0)); // no active editor, so only replace-in-files
-        XRCCTRL(*this,  "cmbFind2",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this,  "cmbFind2",  wxComboBox);
         m_findReplaceInFilesActive = true;
     }
     else if (m_findReplaceInFilesActive)
     {
         XRCCTRL(*this, "nbReplace", wxNotebook)->SetSelection(1); // Search->Replace in Files was selected
-        XRCCTRL(*this, "cmbFind2",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this, "cmbFind2",  wxComboBox);
     }
     else
-        XRCCTRL(*this, "cmbFind1",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this, "cmbFind1",  wxComboBox);
+
+    if (findCombo)
+    {
+        findCombo->SetFocus();
+//        findCombo->SetSelection(0, findCombo->GetValue().length());
+    }

     GetSizer()->SetSizeHints(this);

You have to play with commenting the two lines in the "if (findCombo) " section.
For me the posted version doesn't work.
Commenting both lines works, uncommenting both lines also works.

[oBFusCATed]

Have anyone tried the patch?

[MortenMacFly]

Have anyone tried the patch?

I'm tired of trying GIT patches you know..

[oBFusCATed]

There you go...

Code

Index: src/sdk/findreplacedlg.cpp
===================================================================
--- src/sdk/findreplacedlg.cpp
+++ src/sdk/findreplacedlg.cpp
@@ -229,6 +229,7 @@ FindReplaceDlg::FindReplaceDlg(wxWindow* parent, const wxString& initial, bool h
         XRCCTRL(*this, "chkDelOldSearchRes2",    wxCheckBox)->Show();
     }

+    wxComboBox *findCombo = nullptr;
     m_findPage = 0;
     if (findReplaceInFilesOnly)
     {
@@ -236,16 +237,22 @@ FindReplaceDlg::FindReplaceDlg(wxWindow* parent, const wxString& initial, bool h
         XRCCTRL(*this,  "nbReplace", wxNotebook)->SetSelection(1);
         m_findPage = (XRCCTRL(*this, "nbReplace", wxNotebook)->GetPage(0)); // no active editor, so only replace-in-files
         (XRCCTRL(*this, "nbReplace", wxNotebook)->RemovePage(0)); // no active editor, so only replace-in-files
-        XRCCTRL(*this,  "cmbFind2",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this,  "cmbFind2",  wxComboBox);
         m_findReplaceInFilesActive = true;
     }
     else if (m_findReplaceInFilesActive)
     {
         XRCCTRL(*this, "nbReplace", wxNotebook)->SetSelection(1); // Search->Replace in Files was selected
-        XRCCTRL(*this, "cmbFind2",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this, "cmbFind2",  wxComboBox);
     }
     else
-        XRCCTRL(*this, "cmbFind1",  wxComboBox)->SetFocus();
+        findCombo = XRCCTRL(*this, "cmbFind1",  wxComboBox);
+
+    if (findCombo)
+    {
+        findCombo->SetFocus();
+//        findCombo->SetSelection(0, findCombo->GetValue().length());
+    }

     GetSizer()->SetSizeHints(this);

p.s. now you know how I feel, when I see commits full of style-changes and you start to advertise your tool and workflow

[MortenMacFly]

OK - this one applied.

p.s. now you know how I feel, when I see commits full of style-changes and you start to advertise your tool and workflow

I don't know exactly what you mean, but I still believe you'll definitely reach more devs and potentially interested people with a SVN patch. So its up to you in the end what you want to achieve... I don't mind if you post GIT patches at all, I just won't invest much time to hack a patch so it works for me (and only me). Would you hack SVN patches so they work on GIT? I doubt.

[oBFusCATed]

OK - this one applied.

The idea is not to apply it, but to play with the options and then tell me which are working and which doesn't.

I don't know exactly what you mean ...

I mean that every time I mention that it is a bad practice to combine real changes with white-space/style changes you mention that I have to use your super-dupper-paid SmartXXX software. So you apply double standard here  
Git has no problems with svn patches, because I don't use git to apply them, but the patch utility. The workflow is the same for both git and svn here...

btw: I though you've reported the issue to the SmartXXX guys and they've fixed it?