#1436 Patch ThreadSearch "MatchInComments"

[Tiger Beard]

Patch ThreadSearch "MatchInComments"
The patch adds a filter option to the thread search Quick options and normal options dialogs. The option is SELECTED by default in order to keep the current behaviour unchanges. When the option is UNSELECTED the each line is scanned for C++ Style comments "//". The patch can recognize full line comments with leading whitespace as well as end-of-line comments. Strings containing "//" sequences before an end-of-line comment are handled correctly.
Only files from the plugin/contrib/threadsearch are affected.
I tested the patch locally and it worked as expected. Tested: in Linux 22.04. Not tested in Windows or MacOS

Attached patch was dones with SVN against current trunk in the root directory as per wiki article "Creating a patch to submit (Patch Tracker)"

[Tiger Beard]

This was my first patch, so please bear with me if I got not everything right.

[ollydbg]

Hi, thanks for your contribution.

I just download the patch file, and open in an editor, I see that in the source code changes, there are some "TAB" chars, it should be all spaces in my mind.

I haven't applied the patch and test yet.

[Tiger Beard]

Thanks for checking out the patch. Attached a version w/o tabs.

Am I correct that the patch format needs the tabs between the filename and the revision number. It would be easier if that could be spaces as well...

[ollydbg]

This is my modified patch, I only fix the format.

[Tiger Beard]

Just noted that I missed one case. Context menu /"Find occurences of '..' "in the >standard editor seems to call another search function than the SeachButton in the >thead view window. The difference is that the SearchInComments setting is ignored.

this is actually a feature I was not aware of: Thread Search OptionsDialog / "Use default options when 'find occurrences".

Only thing that might be done is to add the 'Match Comments' = false to line
405 in /src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp
which is a static control text only.

[Tiger Beard]

As I am not sure how to modify OllyDbgs patch I would skip the static text change above and have the patch committed
I am working since one month with it and its working fine, no side effects found.

What do I have to do?

[ollydbg]

Hi, @Tiger Beard. Sorry, I'm a bit busy those days, so I haven't got time to review the patch files. I use this patch for a long time(about a month) in my local built C::B. If other devs can review it, go ahead. If other devs don't have time, I think I will review it in a few days.

Thanks.

[ollydbg]

Hi, about this code line changes:

Only thing that might be done is to add the 'Match Comments' = false to line
405 in /src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp
which is a static control text only.

Do you mean the

'Match Comments' = true

?

I think by default, the C++ comment text is also searched.

Also, I reviewed the whole patch today, and it is OK, I will commit the patch as soon as I can.

Sorry for the delay.

Happy Chinese New Year!

[Tiger Beard]

Thanks for reviewing my patch.

no I did not mean "'Match Comments' = true". The line I meant is a static text line in the configuration panel telling the user what options are set when the context switch version of the command is used.
Myself I was not aware up to making the patch that the context menu search is different from the current parameters set in the options setting, but I can see that in some situations it might make sense.

So in order not not change the current behaviour any new option in the search should be disabled by default. And thats what is currently happening, so there is no code to be changed. Only the user information is incomplete now, because it lists all options settings as information. Only the new comments option would be missing.
Thats why the suggestion is to add these words to the existing static text line above
..."'Match Comments' = false"...

Hope the long text adds clarification rather than new confusion

[ollydbg]

no I did not mean "'Match Comments' = true". The line I meant is a static text line in the configuration panel telling the user what options are set when the context switch version of the command is used.

Oh, I does understand this is the option for the editor's context menu: Find occurrence of: xxx.

Myself I was not aware up to making the patch that the context menu search is different from the current parameters set in the options setting, but I can see that in some situations it might make sense.

So in order not not change the current behaviour any new option in the search should be disabled by default. And thats what is currently happening, so there is no code to be changed. Only the user information is incomplete now, because it lists all options settings as information. Only the new comments option would be missing.
Thats why the suggestion is to add these words to the existing static text line above
..."'Match Comments' = false"...

Hope the long text adds clarification rather than new confusion

Did you test when in the ThreadSearch option: "Use default options when use Find occurrences" is checked. I mean the default behavior(I mean the behavior without your patch) is that the C++ comments is also searched. So, the static text label should be:

'Match Comments' = true

Tell me if I'm wrong. Thanks.

[Tiger Beard]

Oh, your are right, I got it backwards. Ideed, the correct text for the default behaviour is "'Match Comments' = true".

Sorry for the confusion.

[Tiger Beard]

PS: the patch I am missing most for current head is 1437, the context menu addon for wxSmith. You were checking this out as well.
I got so used to that feature when working with dialogs that I found it hard to use trunk for esting with that feature missing.

[ollydbg]

I will review the patch #1437 soon, which I think is much simpler than #1436. Thanks.

[ollydbg]

It's in svn rev13452 now. Thanks.

[ollydbg]

The extra change of below is in rev13454:

diff --git a/src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp b/src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp
index 675d1977..8bb4230a 100644
--- a/src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp
+++ b/src/plugins/contrib/ThreadSearch/ThreadSearchConfPanel.cpp
@@ -406,7 +406,7 @@ void ThreadSearchConfPanel::do_layout()
         wxStaticBoxSizer* SizerThreadSearchOptions = new wxStaticBoxSizer(SizerThreadSearchOptions_staticbox, wxVERTICAL);
         SizerThreadSearchOptions->Add(m_pChkThreadSearchEnable, 0, wxALL, 4);
         SizerThreadSearchOptions->Add(m_pChkUseDefaultOptionsForThreadSearch, 0, wxALL, 4);
-        wxStaticText* m_pStaDefaultOptions = new wxStaticText(m_PageGeneral, wxID_ANY, _("       ('Whole word' = true, 'Start word' = false, 'Match case' = true, 'Regular expression' = false)"));
+        wxStaticText* m_pStaDefaultOptions = new wxStaticText(m_PageGeneral, wxID_ANY, _("       ('Whole word' = true, 'Start word' = false, 'Match case' = true, 'Match Comments' = true, 'Regular expression' = false)"));
         SizerThreadSearchOptions->Add(m_pStaDefaultOptions, 0, 0, 0);
         SizerThreadSearchOptions->Add(m_pChkShowMissingFilesError, 0, wxALL, 4);
         SizerThreadSearchOptions->Add(m_pChkShowCantOpenFileError, 0, wxALL, 4);

[ollydbg]

Oh, sorry, the r13455 fixes the build error by removing test code. I forget to merge(combine) the git commits in my local git branch to a single commit.

[ollydbg]

Hi, thanks for your contribution.

I just download the patch file, and open in an editor, I see that in the source code changes, there are some "TAB" chars, it should be all spaces in my mind.

I haven't applied the patch and test yet.

[Tiger Beard]

Thanbks for your efforts. I am sorry I was not aware that spaces are required.
Do you want me to redo the patch with spaces?

[ollydbg]

Oh, firefox just sent my old comment one month ago again.

sorry about that.