From 59b2ea066d07ef923e182022e9ed590ac93ed69f Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 20 Nov 2013 01:23:20 -0800 Subject: [PATCH] ChangeScreen2: Show inline comments in history When a history comment is expanded include the inline comments written on that revision by the author at the same timestamp. This makes it easier to follow a change's evolution over time, especially if the reader is new to the change and does not have the entire email thread in their inbox. Bug: issue 93 Change-Id: I57fe29fea2b3651306ff0a58746cefcfe1669267 --- .../gerrit/client/change/ChangeScreen2.java | 16 +- .../gerrit/client/change/ChangeScreen2.ui.xml | 2 +- .../gerrit/client/change/FileComments.java | 83 +++++++++ .../gerrit/client/change/FileComments.ui.xml | 37 ++++ .../google/gerrit/client/change/History.java | 172 ++++++++++++++++++ .../gerrit/client/change/LineComment.java | 46 +++++ .../gerrit/client/change/LineComment.ui.xml | 40 ++++ .../google/gerrit/client/change/Message.java | 78 +++++++- .../gerrit/client/change/Message.ui.xml | 1 + .../gerrit/client/changes/ChangeInfo.java | 1 + .../gerrit/client/changes/CommentInfo.java | 4 +- 11 files changed, 462 insertions(+), 18 deletions(-) create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.ui.xml create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.java index a1c7a0c860..3b67d157d0 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.java @@ -71,7 +71,6 @@ import com.google.gwt.user.client.DOM; import com.google.gwt.user.client.EventListener; import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwt.user.client.ui.Button; -import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.HTMLPanel; import com.google.gwt.user.client.ui.Image; import com.google.gwt.user.client.ui.ToggleButton; @@ -152,7 +151,7 @@ public class ChangeScreen2 extends Screen { @UiField CommitBox commit; @UiField RelatedChanges related; @UiField FileTable files; - @UiField FlowPanel history; + @UiField History history; @UiField Button includedIn; @UiField Button revisions; @@ -516,6 +515,7 @@ public class ChangeScreen2 extends Screen { private List>> loadComments( RevisionInfo rev, CallbackGroup group) { + final int id = rev._number(); final List>> r = new ArrayList>>(1); ChangeApi.revision(changeId.get(), rev.name()) @@ -524,6 +524,7 @@ public class ChangeScreen2 extends Screen { @Override public void onSuccess(NativeMap> result) { r.add(result); + history.addComments(id, result); } @Override @@ -652,7 +653,6 @@ public class ChangeScreen2 extends Screen { renderOwner(info); renderActionTextDate(info); - renderHistory(info); initIncludedInAction(info); initRevisionsAction(info, revision); initDownloadAction(info, revision); @@ -671,6 +671,7 @@ public class ChangeScreen2 extends Screen { related.set(info, revision); reviewers.set(info); quickApprove.set(info, revision); + history.set(commentLinkProcessor, changeId, info); if (Gerrit.isSignedIn()) { initEditMessageAction(info, revision); @@ -727,15 +728,6 @@ public class ChangeScreen2 extends Screen { actionDate.setInnerText(FormatUtil.relativeFormat(info.updated())); } - private void renderHistory(ChangeInfo info) { - JsArray messages = info.messages(); - if (messages != null) { - for (int i = 0; i < messages.length(); i++) { - history.add(new Message(commentLinkProcessor, messages.get(i))); - } - } - } - void showUpdates(ChangeInfo newInfo) { if (!isAttached() || newInfo.updated().equals(lastDisplayedUpdate)) { return; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.ui.xml index 49883ee34c..63787e2a32 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.ui.xml @@ -411,6 +411,6 @@ limitations under the License. - + diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java new file mode 100644 index 0000000000..9dbf994919 --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java @@ -0,0 +1,83 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client.change; + +import com.google.gerrit.client.Gerrit; +import com.google.gerrit.client.changes.CommentInfo; +import com.google.gerrit.client.ui.CommentLinkProcessor; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gwt.core.client.GWT; +import com.google.gwt.event.dom.client.ClickEvent; +import com.google.gwt.uibinder.client.UiBinder; +import com.google.gwt.uibinder.client.UiField; +import com.google.gwt.uibinder.client.UiHandler; +import com.google.gwt.user.client.ui.Anchor; +import com.google.gwt.user.client.ui.Composite; +import com.google.gwt.user.client.ui.FlowPanel; +import com.google.gwt.user.client.ui.HTMLPanel; +import com.google.gwtorm.client.KeyUtil; + +import java.util.Collections; +import java.util.Comparator; +import java.util.List; + +class FileComments extends Composite { + interface Binder extends UiBinder {} + private static final Binder uiBinder = GWT.create(Binder.class); + + private final String url; + @UiField Anchor path; + @UiField FlowPanel comments; + + FileComments(CommentLinkProcessor clp, + PatchSet.Id ps, + String title, + List list) { + initWidget(uiBinder.createAndBindUi(this)); + + url = url(ps, list.get(0)); + path.setHref("#" + url); + path.setText(title); + + Collections.sort(list, new Comparator() { + @Override + public int compare(CommentInfo a, CommentInfo b) { + return a.line() - b.line(); + } + }); + for (CommentInfo c : list) { + comments.add(new LineComment(clp, c)); + } + } + + @UiHandler("path") + void onClick(ClickEvent e) { + e.preventDefault(); + e.stopPropagation(); + Gerrit.display(url); + } + + private static String url(PatchSet.Id ps, CommentInfo info) { + // TODO(sop): Switch to Dispatcher.toPatchSideBySide. + Change.Id c = ps.getParentKey(); + return new StringBuilder() + .append("/c/").append(c.get()).append('/') + .append(ps.get()).append('/') + .append(KeyUtil.encode(info.path())) + .append(",cm") + .toString(); + } +} diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.ui.xml new file mode 100644 index 0000000000..9a7564f690 --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.ui.xml @@ -0,0 +1,37 @@ + + + + + .box { + } + .path { + display: block; + white-space: nowrap; + } + .comments { + margin-left: 1em; + } + + + + + + + diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java new file mode 100644 index 0000000000..e043e3c1ee --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java @@ -0,0 +1,172 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client.change; + +import com.google.gerrit.client.account.AccountInfo; +import com.google.gerrit.client.changes.ChangeApi; +import com.google.gerrit.client.changes.ChangeInfo; +import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; +import com.google.gerrit.client.changes.CommentInfo; +import com.google.gerrit.client.rpc.NativeMap; +import com.google.gerrit.client.rpc.Natives; +import com.google.gerrit.client.ui.CommentLinkProcessor; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gwt.core.client.JsArray; +import com.google.gwt.user.client.rpc.AsyncCallback; +import com.google.gwt.user.client.ui.FlowPanel; +import com.google.gwt.user.client.ui.Widget; + +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +class History extends FlowPanel { + private CommentLinkProcessor clp; + private Change.Id changeId; + + private final Set loaded = new HashSet(); + private final Map> byAuthor = + new HashMap>(); + + void set(CommentLinkProcessor clp, Change.Id id, ChangeInfo info) { + this.clp = clp; + this.changeId = id; + + JsArray messages = info.messages(); + if (messages != null) { + for (MessageInfo msg : Natives.asList(messages)) { + Message ui = new Message(this, msg); + if (loaded.contains(msg._revisionNumber())) { + ui.addComments(comments(msg)); + } + add(ui); + } + } + } + + CommentLinkProcessor getCommentLinkProcessor() { + return clp; + } + + Change.Id getChangeId() { + return changeId; + } + + void addComments(int id, NativeMap> map) { + loaded.add(id); + + for (String path : map.keySet()) { + for (CommentInfo c : Natives.asList(map.get(path))) { + c.setPath(path); + if (c.author() != null) { + AuthorRevision k = new AuthorRevision(c.author(), id); + List l = byAuthor.get(k); + if (l == null) { + l = new ArrayList(); + byAuthor.put(k, l); + } + l.add(c); + } + } + } + } + + void load(final int revisionNumber) { + if (revisionNumber > 0 && loaded.add(revisionNumber)) { + ChangeApi.revision(new PatchSet.Id(changeId, revisionNumber)) + .view("comments") + .get(new AsyncCallback>>() { + @Override + public void onSuccess(NativeMap> result) { + addComments(revisionNumber, result); + update(revisionNumber); + } + + @Override + public void onFailure(Throwable caught) { + } + }); + } + } + + private void update(int revisionNumber) { + for (Widget child : getChildren()) { + Message ui = (Message) child; + MessageInfo info = ui.getMessageInfo(); + if (info._revisionNumber() == revisionNumber) { + ui.addComments(comments(info)); + } + } + } + + private List comments(MessageInfo msg) { + if (msg.author() == null) { + return Collections.emptyList(); + } + + AuthorRevision k = new AuthorRevision(msg.author(), msg._revisionNumber()); + List list = byAuthor.get(k); + if (list == null) { + return Collections.emptyList(); + } + + Timestamp when = msg.date(); + List match = new ArrayList(); + List other = new ArrayList(); + for (int i = 0; i < list.size(); i++) { + CommentInfo c = list.get(i); + if (c.updated().compareTo(when) <= 0) { + match.add(c); + } else { + other.add(c); + } + } + if (match.isEmpty()) { + return Collections.emptyList(); + } else if (other.isEmpty()) { + byAuthor.remove(k); + } else { + byAuthor.put(k, other); + } + return match; + } + + private static final class AuthorRevision { + final int author; + final int revision; + + AuthorRevision(AccountInfo author, int revision) { + this.author = author._account_id(); + this.revision = revision; + } + + @Override + public int hashCode() { + return author * 31 + revision; + } + + @Override + public boolean equals(Object o) { + AuthorRevision b = (AuthorRevision) o; + return author == b.author && revision == b.revision; + } + } +} diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java new file mode 100644 index 0000000000..2c3e14e088 --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java @@ -0,0 +1,46 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client.change; + +import com.google.gerrit.client.changes.CommentInfo; +import com.google.gerrit.client.changes.Util; +import com.google.gerrit.client.ui.CommentLinkProcessor; +import com.google.gwt.core.client.GWT; +import com.google.gwt.dom.client.Element; +import com.google.gwt.uibinder.client.UiBinder; +import com.google.gwt.uibinder.client.UiField; +import com.google.gwt.user.client.ui.Composite; +import com.google.gwt.user.client.ui.HTMLPanel; +import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; + +class LineComment extends Composite { + interface Binder extends UiBinder {} + private static final Binder uiBinder = GWT.create(Binder.class); + + @UiField Element location; + @UiField Element message; + + LineComment(CommentLinkProcessor clp, CommentInfo info) { + initWidget(uiBinder.createAndBindUi(this)); + + location.setInnerText(info.has_line() + ? Util.M.lineHeader(info.line()) + : Util.C.fileCommentHeader()); + if (info.message() != null) { + message.setInnerSafeHtml(clp.apply(new SafeHtmlBuilder() + .append(info.message().trim()).wikify())); + } + } +} diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml new file mode 100644 index 0000000000..9c4935a57c --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml @@ -0,0 +1,40 @@ + + + + + .box { + position: relative; + } + .location { + position: absolute; + top: 0; + left: 0; + font-weight: bold; + } + .message { + margin-left: 100px; + } + + + +
+
+ + diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java index 6fcabeea09..4477665a34 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java @@ -18,8 +18,11 @@ import com.google.gerrit.client.AvatarImage; import com.google.gerrit.client.FormatUtil; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; +import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.changes.Util; import com.google.gerrit.client.ui.CommentLinkProcessor; +import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.GWT; import com.google.gwt.dom.client.Element; import com.google.gwt.event.dom.client.ClickEvent; @@ -28,10 +31,17 @@ import com.google.gwt.resources.client.CssResource; import com.google.gwt.uibinder.client.UiBinder; import com.google.gwt.uibinder.client.UiField; import com.google.gwt.user.client.ui.Composite; +import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.HTMLPanel; import com.google.gwt.user.client.ui.UIObject; import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; + class Message extends Composite { interface Binder extends UiBinder {} private static final Binder uiBinder = GWT.create(Binder.class); @@ -46,11 +56,16 @@ class Message extends Composite { @UiField Element summary; @UiField Element date; @UiField Element message; + @UiField FlowPanel comments; + + private final History history; + private final MessageInfo info; + private List commentList; @UiField(provided = true) AvatarImage avatar; - Message(CommentLinkProcessor clp, MessageInfo info) { + Message(History parent, MessageInfo info) { if (info.author() != null) { avatar = new AvatarImage(info.author()); avatar.setSize("", ""); @@ -66,23 +81,40 @@ class Message extends Composite { } }, ClickEvent.getType()); + this.history = parent; + this.info = info; + name.setInnerText(authorName(info)); date.setInnerText(FormatUtil.shortFormatDayTime(info.date())); if (info.message() != null) { String msg = info.message().trim(); summary.setInnerText(msg); - message.setInnerSafeHtml(clp.apply( - new SafeHtmlBuilder().append(msg).wikify())); + message.setInnerSafeHtml(history.getCommentLinkProcessor() + .apply(new SafeHtmlBuilder().append(msg).wikify())); } } + MessageInfo getMessageInfo() { + return info; + } + private boolean isOpen() { return UIObject.isVisible(message); } void setOpen(boolean open) { + if (open && info._revisionNumber() > 0) { + if (commentList == null) { + history.load(info._revisionNumber()); + } else if (!commentList.isEmpty()) { + renderComments(commentList); + commentList = Collections.emptyList(); + } + } + UIObject.setVisible(summary, !open); UIObject.setVisible(message, open); + comments.setVisible(open && comments.getWidgetCount() > 0); if (open) { removeStyleName(style.closed()); } else { @@ -90,6 +122,46 @@ class Message extends Composite { } } + void addComments(List list) { + if (isOpen()) { + renderComments(list); + comments.setVisible(comments.getWidgetCount() > 0); + commentList = Collections.emptyList(); + } else { + commentList = list; + } + } + + private void renderComments(List list) { + CommentLinkProcessor clp = history.getCommentLinkProcessor(); + PatchSet.Id ps = new PatchSet.Id( + history.getChangeId(), + info._revisionNumber()); + TreeMap> m = byPath(list); + List l = m.remove(Patch.COMMIT_MSG); + if (l != null) { + comments.add(new FileComments(clp, ps, Util.C.commitMessage(), l)); + } + for (Map.Entry> e : m.entrySet()) { + comments.add(new FileComments(clp, ps, e.getKey(), e.getValue())); + } + } + + private static TreeMap> + byPath(List list) { + TreeMap> m = + new TreeMap>(); + for (CommentInfo c : list) { + List l = m.get(c.path()); + if (l == null) { + l = new ArrayList(); + m.put(c.path(), l); + } + l.add(c); + } + return m; + } + static String authorName(MessageInfo info) { if (info.author() != null) { if (info.author().name() != null) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.ui.xml index b36fbb26a5..1964d0b31e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.ui.xml @@ -93,6 +93,7 @@ limitations under the License. diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java index f82abee9ac..6eda547b01 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java @@ -267,6 +267,7 @@ public class ChangeInfo extends JavaScriptObject { public static class MessageInfo extends JavaScriptObject { public final native AccountInfo author() /*-{ return this.author; }-*/; public final native String message() /*-{ return this.message; }-*/; + public final native int _revisionNumber() /*-{ return this._revision_number || 0; }-*/; private final native String dateRaw() /*-{ return this.date; }-*/; public final Timestamp date() { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java index 34c0638c93..bb0c2ef60b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java @@ -42,7 +42,7 @@ public class CommentInfo extends JavaScriptObject { } private final native void setId(String id) /*-{ this.id = id; }-*/; - private final native void setPath(String path) /*-{ this.path = path; }-*/; + public final native void setPath(String path) /*-{ this.path = path; }-*/; private final void setSide(Side side) { setSideRaw(side.toString()); @@ -68,7 +68,7 @@ public class CommentInfo extends JavaScriptObject { } private final native String sideRaw() /*-{ return this.side }-*/; - public final native int line() /*-{ return this.line; }-*/; + public final native int line() /*-{ return this.line || 0; }-*/; public final native String in_reply_to() /*-{ return this.in_reply_to; }-*/; public final native String message() /*-{ return this.message; }-*/;