Re: [EGIT PATCH 2/4] FindToolbar port to the new history page.
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Mar 31, 2008, 06:19 UTC
- Message-ID
- <20080331061914.GK10274@spearce.org>
- In-Reply-To
- <1206890325-3732-1-git-send-email-rogersoares@intelinet.com.br>
"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:
> The find toolbar already generates a list of table rows that need > highlighting. The hightlight flag + RevFilter solution currently > doesn't support all the features that the find toolbar has, so for > this port the toolbar is replacing the highlight flag.
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.
Show 6 quoted lines
> @@ -71,6 +70,8 @@ class CommitGraphTable {
>
> private final TableViewer table;
>
> + private final Table rawTable;
> +This field isn't necessary. "table.getTable()" will get you the same widget.
Show 6 quoted lines
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindResults.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindResults.java > new file mode 100644 > index 0000000..0202a70 > --- /dev/null > +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindResults.java > @@ -0,0 +1,184 @@
...
Show 7 quoted lines
> +public class FindResults {
> + private Map<Integer, Integer> matchesMap = new LinkedHashMap<Integer, Integer>();
> +
> + Integer[] keysArray;
> +
> + private int matchesCount;
> +...
> + public synchronized boolean isFoundAt(int index) {
> + return matchesMap.containsKey(new Integer(index));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.
Show 6 quoted lines
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbar.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbar.java > new file mode 100644 > index 0000000..eae0cc4 > --- /dev/null > +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbar.java > @@ -0,0 +1,457 @@
...
> +public class FindToolbar extends Composite {...
> + private void createToolbar() {...
> + final ToolItem prefsItem = new ToolItem(toolBar, SWT.DROP_DOWN);
> + final Menu prefsMenu = new Menu(this.getShell(), SWT.POP_UP);
> + final MenuItem caseItem = new MenuItem(prefsMenu, SWT.CHECK);
> + caseItem.setText("Ignore case");These strings should be in UIText, and UIText.properties, so they can be translated strings.
Show 10 quoted lines
> + committerItem.addSelectionListener(new SelectionAdapter() {
> + public void widgetSelected(SelectionEvent e) {
> + prefs.setValue(UIPreferences.FINDTOOLBAR_COMMITTER,
> + committerItem.getSelection());
> + Activator.getDefault().savePluginPreferences();
> + clear();
> + }
> + });
> + committerItem.setSelection(prefs
> + .getBoolean(UIPreferences.FINDTOOLBAR_COMMITTER));Would it make sense to abstract out and reuse the BooleanPrefAction class I added to GitHistoryPage in ea3f1e7a7684b8?
Show 6 quoted lines
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbarThread.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbarThread.java > new file mode 100644 > index 0000000..931f82b > --- /dev/null > +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/FindToolbarThread.java > @@ -0,0 +1,237 @@
...
> +public class FindToolbarThread extends Thread {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?
> + private static Display display = Display.getDefault();
I'm not an SWT expert, but I think hanging onto a Display reference from a class static isn't a good idea.
> + private static int globalThreadIx = 0;
Shouldn't this be a volatile, or an AtomicInteger, or protected by a synchronized block?
Show 10 quoted lines
> + public void run() {
> + execFind(currentThreadIx, fileRevisions, pattern, toolbar, ignoreCase,
> + findInCommitId, findInComments, findInAuthor, findInCommitter);
> + }
> +
> + private synchronized static void execFind(int threadIx,
> + SWTCommit[] fileRevisions, final String pattern,
> + final FindToolbar toolbar, boolean ignoreCase,
> + boolean findInCommitId, boolean findInComments,
> + boolean findInAuthor, boolean findInCommitter) {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() { ... }-- Shawn.