Author Topic: ThreadSearch plugin is born !  (Read 34370 times)

jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #45 on: March 18, 2007, 05:34:01 pm »
Nice plugin!  The preview idea is very cool (minimal tab clutter), thanks for sharing.  :)

I have some suggestions I hope you'll find useful, although it's possible they are already on your todo list...

Background colour
The hard coded XP background colour looks a bit out of place on Vista - no doubt the same is true on other platforms and themes.  This is an easy fix for win32 using 'GetSysColor' but I'm unsure if there is an equivalent function for other platforms (if one is even needed)... I found the following wxWidgets implementation, if colours are normally correct on other platforms then perhaps it's good enough?

Code
ThreadSearchView.cpp
..
..
void ThreadSearchView::set_properties()
{
// Set correct background colour on win32 systems
SetBackgroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_BTNFACE));
..

Search Results
The filename is not visible in the thread search results listview as the preceding path takes up what little space there is, an option to show only the filename along with saving column widths would be a huge improvement.

The preview normally shows the highlighted line at the bottom (depending on what was previously shown) - would it be possible for the line to be initially set in the middle each time?  ie, if 5 lines are visible, the 3rd line is the one highlighted with the search term.  A program I use called Search & Replace does this and it's good for quickly getting the context of the result.  It might be tricky knowing how many lines are visible but perhaps you could have an option which sets the amount of lines above the highlight - in this case you would set it to 2.

An improvement of lower priority would be to maximise the vertical space usable for search results as it is at such a premium in the messages window.  A good idea for this would be to (optionally?) relocate the controls to an advanced search dialogue and instead provide a search toolbar similar to visual studio etc.  The toolbar could be as simple as a combobox (pressing enter performs search with existing options) and a button which opens the new dialogue.

Bug report
Keyboard shortcuts (Ctrl+V/X/C/Z etc.) are sent to the active editor when caret is in the thread search combobox.  This is unexpected and awkward for pasting search terms etc.

Not really a bug but on my system a search which produces a lot of hits (eg. 'if') occupies the CPU such that codeblocks and (more annoyingly) the cancel search button is unresponsive until the search is complete.  I think it should be possible to correct this without incurring a significant performance hit.

Thanks again for sharing your work, I hope you appreciate my suggestions and I look forward to your views as well as any future releases.

Jamie
« Last Edit: March 18, 2007, 09:00:58 pm by jamieo »

Offline dje

  • Lives here!
  • ****
  • Posts: 683
Re: ThreadSearch plugin is born !
« Reply #46 on: March 18, 2007, 10:57:07 pm »
Hi all !

Sorry... I didn't know I was a bug  :) !

I'll replace all the Jérôme by Jerome !
Pecan, could you explain what this correction change ?

Quote
Even though the cbSTC is hidden, aren't GUI calls being made from this background thread?
Well, I do not have the answer to this interesting question.
As I use the view as parent during hidden cbSTC creation whereas it is not useful, I'll replace it by NULL to limit gui interactions:
Code
cbStyledTextCtrl* control = new cbStyledTextCtrl(m_pThreadSearchView, -1, wxDefaultPosition, wxSize(0, 0));
=>
Code
cbStyledTextCtrl* control = new cbStyledTextCtrl(NULL, -1, wxDefaultPosition, wxSize(0, 0));


Quote
Keyboard shortcuts (Ctrl+V/X/C/Z etc.) are sent to the active editor when caret is in the thread search combobox.  This is unexpected and awkward for pasting search terms etc.
This has been corrected with patch 1704 integration.
I think your nightly may be too old.


Quote
Not really a bug but on my system a search which produces a lot of hits (eg. 'if') occupies the CPU such that codeblocks and (more annoyingly) the cancel search button is unresponsive until the search is complete.  I think it should be possible to correct this without incurring a significant performance hit.
Yes, that's a limitation.
The worker thread sends events to the view to add found lines.
One event per file is sent.
As click or other events are enqueued after worker thread events, the application hangs if you search after an expression present in lots of files.
I would like to be able to specify a lower priority to the thread events but I think events handlers do not manage events priority.

Quote
wxSystemSettings::GetColour(wxSYS_COLOUR_BTNFACE)
This works with
Code
wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOW)
on my XP environment.
I'll test it (not today) on my Linux environment.

Quote
The filename is not visible in the thread search results listview as the preceding path takes up what little space there is, an option to show only the filename along with saving column widths would be a huge improvement.
Yes, I'll think about it. At first glance, it may be configurable so that user can choose between the two solutions (I think directory is often useful).
On XP, I just put the cursor on the file path so that the tool tip gives me the full path.

Quote
The preview normally shows the highlighted line at the bottom
I already saw that point. I'll try to improve it but I think that it's lower priority than many other points.


I really like the toolbar idea but my first priority is to make this plugin run on "other than Windows" platforms.


Finally, thanks for your positive feedback and your constructive remarks/propositions.

Dje

Offline Pecan

  • Plugin developer
  • Lives here!
  • ****
  • Posts: 2750
Re: ThreadSearch plugin is born !
« Reply #47 on: March 19, 2007, 01:08:56 am »
I have a fundamental, maybe stupid, question.

ThreadSearch does the searches using a hidden cbStyledTextCtrl.

Threads are not allowed to do GUI calls without carefull critical sections.

Even though the cbSTC is hidden, aren't GUI calls being made from this background thread?

The reason I ask the question is this from the wxWidgets manual:
Quote
::wxMutexGuiEnter
void wxMutexGuiEnter()

This function must be called when any thread other than the main GUI thread wants to get access to the GUI library. This function will block the execution of the calling thread until the main thread (or any other thread holding the main GUI lock) leaves the GUI library and no other thread will enter the GUI library until the calling thread calls ::wxMutexGuiLeave().

Typically, these functions are used like this:


void MyThread::Foo(void)
{
    // before doing any GUI calls we must ensure that this thread is the only
    // one doing it!

    wxMutexGuiEnter();

    // Call GUI here:
    my_window->DrawSomething();

    wxMutexGuiLeave();
}

Note that under GTK, no creation of top-level windows is allowed in any thread but the main one.
This function is only defined on platforms which support preemptive threads


When I attempt to run ThreadSearch under Linux, I get all sorts of freezes and segment violations as it tries to update/paint the cbStyledTextCtrl.

My thought is: that you cannot allocate a GUI control in a secondary thread. Nor can you cause it to update itself or paint. That's a gui call.

IE, you should not use a GUI editor in the secondary thread.

That may be why it crashes/hangs Linux CB.

I had this problem trying to write a wxTextCtrl log in KeyMacs. I had to completely remove the writes because they caused GUI calls to the wxTextCtrl.

I finally solved the problem with wxMutexGuiEnter/Leave, but I had to take the allocation of the text control out of the secondary thread.
« Last Edit: March 19, 2007, 01:10:56 am by Pecan »

jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #48 on: March 19, 2007, 02:38:17 am »
This has been corrected with patch 1704 integration.
I think your nightly may be too old.
Weird, my nightly (svn 3720) is from yesterday!   :shock:  I will download today's and try a clean install...

Update:   Downloaded CB_20070318_rev3721_win32, extracted it to it's own folder and added mingwm10.dll, wxmsw26u_gcc_cb.dll and your unmodified plugin.  Moved my profile folder elsewhere, started codeblocks, loaded a project, copied some text from the editor, clicked inside the threadsearch combobox, hit ctrl+v, text is pasted to my last position in the editor!  I'm on Vista so could somebody else please try ctrl+v in the combobox?

PS. I had a quick look at patch 1704 and it's for 'OnContextMenu' while my problem involves the keyboard shortcuts...

Quote
Yes, that's a limitation.
The worker thread sends events to the view to add found lines.
One event per file is sent.<snip>
I guessed that was the case, on win32 I have used 'WaitForSingleObject' with 1ms time-out to work around this.  The PSDK recommends 'WaitForMultipleObjects' if the wait is for any length of time but it is unnecessary when used with a short time-out.  In very fast loops I have set this up to execute on every nth iteration.  With a little tuning it should give the gui enough cycles to run smoothly with little impact on performance if any (although I have no numbers to prove it!).

Quote
wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOW)
Hmm.. are you saying COLOUR_BTNFACE is incorrect on your system for the beige xp coloured background as featured in your plugin and the screenshots previously in this thread?  I think COLOUR_WINDOW is an older pre-win95 constant - eg. the background of 'Program Manager' in Windows 3.11?!  On my system at least it results in a pure white background that looks a bit ugly and washed out (see attached png).  I don't have XP installed to check but looking at some software I've written in the past I've always used 'COLOR_BTNFACE' or it's alias 'COLOR_3DFACE' to match the same themed background you had hard coded.  Also, a drawback of using a colour that doesn't match the themed background is that themed buttons will have a 'halo' of colour around the button that won't match (please see magnified examples in the png).

Here's what the PSDK says about these constants.
Code
COLOR_3DFACE = 15
Face color for three-dimensional display elements and for dialog box backgrounds.
COLOR_BTNFACE = 15
Face color for three-dimensional display elements and for dialog box backgrounds.
COLOR_WINDOW = 5
Window background.

Quote
it may be configurable so that user can choose between the two solutions (I think directory is often useful).
Yes, an option for either would be great.

Quote
Quote
The preview normally shows the highlighted line at the bottom
I already saw that point. I'll try to improve it but I think that it's lower priority than many other points.

I really like the toolbar idea but my first priority is to make this plugin run on "other than Windows" platforms.

Finally, thanks for your positive feedback and your constructive remarks/propositions.
As I said, the toolbar idea would be a low priority suggestion as it is purely a gui thing and would be more than just a small mod.  I'm glad you appreciate my feedback, perhaps I'll look into some of the low priority ideas (eg. highlighted line) to see if I can contribute a patch or two, if you agree.  Although, I have not done any wxWidgets so don't count on anything great, anytime soon!   :lol:

Jamie

[attachment deleted by admin]
« Last Edit: March 19, 2007, 06:09:26 pm by jamieo »

jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #49 on: March 19, 2007, 03:54:00 am »
Here's a suggestion for achieving a consistent line position in the search preview and having it offset an optional number of lines.

You do have to have another 'GotoLine' but as I believe they both occur before a repaint there should be no impact on update speed.  Certainly on my machine (1.7Ghz Centrino Laptop) it was just as quick as before - which is instant!  Optimally, you would use an 'if' condition to check the initial 'GotoLine' as it's only required if you previously viewed the same file from a position later in the file - but as there was no noticeable performance hit I kept is simple for the sake of clarity.

The attached png shows the result of previewing a line.  So far while using the up/down arrows to traverse (quickly) up and down the results the line is always bang in the middle.  :)

Code
ThreadSearchView.cpp

In ThreadSearchView::UpdatePreview replace:

// Display the selected line
m_pSearchPreview->GotoLine(line);

with:

// Display the selected line // Preview sets position inconsistently depending on the previous
m_pSearchPreview->GotoLine(0); // position so first, set line to a position before the destination,
m_pSearchPreview->GotoLine(line+2); // then set it to the destination postion + any desired offset.

Jamie

[attachment deleted by admin]
« Last Edit: March 19, 2007, 04:20:38 am by jamieo »

Offline dje

  • Lives here!
  • ****
  • Posts: 683
Re: ThreadSearch plugin is born !
« Reply #50 on: March 20, 2007, 02:02:36 am »
Hi all !

The 0.5 version of the ThreadSearch plugin is available.
What's new ??
  • All Jérôme replaced by Jerome
  • Added code to center line in preview editor on list single click.
  • Added wxMutexGuiEnter() and wxMutexGuiLeave()
  • Hard coded background color replaced by wxSystemSettings::GetColour(wxSYS_COLOUR_BTNFACE)

Files:


Remarks:
  • BIG THANKS TO PECAN FOR MAKING ME KNOW ABOUT wxMutexGui... CALLS. That way, the plugin became far more stable on my Ubuntu. No crashes appended since I used those mutexes. Next Linux problem : it seems that wxGlade generated code does not behave the same way : the height of the editor/list is not managed correctly (scrollbars scale).
  • line centering code is taken from cbEditor::GotoLine. Jamie, I thank you for the solution you proposed but I think this one is better.
  • Thanks to Jamie, I use wxSYS_COLOUR_BTNFACE instead of wxSYS_COLOUR_WINDOW. I found that wxSYS_COLOUR_WINDOW was more appropriate in wxWidgets 2.6.3 documentation but it is nicer with wxSYS_COLOUR_BTNFACE

Quote
text is pasted to my last position in the editor
Well... On XP, I have no problem but different problems appended on Ubuntu.
I'll have to dive into keyboard shortcuts management and event skipping...

Quote
I guessed that was the case, on win32 I have used 'WaitForSingleObject' with 1ms time-out to work around this
I had thought about this solution. I'll implement it but I give it a low priority, as I think people do not often search for 'if' or 'wxString'.

Dje

jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #51 on: March 20, 2007, 03:02:23 am »
Good work dje, I agree that your solution is better for the preview as it automatically centres regardless of control height (plus mine was a bit of a hack just to show an idea ;)).

I hope you don't mind but I have attached a patch which implements some ideas wrt maximising the use of space available, the results of which can be seen in the attached png.  It would be nice if you'd consider adding these ideas - possibly as a few optional settings tucked away in the config dialog?

Also, I've noted a couple of minor issues below - if I get time I'll look into them if you like?

  • Searching a file that has been modified (and saved) since previously previewed produces correct search results but the preview is not re-loaded to show the changes.  To recreate search a file and preview the result so the file is loaded.  Then, type some text into the same file, save, and then search for that text.  The result for the new text will be listed but when clicked upon it will be missing from the preview.   I've over complicated the explanation a bit but basically the preview needs to be reloaded if the file has been saved since last previewed.

  • Some items are not sorted as expected.  For an example, please see the attached png - it seems the the results are mostly sorted except for the odd result from another file that interrupts the series.

Quote
I think people do not often search for 'if' or 'wxString'.
I think so too but it's damn annoying when you do and you can't cancel!!   :lol:

Jamie

[attachment deleted by admin]
« Last Edit: March 20, 2007, 03:27:42 am by jamieo »

Offline dje

  • Lives here!
  • ****
  • Posts: 683
Re: ThreadSearch plugin is born !
« Reply #52 on: March 20, 2007, 08:39:56 am »
Hi !

Quote
possibly as a few optional settings tucked away in the config dialog?
OK, that's what I'll do. I'll keep current settings as default because they match the 'Search results' panel ones.

Quote
Some items are not sorted as expected.
I had already noticed that point.
It happens only if you checked both 'Open files' and 'Project files' (as it seems to be on your picture).
For now, a wxSortedArrayString gathers all files from the different search scopes then the search is performed on each file.
All found items are appended to list control. I don't see for now why it behaves that why (but I didn't have time to debug it).
It could help me if you could have a look !

Quote
I think so too but it's damn annoying when you do and you can't cancel!!
I'll improve it !

Quote
Searching a file that has been modified (and saved) since previously previewed produces correct search results but the preview is not re-loaded to show the changes.

I noticed the 'not reloading' problem.
It can be a little tricky to look for file change, reload file and perform a list and preview refresh. How can you be sure to get the good line, I mean suppose you get current selected list item, you are not sure the same text won't be inserted, the text preview may not be focused on the selected item. In fact, I think it is a very hard thing to synchronize preview with file change and to be sure to do it well !
I chose this strategy because of the previous difficulty and because this strategy is the same as C::B 'Search in files'.

I know I often say 'I already saw that point', but it is true I developped this on a long time and tested it so that I thought and found lots of things (either minor problems or evolutions) I didn't write to avoid beginning the post with 200 pages  :D

Dje


jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #53 on: March 20, 2007, 07:39:50 pm »
OK, that's what I'll do. I'll keep current settings as default because they match the 'Search results' panel ones.
Cool, I look forward to your next build!  :)

Quote
It happens only if you checked both 'Open files' and 'Project files'.
Ah...  I had not investigated it but can confirm unticking 'Open files' does the trick for now, cheers...  I will try to look into it.

Quote
I think it is a very hard thing to synchronize preview with file change and to be sure to do it well !
I didn't explain it very well so I think we misunderstand each other.  I'm not referring to unsaved changes made to a file, I mean if a change is made and then saved but the older version is already in the preview it won't get updated until you first preview a different file because of the following code which is there for good reason.

Code
// Loads file if different from current loaded
if ( m_PreviewFilePath != sFile )

I have two suggestions to improve this (neither I know how to do in wxWidgets  - I'll look into it, but you may be able to do it more easily)...

Either save the file modified time with the file name and then compare them as well, ex:

Code
// Loads file if different from current loaded
if ( (m_PreviewFilePath != sFile) || ( newFiletime != oldFiletime ) )

Or, hook into codeblocks somewhere so whenever a file is saved you reload your preview if it's the same file.  I'm sure there are other ways..

Quote
I know I often say 'I already saw that point', but it is true.
Hey, I don't mind - the points I've raised are of the more obvious, usability type so that's why I started my first post saying 'it's possible they are already on your todo list...'   :)
« Last Edit: March 20, 2007, 08:28:57 pm by jamieo »

jamieo

  • Guest
Re: ThreadSearch plugin is born !
« Reply #54 on: March 20, 2007, 09:04:56 pm »
Here's a patch that'll hopefully explain the file preview thing better (and fix it too!) ;)

To test:
  • Perform a search so the preview is loaded with a file
  • In the same file that is currently previewed, type some obscure text
  • Save the file!
  • Right click the new text and 'find occurrences...'

The new text should be listed as a search result but won't be previewed as the file isn't reloaded, the purpose of this patch is to fix that.  :)

Jamie

[attachment deleted by admin]

Offline dje

  • Lives here!
  • ****
  • Posts: 683
Re: ThreadSearch plugin is born !
« Reply #55 on: March 21, 2007, 11:19:34 pm »
BUG ALERT !!!

When a long search is performed (big directory), the application hangs on ui events until the thread is finished.

Sorry, probably due to the wxMutexGui... added.
I investigate...

Concerns only v 0.5.

Dje

Offline Pecan

  • Plugin developer
  • Lives here!
  • ****
  • Posts: 2750
Re: ThreadSearch plugin is born !
« Reply #56 on: March 22, 2007, 12:48:03 am »
BUG ALERT !!!

When a long search is performed (big directory), the application hangs on ui events until the thread is finished.

Sorry, probably due to the wxMutexGui... added.
I investigate...

Concerns only v 0.5.

Dje

I could be wrong about this, but....

I believe the fact that a GUI control (styledTextCtrl) is being instantiated and used in a secondary thread is the real problem.

Even though the control is "hidden" it still calls Update, Paint, SetFont, etc. It may not actually do the paint or setfont, but they're still called.

It makes cursor calls to bink the cursor, it makes paint and setfont calls when a file is loaded into its control, it makes update calls from OnIdle etc etc.

I found that I could get away with a lot of GUI calls in MSwindows in a secondary thread without a hang, but when I moved to Linux, it crashed and burned constantly.

I hope you can solve your problem, but I just want to mention this to keep you aware of the downside.

mariocup

  • Guest
Re: ThreadSearch plugin is born !
« Reply #57 on: March 29, 2007, 08:34:04 am »
Hi,

it is a possible to mark text within the editor and then use the contextual menu: find occurance of: <selected text>.
I think it would be easier to use a short cut like Ctrl+Alt+F (like Search->Ffind) to put the selection directly to the Thread Search and then pressing return to get the search results.

Would this be much effort?

Bye,

Mario

Offline dje

  • Lives here!
  • ****
  • Posts: 683
Re: ThreadSearch plugin is born !
« Reply #58 on: March 29, 2007, 08:56:23 am »
Hi all !

I'm not dead !
I am working on the hanging bug !

Quote
it is a possible to mark text within the editor and then use the contextual menu: find occurance of: <selected text>.
I can change the default behaviour (find the word under cursor) to 'if selection, find selection else find the word under cursor'.
Is this what you would like ?

Quote
I think it would be easier to use a short cut like Ctrl+Alt+F (like Search->Ffind) to put the selection directly to the Thread Search and then pressing return to get the search results.
The Search->Thread search menu item will be modified.
I hesitate between running tne contextual 'Find occurrences' or your suggestion.

Dje