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

"Roger C. Soares" [off-list ref] 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.
 

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

...

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

...

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

+		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?  

...

+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?

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