From 94e9a699aa8311bfeea2e149de09f96d70814c89 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 5 Jun 2015 16:00:16 +0200 Subject: [PATCH 1/3] Highlight 'Reply' button if there are draft comments on any patch set At the moment the 'Reply' button is only highlighted if there are draft comments on the currently viewed patch set. Since commit 5fd7813e the 'Reply' button always publishes draft comments on all patch sets, hence the 'Reply' button should also be highlighted when there are draft comments on patch sets that are currently not viewed. To find out whether there are draft comments on the change a change query with the 'has:draft' operator is executed. This request can be directly answered by the secondary index. This is cheaper than using ListChangesOption.DRAFT_COMMENTS when loading the change, since this option results in one SQL statement per patch set to load the draft comments from the database. In addition using ListChangesOption.DRAFT_COMMENTS would only work when the draft comment status is included into the ETag computation of the change, which it is currently not and doing it would be bad for performance. Change-Id: Ib97c09d8e7b38c0e35a068f8adebeeb409e8926c Signed-off-by: Edwin Kempin --- .../google/gerrit/client/change/ChangeScreen.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 4133c13ccf..6833b4f8b4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -96,6 +96,7 @@ import net.codemirror.lib.CodeMirror; import java.sql.Timestamp; import java.util.ArrayList; +import java.util.Collections; import java.util.EnumSet; import java.util.List; @@ -223,6 +224,18 @@ public class ChangeScreen extends Screen { super.onLoad(); CallbackGroup group = new CallbackGroup(); if (Gerrit.isSignedIn()) { + ChangeList.query("change:" + changeId.get() + " has:draft", + Collections. emptySet(), + group.add(new AsyncCallback() { + @Override + public void onSuccess(ChangeList result) { + hasDraftComments = result.length() > 0; + } + + @Override + public void onFailure(Throwable caught) { + } + })); ChangeApi.editWithFiles(changeId.get(), group.add( new AsyncCallback() { @Override @@ -957,7 +970,6 @@ public class ChangeScreen extends Screen { @Override public void onSuccess(NativeMap> result) { r.add(result); - hasDraftComments = !result.isEmpty(); } @Override From f6d1e664e18aed8ec16a9eb1e3a57679b9e6b6f9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 5 Jun 2015 16:18:11 +0200 Subject: [PATCH 2/3] Remove draft comment icons from 'Patch Sets' drop-down panel If a user adds/removes draft comments on any patch set, and there was no other modification of the change, then the change detail is not reloaded and as result the draft comment icons shown in the 'Patch Sets' drop-down panel may be outdated. The draft comment icon may be shown for patch sets that have no draft comments, and it may be missing for patch sets that have draft comments. This turns these icons useless. To fix this the change detail would need to be reloaded if there was any change to the draft comment status of a patch set. At the moment the reload doesn't happen because the ETag of the change doesn't include the draft comment status of the patch sets. Including the draft comment status into the ETag computation would be expensive since this would require one SQL statement per patch set to load the draft comments from the database. Since the 'Reply' button is now always highlighted when there are draft comments on any patch set the draft comment icons in the 'Patch Sets' drop-down panel are not needed anymore and can be removed. Highlighting the 'Reply' has also the advantage that users can click on it and actually see all draft comments across all patch sets, while the icons in the 'Patch Sets' drop-down panel only showed which patch sets have draft comments, but not the comments themselves. Removing the draft comment icons from the 'Patch Sets' drop-down panel makes the loading of the panel faster because the DRAFT_COMMENTS option is no longer needed to load the change. This option required one SQL statement per patch set. Bug: issue 3395 Change-Id: I1682f5a4eb420f3672041861a7a14df6daaebaee Signed-off-by: Edwin Kempin --- Documentation/user-review-ui.txt | 3 --- .../gerrit/client/change/ChangeConstants.java | 1 - .../gerrit/client/change/ChangeConstants.properties | 1 - .../google/gerrit/client/change/PatchSetsBox.java | 13 +------------ 4 files changed, 1 insertion(+), 17 deletions(-) diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt index dddbadc989..13aed381aa 100644 --- a/Documentation/user-review-ui.txt +++ b/Documentation/user-review-ui.txt @@ -408,9 +408,6 @@ The patch set drop-down list shows the list of patch sets and allows to switch between them. The patch sets are sorted in descending order so that the current patch set is always on top. -Patch sets that have unpublished draft comments are marked by a comment -icon. - Draft patch sets are marked with `DRAFT`. image::images/user-review-ui-change-screen-patch-set-list.png[width=800, link="images/user-review-ui-change-screen-patch-set-list.png"] diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java index 0c11a32294..204b3c2524 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java @@ -33,7 +33,6 @@ interface ChangeConstants extends Constants { String date(); String author(); String draft(); - String draftCommentsTooltip(); String notAvailable(); String relatedChanges(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties index 381a1fcecf..f58424e784 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties @@ -14,7 +14,6 @@ commit = Commit date = Date author = Author / Committer draft = (DRAFT) -draftCommentsTooltip = Draft comment(s) inside notAvailable = N/A relatedChanges = Related Changes diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/PatchSetsBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/PatchSetsBox.java index f3ab35e686..582126846d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/PatchSetsBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/PatchSetsBox.java @@ -43,7 +43,6 @@ import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwt.user.client.ui.Composite; import com.google.gwt.user.client.ui.FlexTable; import com.google.gwt.user.client.ui.HTMLPanel; -import com.google.gwt.user.client.ui.ImageResourceRenderer; import com.google.gwt.user.client.ui.PopupPanel; import com.google.gwt.user.client.ui.Widget; import com.google.gwt.user.client.ui.impl.HyperlinkImpl; @@ -122,8 +121,7 @@ class PatchSetsBox extends Composite { RestApi call = ChangeApi.detail(changeId.get()); ChangeList.addOptions(call, EnumSet.of( ListChangesOption.ALL_COMMITS, - ListChangesOption.ALL_REVISIONS, - ListChangesOption.DRAFT_COMMENTS)); + ListChangesOption.ALL_REVISIONS)); call.get(new AsyncCallback() { @Override public void onSuccess(ChangeInfo result) { @@ -187,15 +185,6 @@ class PatchSetsBox extends Composite { if (r.draft()) { sb.append(Resources.C.draft()).append(' '); } - if (r.hasDraftComments()) { - sb.openSpan() - .addStyleName(style.draft_comment()) - .setAttribute("title", Resources.C.draftCommentsTooltip()) - .append(new ImageResourceRenderer() - .render(Gerrit.RESOURCES.draftComments())) - .closeSpan() - .append(' '); - } sb.append(r.id()); sb.closeTd(); From 59b9960e9c51406dcc13975ebea8418eb9b6bdea Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 5 Jun 2015 16:40:32 +0200 Subject: [PATCH 3/3] Remove ListChangesOption.DRAFT_COMMENTS If this option is used the change details include for each patch set the information whether there are draft comments on it. The problem is that the draft comment status is not included into the ETag computation of the change and as result the change is not reloaded when the draft comment status has changed. Including the draft comment status into the ETag computation would be expensive since this requires one SQL statement per patch set to load the draft comments from the database. An easier and cheaper way to find out if there are draft comments on a change is to do a change query with the 'has:draft' operator. This query can be directly answered from the secondary index and no SQL statements need to be fired. Already now ListChangesOption.DRAFT_COMMENTS is no longer used by the Gerrit client. Also the name of the DRAFT_COMMENTS option was misleading. It didn't include the draft comments into change detail, but only for each patch set the information whether it has draft comments. Change-Id: I297fcf01256f02953aa0b5caee4f3aee4554bc59 Signed-off-by: Edwin Kempin --- Documentation/rest-api-changes.txt | 10 ---------- .../extensions/client/ListChangesOption.java | 3 --- .../gerrit/extensions/common/RevisionInfo.java | 1 - .../google/gerrit/client/changes/ChangeInfo.java | 1 - .../google/gerrit/server/change/ChangeJson.java | 14 -------------- 5 files changed, 29 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 4cd6ccfc06..7dabf9b95f 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -231,13 +231,6 @@ default. Optional fields are: `CURRENT_REVISION` or `ALL_REVISIONS` option is selected. -- -[[draft_comments]] --- -* `DRAFT_COMMENTS`: include the `has_draft_comments` field for - revisions. Only valid when the `CURRENT_REVISION` or `ALL_REVISIONS` - option is selected. --- - [[current-commit]] -- * `CURRENT_COMMIT`: parse and output all header fields from the @@ -4198,9 +4191,6 @@ link:#list-changes[Query Changes]. |=========================== |Field Name ||Description |`draft` |not set if `false`|Whether the patch set is a draft. -|`has_draft_comments` |not set if `false`|Whether the patch -set has one or more draft comments by the calling user. Only set if -link:#draft_comments[DRAFT_COMMENTS] option is requested. |`_number` ||The patch set number. |`created` || The link:rest-api.html#timestamp[timestamp] of when the patch set was diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java index 5caa9032dd..c1edd6a2e0 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java @@ -46,9 +46,6 @@ public enum ListChangesOption { /** Set the reviewed boolean for the caller. */ REVIEWED(11), - /** Include draft comments for the caller. */ - DRAFT_COMMENTS(12), - /** Include download commands for the caller. */ DOWNLOAD_COMMANDS(13), diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java index 7c71ba33f0..bc0fa6d619 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java @@ -20,7 +20,6 @@ import java.util.Map; public class RevisionInfo { public transient boolean isCurrent; public Boolean draft; - public Boolean hasDraftComments; public int _number; public Timestamp created; public AccountInfo uploader; 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 d4f44213a2..7f93500107 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 @@ -307,7 +307,6 @@ public class ChangeInfo extends JavaScriptObject { public final native int _number() /*-{ return this._number; }-*/; public final native String name() /*-{ return this.name; }-*/; public final native boolean draft() /*-{ return this.draft || false; }-*/; - public final native boolean hasDraftComments() /*-{ return this.has_draft_comments || false; }-*/; public final native boolean isEdit() /*-{ return this._number == 0; }-*/; public final native CommitInfo commit() /*-{ return this.commit; }-*/; public final native void setCommit(CommitInfo c) /*-{ this.commit = c; }-*/; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index c7a705fdd9..77e44fa423 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -27,7 +27,6 @@ import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVI import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_ACCOUNTS; import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS; import static com.google.gerrit.extensions.client.ListChangesOption.DOWNLOAD_COMMANDS; -import static com.google.gerrit.extensions.client.ListChangesOption.DRAFT_COMMENTS; import static com.google.gerrit.extensions.client.ListChangesOption.LABELS; import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES; import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED; @@ -86,7 +85,6 @@ import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.WebLinks; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.git.GitRepositoryManager; @@ -141,7 +139,6 @@ public class ChangeJson { private final WebLinks webLinks; private final EnumSet options; private final ChangeMessagesUtil cmUtil; - private final PatchLineCommentsUtil plcUtil; private final Provider checkerProvider; private final ActionJson actionJson; private final RebaseChange rebaseChange; @@ -166,7 +163,6 @@ public class ChangeJson { DynamicMap downloadCommands, WebLinks webLinks, ChangeMessagesUtil cmUtil, - PatchLineCommentsUtil plcUtil, Provider checkerProvider, ActionJson actionJson, RebaseChange rebaseChange) { @@ -185,7 +181,6 @@ public class ChangeJson { this.downloadCommands = downloadCommands; this.webLinks = webLinks; this.cmUtil = cmUtil; - this.plcUtil = plcUtil; this.checkerProvider = checkerProvider; this.actionJson = actionJson; this.rebaseChange = rebaseChange; @@ -881,15 +876,6 @@ public class ChangeJson { new RevisionResource(new ChangeResource(ctl, rebaseChange), in)); } - if (has(DRAFT_COMMENTS) - && userProvider.get().isIdentifiedUser()) { - IdentifiedUser user = (IdentifiedUser)userProvider.get(); - out.hasDraftComments = - plcUtil.draftByPatchSetAuthor(db.get(), in.getId(), - user.getAccountId(), ctl.getNotes()).iterator().hasNext() - ? true - : null; - } return out; }