Re: [EGIT PATCH] showing commiter and parent commit(s) on the revision detail viewer

From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
Date: Jan 27, 2008, 00:34 UTC
Message-ID: <200801270134.11892.robin.rosenberg.lists@dewire.com>
In-Reply-To: <1201262529-3306-1-git-send-email-rogersoares@intelinet.com.br>

Hi Roger,

The general idea is ok. Some minor coments though.

>Re: [EGIT PATCH] showing commiter and parent commit(s) on the revision detail viewer

"Show" and final period.

fredagen den 25 januari 2008 skrev Roger C. Soares:

Show 14 quoted lines

> @@ -221,14 +223,16 @@ public class GitHistoryPage extends HistoryPage implements IAdaptable,
>  				if(selection2.length == 1) {
>  					// if the table item is not visible in the UI and it's selected via keyboard
>  					// this listener is called before the listener that sets the item data.
> -					if(selection2[0] == null) {
> +					GitCommitFileRevision revision = (GitCommitFileRevision) selection2[0];
> +					if(revision == null) {
>  						int ix = table.getSelectionIndex();
> -						GitFileRevision revision = (GitFileRevision) fileRevisions.get(ix);
> +						revision = (GitCommitFileRevision) fileRevisions.get(ix);
>  						selection2[0] = revision;
>  					}
> -					setRevisionInfoTextViewers(selection2[0]);
> +					setRevisionInfoTextViewers(revision);

I'm rather allergic to casts, so if we could have only one I'd feel better. Something like

					IFileRevision revision = selection2[0];
					if(revision == null) {
 						int ix = table.getSelectionIndex();
						revision = fileRevisions.get(ix);
 						selection2[0] = revision;
 					}
					setRevisionInfoTextViewers((GitCommitFileRevision)revision);

>  				}
>  
> +

two lines of whitepspace

>  		if (revisionInfo.length() == 0) {
> -			revisionInfo.append("Commit: ");
> +			revisionInfo.append("Commit ID: ");

I prefer just commit. It's shorter and that it is the id is obvious.

> +		revisionInfo.append(formatPersonIdentForRevInfo(commit.getAuthor()));
> +		revisionInfo.append("\nCommiter: ");

Two t's in "Committer"

> +				} catch (IOException e) {
> +					e.printStackTrace();

We should start doing error handling better. But that's a separate chapter. Add a // TODO for now. I'll address them one all one one go later, unless someone does it for me.

-- robin