From 59b2ea066d07ef923e182022e9ed590ac93ed69f Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 20 Nov 2013 01:23:20 -0800 Subject: [PATCH 1/5] 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; }-*/; From 589120fb97744f8c308279002d6cd6a2082d1192 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 21 Nov 2013 01:12:03 -0800 Subject: [PATCH 2/5] SideBySide2: Fix minor key help layout issues Move to be with 'o' in the Actions section. These keys do the same thing and they should not be in different key groups. Register Actions after Comment Editing. The sections alternate left/right when packed into the dialog so this puts Actions below Navigation for signed in users that also have Comment Editing. Application Jumping Navigation Actions Comment Editing The overall flow reads better with this ordering. Change-Id: Ib68f92753c81732e99ae31e6cf5a5f37190d4762 --- .../com/google/gerrit/client/diff/SideBySide2.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java index 4f7cd2670b..5f03b1e1cc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java @@ -132,7 +132,6 @@ public class SideBySide2 extends Screen { private KeyCommandSet keysNavigation; private KeyCommandSet keysAction; private KeyCommandSet keysComment; - private KeyCommandSet keysOpenByEnter; private List handlers; private List deferred; @@ -351,6 +350,8 @@ public class SideBySide2 extends Screen { keysNavigation.add(new NoOpKeyCommand(0, 'p', PatchUtil.C.chunkPrev2())); keysAction = new KeyCommandSet(Gerrit.C.sectionActions()); + keysAction.add(new NoOpKeyCommand(0, KeyCodes.KEY_ENTER, + PatchUtil.C.expandComment())); keysAction.add(new NoOpKeyCommand(0, 'o', PatchUtil.C.expandComment())); keysAction.add(new NoOpKeyCommand( KeyCommand.M_SHIFT, 'o', PatchUtil.C.expandAllCommentsOnCurrentLine())); @@ -367,10 +368,6 @@ public class SideBySide2 extends Screen { } }); - keysOpenByEnter = new KeyCommandSet(Gerrit.C.sectionNavigation()); - keysOpenByEnter.add(new NoOpKeyCommand(0, KeyCodes.KEY_ENTER, - PatchUtil.C.expandComment())); - if (Gerrit.isSignedIn()) { keysAction.add(new NoOpKeyCommand(0, 'c', PatchUtil.C.commentInsert())); keysComment = new KeyCommandSet(PatchUtil.C.commentEditorSet()); @@ -383,11 +380,10 @@ public class SideBySide2 extends Screen { } removeKeyHandlerRegs(); handlers.add(GlobalKey.add(this, keysNavigation)); - handlers.add(GlobalKey.add(this, keysAction)); - handlers.add(GlobalKey.add(this, keysOpenByEnter)); if (keysComment != null) { handlers.add(GlobalKey.add(this, keysComment)); } + handlers.add(GlobalKey.add(this, keysAction)); } private GerritCallback>> getCommentCallback( From a4f6c6d3258232d79b74dbe685f9d1892abc0fdc Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 21 Nov 2013 01:19:57 -0800 Subject: [PATCH 3/5] SideBySide2: Make the header background match gutter background Using two shades of grey for the header and the gutter looked horrible. The file header was much darker than the column number gutters. The issue was very noticeable if context is expanded and an unmodified line 1 is present on both sides. Set the header to the same color. Change-Id: I9b9afd32bcf565faa26c57169d7519e8e976f456 --- .../main/java/com/google/gerrit/client/diff/DiffTable.ui.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml index 98574845a1..bc80bfd7bf 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml @@ -63,7 +63,7 @@ limitations under the License. .b .intralineBg { background-color: #dfd; } .fileCommentRow { - background-color: #eee; + background-color: #f7f7f7; line-height: 1; } .fileCommentCell { From 78e400cde38c7fd23b83b45dd5407c7a71dbc7b4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 21 Nov 2013 01:33:15 -0800 Subject: [PATCH 4/5] SideBySide2: Simplify logic for showing/hiding header The file header and Gerrit menus should show/hide together. Consolidate the logic for this into one place and simplify the conditional value to be consistent between them: true means show headers, false means hide headers. Change-Id: Iae59a40369ef698279eee2c1faa2b147fbb622c8 --- .../google/gerrit/client/diff/DiffTable.java | 17 ++++++++--------- .../gerrit/client/diff/FileCommentPanel.java | 4 ++-- .../gerrit/client/diff/ScrollSynchronizer.java | 6 ++---- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java index 23af7208e4..f2237c59e4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.ChangeInfo.RevisionInfo; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.GWT; @@ -100,14 +101,12 @@ class DiffTable extends Composite { this.host = host; } - void updateFileCommentVisibility(boolean forceHide) { - UIObject.setVisible(patchSetNavRow, !forceHide); - if (forceHide || (fileCommentPanelA.getBoxCount() == 0 && - fileCommentPanelB.getBoxCount() == 0)) { - UIObject.setVisible(fileCommentRow, false); - } else { - UIObject.setVisible(fileCommentRow, true); - } + void setHeaderVisible(boolean show) { + Gerrit.setHeaderVisible(show); + UIObject.setVisible(patchSetNavRow, show); + UIObject.setVisible(fileCommentRow, show + && (fileCommentPanelA.getBoxCount() > 0 + || fileCommentPanelB.getBoxCount() > 0)); host.resizeCodeMirror(); } @@ -117,7 +116,7 @@ class DiffTable extends Composite { void createOrEditFileComment(DisplaySide side) { getPanelFromSide(side).createOrEditFileComment(); - updateFileCommentVisibility(false); + setHeaderVisible(true); } void addFileCommentBox(CommentBox box) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java index f252e65b81..37e017896d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java @@ -74,11 +74,11 @@ class FileCommentPanel extends Composite { void addFileComment(CommentBox box) { boxes.add(box); body.add(box); - table.updateFileCommentVisibility(false); + table.setHeaderVisible(true); } void onRemoveDraftBox(DraftBox box) { boxes.remove(box); - table.updateFileCommentVisibility(false); + table.setHeaderVisible(true); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java index f5ae14980b..425878c9bd 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java @@ -39,12 +39,10 @@ class ScrollSynchronizer { private void updateScreenHeader(ScrollInfo si) { if (si.getTop() == 0 && !Gerrit.isHeaderVisible()) { - Gerrit.setHeaderVisible(true); - diffTable.updateFileCommentVisibility(false); + diffTable.setHeaderVisible(true); } else if (si.getTop() > 0.5 * si.getClientHeight() && Gerrit.isHeaderVisible()) { - Gerrit.setHeaderVisible(false); - diffTable.updateFileCommentVisibility(true); + diffTable.setHeaderVisible(false); } } From a310bbc4485b8ce90856232fde3738d2bd4423e9 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 21 Nov 2013 01:41:38 -0800 Subject: [PATCH 5/5] SideBySide2: Draw a line under the file header when "fullscreen" When the Gerrit menus and patch list header have been hidden to show more of the file contents, draw a line under the tiny header (reviewed box, path, navigation cluster) to separate it from the displayed file contents. This makes it less likely the header is confused with a line of text, without getting in the way. Change-Id: Icb5f77c9e67decc3f8e3e2f5e59f7278926b7a08 --- .../main/java/com/google/gerrit/client/diff/DiffTable.java | 6 ++++++ .../java/com/google/gerrit/client/diff/DiffTable.ui.xml | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java index f2237c59e4..1e333210f2 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java @@ -37,6 +37,7 @@ class DiffTable extends Composite { private static final Binder uiBinder = GWT.create(Binder.class); interface DiffTableStyle extends CssResource { + String fullscreen(); String intralineBg(); String diff(); String activeLine(); @@ -107,6 +108,11 @@ class DiffTable extends Composite { UIObject.setVisible(fileCommentRow, show && (fileCommentPanelA.getBoxCount() > 0 || fileCommentPanelB.getBoxCount() > 0)); + if (show) { + host.header.removeStyleName(style.fullscreen()); + } else { + host.header.addStyleName(style.fullscreen()); + } host.resizeCodeMirror(); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml index bc80bfd7bf..3764cd1840 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml @@ -23,6 +23,10 @@ limitations under the License. @external .cm-keymap-fat-cursor, CodeMirror-cursor; @external .cm-searching, .cm-trailingspace, .cm-tab; + .fullscreen { + border-bottom: 1px solid #ddd; + } + .difftable { max-width: 1484px; } .difftable .CodeMirror-lines { padding: 0; } .difftable .CodeMirror pre {