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

["Roger C. Soares" <rogersoares@intelinet.com.br>]

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.

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.

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? ;)


> This field isn't necessary.  "table.getTable()" will get you the
> same widget.
>   
Ok.


> 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.

Robin sent a patch some time ago to change those new Integer() to 
Integer.valueOf(), I guess he didn't push it yet.


> These strings should be in UIText, and UIText.properties, so they
> can be translated strings.
>   
Ok.


> 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.


> Shouldn't this maybe be a Job instead, and scheduled on the History
> site so the History title bar goes italics and the user can see
> the progress meter in the status bar of the workspace and in their
> Progress view?
>   
Maybe. When I wrote this I wasn't aware about the existence of the Job 
interface. Then when I met it I thought about replacing it, but havent't 
done it yet.


> I'm not an SWT expert, but I think hanging onto a Display reference
> from a class static isn't a good idea.
>   
You're right.


> Shouldn't this be a volatile, or an AtomicInteger, or protected by
> a synchronized block?
>   
Yep.


> Wow.  That's black magic.  You are blocking the threads from
> doing multiple searches at once by synchonizing on the static
> method, but since you are in an instance method you had to pass
> everything through as arguments.  :-|
>
> It would be a lot easier to follow if this was an instance member,
> and thus had access to the instance fields, and if you used an
> explicit lock, e.g.:
>
> 	private static final Object EXEC_LOCK = new Object();
>
> 	public void run() {
> 		synchronized (EXEC_LOCK) {
> 			execFind();
> 		}
> 	}
>
> 	private void execFind() { ... }
>   
Yep, looks better.

[]s,
Roger.