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

["Shawn O. Pearce" <spearce@spearce.org>]

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

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

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

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

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

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