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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
