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:
> @@ -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
-
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
