Re: [EGIT PATCH 2/4] FindToolbar port to the new history page.

From: Shawn O. Pearce <spearce@spearce.org>
Date: Apr 1, 2008, 04:02 UTC
Message-ID: <20080401040241.GQ10274@spearce.org>
In-Reply-To: <47F1AF86.3030503@intelinet.com.br>

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:

Show 10 quoted lines

> Shawn O. Pearce escreveu:
> >Hmm.  So what functionality did the highlight flag + RevFilter
> >not get you?  It supports both regex as well as non-regex matches,
> >is quick, and can be joined together with other filters.  A lot of
> >the code in the FindToolbarThread should drop out.
> 
> Ok, so, my motivation first. I don't have too much time to work on egit 
> but I'm interested in using your work in my build. So, I'm pushing the 
> features I need. This patch was intended as a simple port of the 
> existing FindToolbar to the new history page so I can use it.

OK, that makes sense. Under that basis I'm willing to take your port in, especially if the other items I mentioned that you said "Ok" to were cleaned up.

Show 6 quoted lines

> I tried the applyFlags you described but it doesn't have a monitor 
> approach to give feedback to the toolbar so it knows when to refresh the 
> table and to select the first match, or to go red when nothing was 
> found. I also couldn't find from the highlight flag + RevFilter solution 
> how to get the total rows encountered and the index of a match so the 
> toolbar can show that the selected match is number 2 from 10.

OK.  Major gaps in the jgit API.  I now understand better what you
were needing here.  I'll probably go another around on that API
soon and see if I can't update your port once I have these things
down at the jgit level.

> So, this patch was intented as a port. I'm not sure everything related 
> to search should go inside jgit, but I agree that RevFilters should be 
> reused. I was thinking about it as an improvement after the port, it's 
> not my priority right now but someone else can do it? ;)

Right.  :)

Show 8 quoted lines

> >So this is doing basically the same thing as the highlight RevFlag
> >(give a boolean about match status for a given RevCommit) but needs
> >to consult a HashMap by creating a temporary boxed Integer, and this
> >is deep down inside of the painting code for the table.  Urrgh.
> 
> The map is used to give the x from total information. When using a 
> VIRTUAL table it doesn't have a noticable performance impact because 
> only a small set is required at a time.

Hmm. Not really. We're still beating on that paint listener every time the screen needs to draw. I don't think SWT is double buffering the table either, so every redraw event is coming through this code. Be nice if we didn't have to suffer through a HashMap hit every time.

> >Would it make sense to abstract out and reuse the BooleanPrefAction
> >class I added to GitHistoryPage in ea3f1e7a7684b8?  
> >  
> Probably, I'll get a look on it.

This is maybe something to clean up later, after the port is initially in my tree.

-- 
Shawn.