Merge "Revert "Show info message on WIP changes about reviewer notifications""
diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt
index 7a0e305..31008f6 100644
--- a/Documentation/config-project-config.txt
+++ b/Documentation/config-project-config.txt
@@ -736,6 +736,15 @@
the parent project. If the property is not set in any parent project, the
default value is `FALSE`.
+[[reviewer.skipAddingAuthorAndCommitterAsReviewers]]reviewer.skipAddingAuthorAndCommitterAsReviewers::
++
+Whether to skip adding the Git commit author and committer as reviewers for
+a new change.
++
+Default is `INHERIT`, which means that this property is inherited from
+the parent project. If the property is not set in any parent project, the
+default value is `FALSE`.
+
[[file-groups]]
== The file +groups+
diff --git a/Documentation/images/user-review-ui-apply-fix.png b/Documentation/images/user-review-ui-apply-fix.png
new file mode 100644
index 0000000..d838d48
--- /dev/null
+++ b/Documentation/images/user-review-ui-apply-fix.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-metadata.png b/Documentation/images/user-review-ui-change-metadata.png
new file mode 100644
index 0000000..23abc07
--- /dev/null
+++ b/Documentation/images/user-review-ui-change-metadata.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-annotated.png b/Documentation/images/user-review-ui-change-screen-annotated.png
index 5c3f80a..4e12c96 100644
--- a/Documentation/images/user-review-ui-change-screen-annotated.png
+++ b/Documentation/images/user-review-ui-change-screen-annotated.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-change-info-labels.png b/Documentation/images/user-review-ui-change-screen-change-info-labels.png
deleted file mode 100644
index 61e2b25..0000000
--- a/Documentation/images/user-review-ui-change-screen-change-info-labels.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-comments-tab.png b/Documentation/images/user-review-ui-change-screen-comments-tab.png
new file mode 100644
index 0000000..d522f60
--- /dev/null
+++ b/Documentation/images/user-review-ui-change-screen-comments-tab.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-file-list.png b/Documentation/images/user-review-ui-change-screen-file-list.png
index 721b229..b0c2af3 100644
--- a/Documentation/images/user-review-ui-change-screen-file-list.png
+++ b/Documentation/images/user-review-ui-change-screen-file-list.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png b/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
index 9ef8f27..224de2d 100644
--- a/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
+++ b/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-reply.png b/Documentation/images/user-review-ui-change-screen-reply.png
index 1c50fc5..201db13 100644
--- a/Documentation/images/user-review-ui-change-screen-reply.png
+++ b/Documentation/images/user-review-ui-change-screen-reply.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-topleft.png b/Documentation/images/user-review-ui-change-screen-topleft.png
index a1f7813..b3bf8e7f 100644
--- a/Documentation/images/user-review-ui-change-screen-topleft.png
+++ b/Documentation/images/user-review-ui-change-screen-topleft.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen.png b/Documentation/images/user-review-ui-change-screen.png
index ff2570b..98a5d6d 100644
--- a/Documentation/images/user-review-ui-change-screen.png
+++ b/Documentation/images/user-review-ui-change-screen.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-copy-links.png b/Documentation/images/user-review-ui-copy-links.png
new file mode 100644
index 0000000..f8fa114
--- /dev/null
+++ b/Documentation/images/user-review-ui-copy-links.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png b/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
index 047034c..98cf7af 100644
--- a/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
+++ b/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-side-by-side-diff-screen.png b/Documentation/images/user-review-ui-side-by-side-diff-screen.png
index 74d02e3..ebdd177 100644
--- a/Documentation/images/user-review-ui-side-by-side-diff-screen.png
+++ b/Documentation/images/user-review-ui-side-by-side-diff-screen.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-submit-requirements.png b/Documentation/images/user-review-ui-submit-requirements.png
new file mode 100644
index 0000000..e4b88c1
--- /dev/null
+++ b/Documentation/images/user-review-ui-submit-requirements.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-suggest-fix.png b/Documentation/images/user-review-ui-suggest-fix.png
new file mode 100644
index 0000000..e08fb26
--- /dev/null
+++ b/Documentation/images/user-review-ui-suggest-fix.png
Binary files differ
diff --git a/Documentation/index.txt b/Documentation/index.txt
index c3d79b1..89b88aa 100644
--- a/Documentation/index.txt
+++ b/Documentation/index.txt
@@ -24,7 +24,7 @@
== Tutorials
. Web
-.. link:user-review-ui.html[Reviewing Changes]
+.. link:user-review-ui.html[Review UI Overview]
.. link:user-search.html[Searching Changes]
.. link:user-inline-edit.html[Manipulating Changes in Browser]
.. link:user-notify.html[Subscribing to Email Notifications]
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 94db15e..9e71df7 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -3896,17 +3896,19 @@
Map with the comment link configurations of the project. The name of
the comment link configuration is mapped to a link:#commentlink-info[
CommentlinkInfo] entity.
-|`plugin_config` |optional|
+|`plugin_config` |optional|
Plugin configuration as map which maps the plugin name to a map of
parameter names to link:#config-parameter-info[ConfigParameterInfo]
entities. Only filled for users who have read access to `refs/meta/config`.
-|`actions` |optional|
+|`actions` |optional|
Actions the caller might be able to perform on this project. The
information is a map of view names to
-|`reject_empty_commit` |optional|
+|`reject_empty_commit` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
empty commits should be rejected when a change is merged.
link:rest-api-changes.html#action-info[ActionInfo] entities.
+|`skip_adding_author_and_committer_as_reviewers` |optional|
+Whether to skip adding the Git commit author and committer as reviewers for a new change.
|=======================================================
[[config-input]]
diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt
index 6f5f729..73668d7 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -1,21 +1,15 @@
:linkattrs:
-= Review UI
+= Review UI Overview
Reviewing changes is an important task and the Gerrit Web UI provides
many functionalities to make the review process comfortable and
efficient.
-The UI has three different main views,
-
-** The dashboard, which shows all changes that are relevant to you
-** The change screen, which shows the change with all its metadata
-** The diff view, which shows changes to a single file
-
[[change-screen]]
== Change Screen
-The change screen shows the details of a single change and provides
-various actions on it.
+The change screen is the main view for a change. It shows the details of a
+single change and allows various actions on it.
image::images/user-review-ui-change-screen.png[width=800, link="images/user-review-ui-change-screen.png"]
@@ -28,44 +22,81 @@
Top left, you find the status of the change, and a permalink.
-image::images/user-review-ui-change-screen-topleft.png[width=400, link="images/user-review-ui-change-screen-topleft.png"]
+image::images/user-review-ui-change-screen-topleft.png[width=600, link="images/user-review-ui-change-screen-topleft.png"]
[[change-status]]
The change status shows the state of the change:
-- [[active]]`Active`:
+- `Active`:
+
The change is under active review.
-- [[merge-conflict]]`Merge Conflict`:
+- `Merge Conflict`:
+
-The change can't be merged due to conflicts.
+The change can't be merged into the destination branch due to conflicts.
-- [[ready-to-submit]]`Ready to Submit`:
+- `Ready to Submit`:
+
-The change has all necessary approvals and may be submitted.
+The change has all necessary approvals and fulfils all other submit
+requirements. It can be submitted.
-- [[merged]]`Merged`:
+- `Merged`:
+
The change was successfully merged into the destination branch.
-- [[abandoned]]`Abandoned`:
+- `Abandoned`:
+
-The change was abandoned.
+The change was abandoned. It is not intended to be updated, reviewed or
+submitted anymore.
+
+- `Private`:
++
+The change is marked as link:intro-user.html#private-changes[Private]. And has
+reduced visibility.
+
+- `Revert Created|Revert Submitted`:
++
+The change has a corresponding revert change. Revert changes can be created
+through UI (see <<actions, Actions section>>).
+
+- `WIP`:
++
+The change was marked as "Work in Progress". For example to indicate to
+reviewers that they shouldn't review the change yet.
[[star]]
=== Star Change
-Clicking the star icon marks the change as a favorite: it turns on
+Clicking the star icon bookmarks the change: it turns on
email notifications for this change, and the change is added to the
list under `Your` > `Starred Changes`. They can be queried by the
link:user-search.html#is[is:starred] search operator.
+[[quick-links]]
+=== Links Menu
+
+Links menu contains various change related strings for quick copying. Such as:
+Change Number, URL, Title+Url, etc. The lines in this menu can also be accessed
+via shortcuts for convenience.
+
+image::images/user-review-ui-copy-links.png[width=600, link="images/user-review-ui-copy-links.png"]
+
[[change-info]]
=== Change metadata
-The change metadata block contains detailed information about the change
-and offers actions on the change.
+The change metadata block contains detailed information about the change.
+
+image::images/user-review-ui-change-metadata.png[width=600, link="images/user-review-ui-change-metadata.png"]
+
+- [[owner]]Owner/Uploader/Author/Committer
++
+Owner is the person who created the change
++
+Uploader is the person who uploaded the latest patchset (the patchset that will
+be merged if the change is submitted)
++
+Author/Committer are concepts from Git and are retrieved from the commit when
+it's sent for review.
- [[reviewers]]Reviewers:
+
@@ -74,16 +105,36 @@
For each reviewer there is a tooltip that shows on which labels the
reviewer is allowed to vote.
+
-New reviewers can be added by clicking on the pencil icon. Typing
-into the pop-up text field activates auto completion of user and group
-names.
+New reviewers can be added through reply dialog that is opened by clicking on
+the pencil icon or on "Reply" button. Typing into the reviewer text field
+activates auto completion of user and group names.
+
+
+- [[cc-list]]CC:
++
+Accounts in CC receive notifications for the updates on the change, but don't
+need to vote/review. If the CC'ed user votes they are moved to reviewers.
++
+
+- [[attention-set]]Attention set:
++
+Users in attention set are marked by "chevron" symbol (see screenshot above).
+The mark indicates that there are actions their attention is required on the
+change: Something updated/changed since last review, their vote is required,
+etc.
++
+Changes for which you are currently in attention set can be found using
+`attention:<User>` in search and show up in a separate category of personal
+dashboard.
++
+Clicking on the mark removes the user from attention set.
+
+
[[remove-reviewer]]
-Reviewers can be removed from the change by clicking on the `x` icon
-in the reviewer's chip token. Removing a reviewer also removes the
-current votes of the reviewer. The removal of votes is recorded as a
-message on the change.
-+
+Reviewers can be removed from the change by selecting the appropriate option on
+the chip's hovercard. Removing a reviewer also removes current votes of the
+reviewer. The removal of votes is recorded in the change log.
+
Removing reviewers is protected by permissions:
** Users can always remove themselves.
@@ -92,10 +143,7 @@
Remove Reviewer] access right, the branch owner, the project owner
and Gerrit administrators may remove anyone.
-+
-image::images/user-review-ui-change-screen-info-reviewers.png[width=600, link="images/user-review-ui-change-screen-reviewers.png"]
-
-- [[project-branch-topic]]Project / Branch / Topic:
+- [[repo-branch-topic]]Project (Repo) / Branch / Topic:
+
The name of the project for which the change was done is displayed as a
link to the link:user-dashboards.html#project-default-dashboard[default
@@ -112,15 +160,55 @@
access right. To be able to set a topic on a closed change, the
`Edit Topic Name` must be assigned with the `force` flag.
+- [[parent]]Parent:
++
+Parent commit of the latest uploaded patchset. Or if the change has been merged
+the parent of the commit it was merged as into the destination branch.
+
+- [[merged-as]]Merged As:
++
+The SHA of the commit corresponding to the merged change on the destination
+branch.
+
+- [[revert-created-as]]Revert (Created|Submitted) As
++
+Points to the revert change, if one was created.
+
+- [[cherry-pick-of]]Cherry-pick of
++
+If the change was created as cherry-pick of some other change to a different
+branch, points to the original change.
+
- [[submit-strategy]]Submit Strategy:
+
The link:project-setup.html#submit_type[submit strategy] that will be
used to submit the change. The submit strategy is only displayed for
open changes.
-- [[actions]]Actions:
+- [[hastags]]Hashtags:
+
-Actions buttons are at the top, and in the overflow menu.
+Arbitrary string hashtags, that can be used to categorize changes and later use
+hashtags for search queries.
+
+[[submit-requirements]]
+=== Submit Requirements
+
+image::images/user-review-ui-submit-requirements.png[width=600, link="images/user-review-ui-copy-links.png"]
+
+Submit Requirements describe various conditions that must be fulfilled before
+the change can be submitted. Hovering over the requirement will show the
+description of the requirement, as well as additional information, such as:
+corresponding expression that is being evaluated, who can vote on the related
+labels etc.
+
+Approving votes are colored green; negative votes are colored red.
+
+For more detail on Submit Requirements see
+link:config-submit-requirements.html[Submit Requirement Configuration] page.
+
+[[actions]]
+=== Actions:
+Actions buttons are at the top right and in the overflow menu.
Depending on the change state and the permissions of the user, different
actions are available on the change:
@@ -220,13 +308,7 @@
+
image::images/user-review-ui-change-screen-change-info-actions.png[width=400, link="images/user-review-ui-change-screen-change-info-actions.png"]
-- [[labels]]Labels & Votes:
-+
-Approving votes are colored green; negative votes are colored red.
-+
-image::images/user-review-ui-change-screen-change-info-labels.png[width=400, link="images/user-review-ui-change-screen-change-info-labels.png"]
-
-[[files]]
+[[files-tab]]
=== File List
The file list shows the files that are modified in the currently viewed
@@ -251,17 +333,40 @@
The list of commits that are being integrated into the destination
branch by submitting the merge commit.
+Every file is accompanied by a number of extra information, such as status
+(modified, added, deleted, etc.), number of changed lines, type (executable,
+link, plain), comments and others. Hovering over most icons and columns reveals
+additional information.
+
+Each file can be expanded to view the contents of the file and diff. For more
+information see <<diff-view, Diff View>> section.
+
+[[comments-tab]]
+=== Comments Tab
+
+Instead of the file list, a comments tab can be selected. Comments tab presents
+comments along with related file/diff snippets. It also offers some filtering
+opportunities at the top (ex. only unresolved, only comments from user X, etc.)
+
+image::images/user-review-ui-change-screen-comments-tab.png[width=800, link="images/user-review-ui-change-screen-comments-tab.png"]
+
+[[checks-tab]]
+=== Checks Tab
+Checks tab contains results of different "Check Runs" installed by plugins. For
+more information see link:pg-plugin-checks-api.html[Checks API] page.
[[patch-sets]]
=== Patch Sets
-The change screen only presents one patch set at a time. Which patch
-set is currently viewed can be seen from the `Patch Sets` drop-down
-panel in the change header.
+The change screen only presents one pair of patch sets (`Patchset A` and
+`Patchset B`) at a time. `A` is always an earlier upload than `B` and serves as
+a base for diffing when viewing changes in the files. Which patch
+sets is currently viewed can be seen from the `Patch Sets` drop-down
+panel in the change header. If patchset 'A' is not selected a parent commit of
+patchset 'B' is used by default.
image::images/user-review-ui-change-screen-patch-sets.png[width=300, link="images/user-review-ui-change-screen-patch-sets.png"]
-
[[download]]
=== Download
@@ -278,7 +383,8 @@
Each command has a copy-to-clipboard icon that allows the command to be
copied into the clipboard. This makes it easy to paste and execute the
-command on a Git command line.
+command on a Git command line. Additionally each line can copied to clipboard
+using number (1..9) of the appropriate line as a keyboard shortcut.
If several download schemes are configured on the server (e.g. SSH and
HTTP) there is a drop-down list to switch between the download schemes.
@@ -306,22 +412,20 @@
image::images/user-review-ui-change-screen-included-in.png[width=800, link="images/user-review-ui-change-screen-included-in.png"]
-
-
[[related-changes]]
=== Related Changes
If there are changes that are related to the currently viewed change
they are displayed in the third column of the change screen.
-There are several lists of related changes and a tab control is used to
-display each list of related changes in its own tab.
+There are several lists of related changes that are displayed in separate
+sectionsunder each other.
-The following tabs may be displayed:
+The following sections may be displayed:
-- [[related-changes-tab]]`Related Changes`:
+- [[related-changes-section]]`Related Changes`:
+
-This tab page shows changes on which the current change depends
+This section shows changes on which the current change depends
(ancestors) and open changes that depend on the current change
(descendants). For merge commits it also shows the closed changes that
will be merged into the destination branch by submitting the merge
@@ -341,10 +445,10 @@
+
** [[not-current]]Not current:
+
-The selected patch set of the change is outdated; it is not the current
-patch set of the change.
+The patch set of the related change which is related to the current change is
+outdated; it is not the current patch set of the change.
+
-It means that the
+For ancestor it means that the
currently viewed patch set depends on a outdated patch set of the
ancestor change. This is because a new patch set for the ancestor
change was uploaded in the meantime and as result the currently viewed
@@ -364,20 +468,24 @@
note that following the link to an indirect descendant change may
result in a completely different related changes listing.
-** [[closed-ancestor]]Closed ancestor:
+** [[merged-related-change]]Merged
+
-Indicates a closed ancestor, e.g. the commit was directly pushed into
-the repository bypassing code review, or the ancestor change was
-reviewed and submitted on another branch. The latter may indicate that
-the user has accidentally pushed the commit to the wrong branch, e.g.
-the commit was done on `branch-a`, but was then pushed to
-`refs/for/branch-b`.
+The change has been merged.
++
+If the relationship to submitted change falls under conditions described in
+<<not-current, Not current>> the status is orange. Such changes can appear as
+both ancestors and descendants of the change.
+
+** [[submittable-related-change]]Submittable
++
+All the submit requirements are fulfilled for the related change and it can be
+submitted when all of its ancestors are submitted.
** [[closed-ancestor-abandoned]]Abandoned:
+
Indicates an abandoned change.
-- [[conflicts-with]]`Conflicts With`:
+- [[conflicts-with]]`Merge Conflicts`:
+
This section shows changes that conflict with the current change.
Non-mergeable changes are filtered out; only conflicting changes that
@@ -393,10 +501,9 @@
currently viewed change, when clicking the submit button. It includes
ancestors of the current patch set.
+
-This may include changes and its ancestors with the same topic if
-`change.submitWholeTopic` is enabled. Only open changes with the
-same topic are included in the list.
-+
+If `change.submitWholeTopic` is enabled this section also includes changes with
+the same topic. The list recursively includes all changes that can be reached by
+ancestor and topic relationships. Only open changes are included in the result.
- [[cherry-picks]]`Cherry-Picks`:
+
@@ -411,12 +518,18 @@
If there are no related changes for a tab, the tab is not displayed.
+- [[same-topic]]`Same Topic`:
++
+This section shows changes which are part of the same topic. If
+`change.submitWholeTopic` is enabled, then this section is omitted and changes
+are included as part of <<submitted-together, `Submitted Together`>>
+
[[reply]]
=== Reply
The `Reply...` button in the change header allows to reply to the
currently viewed patch set; one can add a summary comment, publish
-inline draft comments, and vote on the labels.
+inline draft comments, vote on the labels and adjust attention set.
image::images/user-review-ui-change-screen-reply.png[width=800, link="images/user-review-ui-change-screen-reply.png"]
@@ -424,10 +537,8 @@
[[summary-comment]]
A text box allows to type a summary comment for the currently viewed
-patch set. Some basic markdown-like syntax is supported which renders
-indented lines preformatted, lines starting with "- " or "* " as list
-items, and lines starting with "> " as block quotes (also see replying to
-link:#reply-to-message[messages] and link:#reply-inline-comment[inline comments]).
+patch set. Markdown syntax is supported same as in other
+<<comments-markdown, Comments>>.
[[vote]]
If the current patch set is viewed, buttons are displayed for
@@ -439,7 +550,7 @@
are links to navigate to the inline comments which can be used if a
comment needs to be edited.
-The `Post` button publishes the comments and the votes.
+The `SEND` button publishes the comments and the votes.
[[quick-approve]]
If a user can approve a label that is still required, a quick approve
@@ -460,12 +571,12 @@
image::images/user-review-ui-change-screen-quick-approve.png[width=800, link="images/user-review-ui-change-screen-quick-approve.png"]
-[[history]]
-=== History
+[[change-log]]
+=== Change Log
The history of the change can be seen in the lower part of the screen.
-The history contains messages for all kinds of change updates, e.g. a
+The log contains messages for all kinds of change updates, e.g. a
message is added when a new patch set is uploaded or when a review was
done.
@@ -491,12 +602,12 @@
image::images/user-review-ui-change-screen-plugin-extensions.png[width=300, link="images/user-review-ui-change-screen-plugin-extensions.png"]
-[[side-by-side]]
+[[diff-view]]
== Side-by-Side Diff Screen
-The side-by-side diff screen shows a single patch; the old file version
-is displayed on the left side of the screen; the new file version is
-displayed on the right side of the screen.
+The side-by-side diff screen shows a single patch (or difference between two
+patchsets); the old file version is displayed on the left side of the screen;
+the new file version is displayed on the right side of the screen.
This screen allows to review a patch and to comment on it.
@@ -557,6 +668,10 @@
Code blocks with comments may overlap. This means it is possible to
attach several comments to the same code.
+[[comments-markdown]]
+The comments support markdown. It follows the CommonMark spec, except inline
+images and direct HTML are not rendered and kept as plaintext.
+
[[line-links]]
The lines of the patch file are linkable: simply append
'#<linenumber>' to the URL, or click on the line-number. This not only
@@ -565,15 +680,14 @@
[[reply-inline-comment]]
Clicking on the `Reply` button opens an editor to type the reply.
-Quoting is supported, but only by manually copying & pasting the old
-comment that should be quoted and prefixing every line by "> ". Please
-note that for a correct rendering it is important to leave a blank line
-between a quoted block and the reply to it.
+Previous comment can be quoted using "Quote" button. A new draft would be open
+on the same comment thread with the text of the previoused comment quoted using
+markdown syntax.
image::images/user-review-ui-side-by-side-diff-screen-inline-comments.png[width=800, link="images/user-review-ui-side-by-side-diff-screen-inline-comments.png"]
Comments are first saved as drafts, and you can revisit the drafts as
-you read through code review. Finally, they should be published by
+you read through code review. Finally, they will be published by
clicking the "Reply".
[[done]]
@@ -610,6 +724,21 @@
make it visible to other users it must be published from the change
screen by link:#reply[replying] to the change.
+[[suggest-fix]]
+=== Suggest fix (WIP)
+Comments can contain suggested fixes.
+
+Clicking "Suggest Fix" will insert a special code-block in the text of the
+comment. The contents of this code block will replace the lines the comment is
+attached to (what gets highlighted when hovering over comment).
+
+image::images/user-review-ui-suggest-fix.png[width=400, link="images/user-review-ui-suggest-fix.png"]
+
+The author of the change can then preview and apply the change. This will created
+a new patchset with changes applied.
+
+image::images/user-review-ui-apply-fix.png[width=800, link="images/user-review-ui-apply-fix.png"]
+
[[file-level-comments]]
=== File Level Comments
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 3e48eec..5b4a9e5 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1080,6 +1080,19 @@
}
}
+ protected void setSkipAddingAuthorAndCommitterAsReviewers(InheritableBoolean value)
+ throws Exception {
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig config = projectConfigFactory.read(md);
+ config.updateProject(
+ p ->
+ p.setBooleanConfig(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS, value));
+ config.commit(md);
+ projectCache.evictAndReindex(config.getProject());
+ }
+ }
+
protected void blockAnonymousRead() throws Exception {
String allRefs = RefNames.REFS + "*";
projectOperations
diff --git a/java/com/google/gerrit/common/RawInputUtil.java b/java/com/google/gerrit/common/RawInputUtil.java
index 4a676e6..23e4a23 100644
--- a/java/com/google/gerrit/common/RawInputUtil.java
+++ b/java/com/google/gerrit/common/RawInputUtil.java
@@ -14,7 +14,6 @@
package com.google.gerrit.common;
-import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
@@ -31,7 +30,6 @@
public static RawInput create(byte[] bytes, String contentType) {
requireNonNull(bytes);
- checkArgument(bytes.length > 0);
return new RawInput() {
@Override
public InputStream getInputStream() throws IOException {
diff --git a/java/com/google/gerrit/entities/BooleanProjectConfig.java b/java/com/google/gerrit/entities/BooleanProjectConfig.java
index 5201f6d..605c40c 100644
--- a/java/com/google/gerrit/entities/BooleanProjectConfig.java
+++ b/java/com/google/gerrit/entities/BooleanProjectConfig.java
@@ -41,7 +41,9 @@
ENABLE_REVIEWER_BY_EMAIL("reviewer", "enableByEmail"),
MATCH_AUTHOR_TO_COMMITTER_DATE("submit", "matchAuthorToCommitterDate"),
REJECT_EMPTY_COMMIT("submit", "rejectEmptyCommit"),
- WORK_IN_PROGRESS_BY_DEFAULT("change", "workInProgressByDefault");
+ WORK_IN_PROGRESS_BY_DEFAULT("change", "workInProgressByDefault"),
+ SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS(
+ "reviewer", "skipAddingAuthorAndCommitterAsReviewers");
// Git config
private final String section;
diff --git a/java/com/google/gerrit/entities/LabelFunction.java b/java/com/google/gerrit/entities/LabelFunction.java
index f361741..d49ab0f 100644
--- a/java/com/google/gerrit/entities/LabelFunction.java
+++ b/java/com/google/gerrit/entities/LabelFunction.java
@@ -14,6 +14,7 @@
package com.google.gerrit.entities;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.SubmitRecord.Label;
import java.util.Collections;
@@ -48,6 +49,16 @@
ALL = Collections.unmodifiableMap(all);
}
+ public static final Map<String, LabelFunction> ALL_NON_DEPRECATED;
+
+ static {
+ Map<String, LabelFunction> allNonDeprecated = new LinkedHashMap<>();
+ for (LabelFunction f : ImmutableSet.of(NO_BLOCK, NO_OP, PATCH_SET_LOCK)) {
+ allNonDeprecated.put(f.getFunctionName(), f);
+ }
+ ALL_NON_DEPRECATED = Collections.unmodifiableMap(allNonDeprecated);
+ }
+
public static Optional<LabelFunction> parse(@Nullable String str) {
return Optional.ofNullable(ALL.get(str));
}
diff --git a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
index c24227d..fbb2fd7 100644
--- a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
+++ b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
@@ -197,8 +197,6 @@
@AutoValue.Builder
public abstract static class Builder {
- public abstract Builder childPredicateResults(ImmutableList<PredicateResult> value);
-
protected abstract ImmutableList.Builder<PredicateResult> childPredicateResultsBuilder();
public abstract Builder predicateString(String value);
diff --git a/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java b/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
index 3ba1277..1a51c15 100644
--- a/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
+++ b/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
@@ -40,6 +40,7 @@
public InheritedBooleanInfo enableReviewerByEmail;
public InheritedBooleanInfo matchAuthorToCommitterDate;
public InheritedBooleanInfo rejectEmptyCommit;
+ public InheritedBooleanInfo skipAddingAuthorAndCommitterAsReviewers;
public MaxObjectSizeLimitInfo maxObjectSizeLimit;
@Deprecated // Equivalent to defaultSubmitType.value
diff --git a/java/com/google/gerrit/extensions/api/projects/ConfigInput.java b/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
index 8005fc5..906fc4c 100644
--- a/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
+++ b/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
@@ -34,6 +34,7 @@
public InheritableBoolean enableReviewerByEmail;
public InheritableBoolean matchAuthorToCommitterDate;
public InheritableBoolean rejectEmptyCommit;
+ public InheritableBoolean skipAddingAuthorAndCommitterAsReviewers;
public String maxObjectSizeLimit;
public SubmitType submitType;
public ProjectState state;
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index d575324..2883ef8 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -32,6 +32,7 @@
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
@@ -653,6 +654,9 @@
}
private ImmutableList<InternalReviewerInput> getReviewerInputs() {
+ if (projectState.is(BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS)) {
+ return reviewerInputs;
+ }
return Streams.concat(
reviewerInputs.stream(),
Streams.stream(
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index 79e2054..fb6b177 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -43,6 +43,16 @@
* what labels are defined for the project. The label definition can change between the time a vote
* is originally made and a later point, for example when a change is submitted. This class
* normalizes old votes against current project configuration.
+ *
+ * <p>Normalizing a vote means making it compliant with the current label definition:
+ *
+ * <ul>
+ * <li>If the voting value is greater than the max allowed value according to the label
+ * definition, the voting value is changed to the max allowed value.
+ * <li>If the voting value is lower than the min allowed value according to the label definition,
+ * the voting value is changed to the min allowed value.
+ * <li>If the label definition for a vote is missing, the vote is deleted.
+ * </ul>
*/
@Singleton
public class LabelNormalizer {
diff --git a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
index ae9828a..9ccbf90 100644
--- a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
+++ b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
@@ -71,6 +71,11 @@
.put(
BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT,
new Mapper(i -> i.workInProgressByDefault, (i, v) -> i.workInProgressByDefault = v))
+ .put(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS,
+ new Mapper(
+ i -> i.skipAddingAuthorAndCommitterAsReviewers,
+ (i, v) -> i.skipAddingAuthorAndCommitterAsReviewers = v))
.build();
static {
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index a964ee1..fc1256e 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -1143,9 +1143,11 @@
error(
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet())));
+ KEY_FUNCTION,
+ name,
+ Joiner.on(", ").join(LabelFunction.ALL_NON_DEPRECATED.keySet())));
}
- label.setFunction(function.orElse(null));
+ function.ifPresent(label::setFunction);
label.setCopyCondition(rc.getString(LABEL, name, KEY_COPY_CONDITION));
if (!values.isEmpty()) {
diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java
index e86ad41..07f7ba5 100644
--- a/java/com/google/gerrit/server/project/RefUtil.java
+++ b/java/com/google/gerrit/server/project/RefUtil.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import java.io.IOException;
import java.util.Collections;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RevisionSyntaxException;
@@ -49,6 +50,9 @@
} catch (RevisionSyntaxException e) {
throw new UnprocessableEntityException(
String.format("base revision \"%s\" is invalid", baseRevision), e);
+ } catch (AmbiguousObjectException e) {
+ throw new UnprocessableEntityException(
+ String.format("base revision \"%s\" is ambiguous", baseRevision), e);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 52207db..5fd2159 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -134,6 +134,27 @@
}
@Test
+ public void rejectCreatingLabelWithInvalidFunction() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[label \"Foo\"]\n function = INVALID");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: Invalid function for label \"foo\"."
+ + " Valid names are: NoBlock, NoOp, PatchSetLock",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
public void rejectSettingCopyMinScore() throws Exception {
testRejectSettingLabelFlag(
LabelConfigValidator.KEY_COPY_MIN_SCORE, /* value= */ true, "is:MIN");
diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
index edfb577..cc72924 100644
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -108,6 +108,7 @@
"Uploading to an edit worked!".getBytes(UTF_8);
private static final String CONTENT_BINARY_ENCODED_NEW3 =
"data:text/plain,VXBsb2FkaW5nIHRvIGFuIGVkaXQgd29ya2VkIQ==";
+ private static final String CONTENT_BINARY_ENCODED_EMPTY = "data:text/plain;base64,";
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@@ -697,6 +698,16 @@
}
@Test
+ public void changeEditModifyFileSetEmptyContentModeRest() throws Exception {
+ createEmptyEditFor(changeId);
+ FileContentInput in = new FileContentInput();
+ in.binary_content = CONTENT_BINARY_ENCODED_EMPTY;
+ in.fileMode = FILE_MODE;
+ adminRestSession.put(urlEditFile(changeId, FILE_NAME), in).assertNoContent();
+ ensureSameBytes(getFileContentOfEdit(changeId, FILE_NAME), "".getBytes(UTF_8));
+ }
+
+ @Test
public void createAndUploadBinaryInChangeEditOneRequestRest() throws Exception {
FileContentInput in = new FileContentInput();
in.binary_content = CONTENT_BINARY_ENCODED_NEW;
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 08f65da..50bb8ae 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -1169,6 +1169,27 @@
}
@Test
+ public void pushForMasterWithForgedAuthorAndCommitter_skipAddingAuthorAndCommitterAsReviewers()
+ throws Exception {
+ setSkipAddingAuthorAndCommitterAsReviewers(InheritableBoolean.TRUE);
+ TestAccount user2 = accountCreator.user2();
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ commitBuilder()
+ .author(user.newIdent())
+ .committer(user2.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+ // Push commit as "Administrator".
+ pushHead(testRepo, "refs/for/master");
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
public void pushForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
// Create a commit with different forged author and committer.
RevCommit c =
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 67c784b..079f84e 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -57,8 +57,10 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
+import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
+import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
@@ -494,6 +496,20 @@
}
@Test
+ public void createAuthorNotAddedAsCcWithAvoidAddingOriginalAuthorAsReviewer() throws Exception {
+ ConfigInput config = new ConfigInput();
+ config.skipAddingAuthorAndCommitterAsReviewers = InheritableBoolean.TRUE;
+ gApi.projects().name(project.get()).config(config);
+ ChangeInput input = newChangeInput(ChangeStatus.NEW);
+ input.author = new AccountInput();
+ input.author.email = user.email();
+ input.author.name = user.fullName();
+
+ ChangeInfo info = assertCreateSucceeds(input);
+ assertThat(info.reviewers).isEmpty();
+ }
+
+ @Test
public void createNewWorkInProgressChange() throws Exception {
ChangeInput input = newChangeInput(ChangeStatus.NEW);
input.workInProgress = true;
diff --git a/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java b/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
index fb3259f..f2184de 100644
--- a/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
@@ -213,7 +213,7 @@
@Test
@GerritConfig(name = "event.comment-added.publishPatchSetLevelComment", value = "false")
- public void publishPatchSetLevelComment() throws Exception {
+ public void publishPatchSetLevelComment_disabled() throws Exception {
PushOneCommit.Result r = createChange();
TestListener listener = new TestListener();
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
@@ -225,6 +225,20 @@
}
@Test
+ @GerritConfig(name = "event.comment-added.publishPatchSetLevelComment", value = "true")
+ public void publishPatchSetLevelComment_enabled() throws Exception {
+ PushOneCommit.Result r = createChange();
+ TestListener listener = new TestListener();
+ try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
+ String patchSetLevelComment = "a patch set level comment";
+ ReviewInput reviewInput = new ReviewInput().patchSetLevelComment(patchSetLevelComment);
+ revision(r).review(reviewInput);
+ assertThat(listener.getLastCommentAddedEvent().getComment())
+ .isEqualTo(String.format("Patch Set 1:\n\n%s", patchSetLevelComment));
+ }
+ }
+
+ @Test
public void reviewChange_MultipleVotes() throws Exception {
TestListener listener = new TestListener();
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
index 29fd5ed..1f725f8 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
@@ -40,6 +40,9 @@
.setBooleanConfig(
BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
InheritableBoolean.INHERIT)
+ .setBooleanConfig(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS,
+ InheritableBoolean.TRUE)
.build();
@Test
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 329102a..adc3bd7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1570,13 +1570,10 @@
handleRebaseConfirm(e: CustomEvent<ConfirmRebaseEventDetail>) {
assertIsDefined(this.confirmRebase, 'confirmRebase');
assertIsDefined(this.actionsModal, 'actionsModal');
- const el = this.confirmRebase;
const payload = {
base: e.detail.base,
allow_conflicts: e.detail.allowConflicts,
};
- this.actionsModal.close();
- el.hidden = true;
this.fireAction(
'/rebase',
assertUIActionInfo(this.revisionActions.rebase),
@@ -1841,6 +1838,7 @@
if (!response) {
return;
}
+ // response is guaranteed to be ok (due to semantics of rest-api methods)
return this.restApiService.getResponseObject(response).then(obj => {
switch (action.__key) {
case ChangeActions.REVERT: {
@@ -1874,6 +1872,9 @@
case ChangeActions.REBASE_EDIT:
case ChangeActions.REBASE:
case ChangeActions.SUBMIT:
+ // Hide rebase dialog only if the action succeeds
+ this.actionsModal?.close();
+ this.hideAllDialogs();
fireReload(this, true);
break;
case ChangeActions.REVERT_SUBMISSION: {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 1d10128..a1021a0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -1422,6 +1422,39 @@
await element.reload();
});
+ test('revert change payload', async () => {
+ await element.updateComplete;
+ queryAndAssert<GrButton>(
+ element,
+ 'gr-button[data-action-key="revert"]'
+ ).click();
+ const revertAction = {
+ __key: 'revert',
+ __type: 'change',
+ __primary: false,
+ method: HttpMethod.POST,
+ label: 'Revert',
+ title: 'Revert the change',
+ enabled: true,
+ };
+ queryAndAssert(element, 'gr-confirm-revert-dialog').dispatchEvent(
+ new CustomEvent('confirm', {
+ detail: {
+ message: 'foo message',
+ revertType: 1,
+ },
+ })
+ );
+ assert.deepEqual(fireActionStub.lastCall.args, [
+ '/revert',
+ assertUIActionInfo(revertAction),
+ false,
+ {
+ message: 'foo message',
+ },
+ ]);
+ });
+
test('revert change with plugin hook', async () => {
const newRevertMsg = 'Modified revert msg';
sinon
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 6a74dce..f32bb8d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -279,7 +279,7 @@
${this.renderNonOwner(ChangeRole.AUTHOR)}
${this.renderNonOwner(ChangeRole.COMMITTER)} ${this.renderReviewers()}
${this.renderCCs()} ${this.renderProjectBranch()} ${this.renderParent()}
- ${this.renderMergedAs()} ${this.renderShowReverCreatedAs()}
+ ${this.renderMergedAs()} ${this.renderShowRevertCreatedAs()}
${this.renderTopic()} ${this.renderCherryPickOf()}
${this.renderStrategy()} ${this.renderHashTags()}
${this.renderSubmitRequirements()} ${this.renderWeblinks()}
@@ -561,7 +561,7 @@
</section>`;
}
- private renderShowReverCreatedAs() {
+ private renderShowRevertCreatedAs() {
if (!this.showRevertCreatedAs()) return nothing;
return html`<section
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
index 69fd142..a2beee2 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
@@ -99,7 +99,7 @@
override render() {
const change = this.change;
if (!change) throw new Error('Missing change');
- const linkClass = this._computeLinkClass(change);
+ const linkClass = this.computeLinkClass(change);
return html`
<div class="changeContainer">
<a
@@ -118,16 +118,16 @@
>✓</span
>`
: ''}
- ${this.showChangeStatus && !isChangeInfo(change)
- ? html`<span class=${this._computeChangeStatusClass(change)}>
- (${this._computeChangeStatus(change)})
+ ${this.showChangeStatus
+ ? html`<span class=${this.computeChangeStatusClass(change)}>
+ (${this.computeChangeStatus(change)})
</span>`
: ''}
</div>
`;
}
- _computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
+ private computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
const statuses = [];
if (change.status === ChangeStatus.ABANDONED) {
statuses.push('strikethrough');
@@ -138,11 +138,16 @@
return statuses.join(' ');
}
- _computeChangeStatusClass(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatusClass(
+ change: RelatedChangeAndCommitInfo | ChangeInfo
+ ) {
const classes = ['status'];
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
classes.push('notCurrent');
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
classes.push('indirectAncestor');
} else if (change.submittable) {
classes.push('submittable');
@@ -152,16 +157,19 @@
return classes.join(' ');
}
- _computeChangeStatus(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatus(change: RelatedChangeAndCommitInfo | ChangeInfo) {
switch (change.status) {
case ChangeStatus.MERGED:
return 'Merged';
case ChangeStatus.ABANDONED:
return 'Abandoned';
}
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
return 'Not current';
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
return 'Indirect ancestor';
} else if (change.submittable) {
return 'Submittable';
@@ -169,7 +177,7 @@
return '';
}
- _isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
+ private isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
return (
this.connectedRevisions &&
!this.connectedRevisions.includes(change.commit.commit)
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index bdb95ea..cac9ae5 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -418,6 +418,7 @@
)}<gr-related-change
.change=${change}
.href=${createChangeUrl({change, usp: 'cherry-pick'})}
+ show-change-status
>${change.branch}: ${change.subject}</gr-related-change
>
</div>`
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 676a468..3e90145 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -249,7 +249,7 @@
<gr-related-collapse title="Cherry picks">
<div class="relatedChangeLine show-when-collapsed">
<span class="marker space"> </span>
- <gr-related-change>
+ <gr-related-change show-change-status="">
test-branch: Test subject
</gr-related-change>
</div>
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index f7e3542..3fdb1c0 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -323,10 +323,18 @@
override updated(changedProperties: PropertyValues) {
if (changedProperties.has('result')) {
- this.isExpandable = !!this.result?.summary && !!this.result?.message;
+ this.isExpandable = this.computeIsExpandable();
}
}
+ private computeIsExpandable() {
+ const hasSummary = !!this.result?.summary;
+ const hasMessage = !!this.result?.message;
+ const hasLinks = (this.result?.links ?? []).length > 0;
+ const hasPointers = (this.result?.codePointers ?? []).length > 0;
+ return hasSummary && (hasMessage || hasLinks || hasPointers);
+ }
+
override focus() {
if (this.nameEl) this.nameEl.focus();
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts b/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
index 934e958..113470c 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
@@ -117,7 +117,6 @@
aria-checked="false"
aria-label="Expand result row"
class="show-hide"
- hidden=""
role="switch"
tabindex="0"
>
@@ -262,6 +261,7 @@
</h3>
<gr-result-row
class="FAKEErrorFinderFinderFinderFinderFinderFinderFinder"
+ isexpandable
>
</gr-result-row>
<gr-result-row
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
index b589915..6cdd808 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
@@ -880,7 +880,13 @@
private renderBrowserNotifications() {
if (!this.flagsService.isEnabled(KnownExperimentId.PUSH_NOTIFICATIONS))
return nothing;
- if (!areNotificationsEnabled(this.account)) return nothing;
+ if (
+ !this.flagsService.isEnabled(
+ KnownExperimentId.PUSH_NOTIFICATIONS_DEVELOPER
+ ) &&
+ !areNotificationsEnabled(this.account)
+ )
+ return nothing;
return html`
<section id="allowBrowserNotificationsSection">
<label class="title" for="allowBrowserNotifications"
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
index 4005bb3..140b752 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
@@ -165,12 +165,7 @@
override render() {
return html`
- <div
- class="dropdown-content"
- slot="dropdown-content"
- id="suggestions"
- role="listbox"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
${repeat(
this.suggestions,
@@ -209,7 +204,7 @@
}
setPositionTarget(target: HTMLElement) {
- this.fitController?.setPositionTarget(target);
+ this.fitController.setPositionTarget(target);
}
private handleUp() {
@@ -300,7 +295,7 @@
} else {
this.cursor.stops = [];
}
- this.fitController?.refit();
+ this.fitController.refit();
}
private setIndex() {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
index 641dd2d..cb83396 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
@@ -42,12 +42,7 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div
- class="dropdown-content"
- id="suggestions"
- role="listbox"
- slot="dropdown-content"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
<li
aria-label="test name 1"
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 6cd3cb0..804d101 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -7,7 +7,12 @@
import '../../../elements/shared/gr-hovercard/gr-hovercard';
import './gr-diff-builder-side-by-side';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
-import {DiffBuilder, DiffContextExpandedEventDetail} from './gr-diff-builder';
+import {
+ DiffBuilder,
+ ImageDiffBuilder,
+ DiffContextExpandedEventDetail,
+ isImageDiffBuilder,
+} from './gr-diff-builder';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
@@ -114,7 +119,7 @@
layers: DiffLayer[] = [];
// visible for testing
- builder?: DiffBuilder;
+ builder?: DiffBuilder | ImageDiffBuilder;
/**
* All layers, both from the outside and the default ones. See `layers` for
@@ -206,8 +211,8 @@
return (
this.cancelableRenderPromise
.then(async () => {
- if (this.isImageDiff) {
- (this.builder as GrDiffBuilderImage).renderDiff();
+ if (isImageDiffBuilder(this.builder)) {
+ this.builder.renderImageDiff();
}
await this.untilGroupsRendered();
this.fireDiffEvent('render-content');
@@ -518,7 +523,7 @@
// If endIndex isn't present, continue to the end of the line.
const endIndex =
highlight.endIndex === undefined
- ? line.text.length
+ ? GrAnnotation.getStringLength(line.text)
: highlight.endIndex;
GrAnnotation.annotateElement(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 5eabc59..0f02d71 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -271,10 +271,11 @@
const str0 = slice(str, 0, 6);
const str1 = slice(str, 6);
+ const numHighlightedChars = GrAnnotation.getStringLength(str1);
layer.annotate(el, lineNumberEl, l, Side.LEFT);
- assert.isTrue(annotateElementSpy.called);
+ assert.isTrue(annotateElementSpy.calledWith(el, 6, numHighlightedChars));
assert.equal(el.childNodes.length, 2);
assert.instanceOf(el.childNodes[0], Text);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
index 75ee088..096d32e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
@@ -3,228 +3,265 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {GrEndpointParam} from '../../../elements/plugins/gr-endpoint-param/gr-endpoint-param';
-import {RenderPreferences} from '../../../api/diff';
+import {RenderPreferences, Side} from '../../../api/diff';
import '../gr-diff-image-viewer/gr-image-viewer';
-import {GrImageViewer} from '../gr-diff-image-viewer/gr-image-viewer';
-import {createElementDiff} from '../gr-diff/gr-diff-utils';
+import {ImageDiffBuilder} from './gr-diff-builder';
+import {html, LitElement, nothing, render} from 'lit';
+import {customElement, property, query, state} from 'lit/decorators.js';
// MIME types for images we allow showing. Do not include SVG, it can contain
// arbitrary JavaScript.
const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|x-icon|jpeg|jpg|png|tiff|webp)$/;
-export class GrDiffBuilderImage extends GrDiffBuilderSideBySide {
+export class GrDiffBuilderImage
+ extends GrDiffBuilderSideBySide
+ implements ImageDiffBuilder
+{
constructor(
diff: DiffInfo,
prefs: DiffPreferencesInfo,
outputEl: HTMLElement,
- private readonly _baseImage: ImageInfo | null,
- private readonly _revisionImage: ImageInfo | null,
+ private readonly baseImage: ImageInfo | null,
+ private readonly revisionImage: ImageInfo | null,
renderPrefs?: RenderPreferences,
- private readonly _useNewImageDiffUi: boolean = false
+ private readonly useNewImageDiffUi: boolean = false
) {
super(diff, prefs, outputEl, [], renderPrefs);
}
- public renderDiff() {
- const section = createElementDiff('tbody', 'image-diff');
-
- if (this._useNewImageDiffUi) {
- this._emitImageViewer(section);
-
- this.outputEl.appendChild(section);
- } else {
- this._emitImagePair(section);
- this._emitImageLabels(section);
-
- this.outputEl.appendChild(section);
- this.outputEl.appendChild(this._createEndpoint());
- }
- }
-
- private _createEndpoint() {
- const tbody = createElementDiff('tbody');
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
-
- // TODO(kaspern): Support blame for image diffs and remove the hardcoded 4
- // column limit.
- td.setAttribute('colspan', '4');
- const endpointDomApi = createElementDiff('gr-endpoint-decorator');
- endpointDomApi.setAttribute('name', 'image-diff');
- endpointDomApi.appendChild(
- this._createEndpointParam('baseImage', this._baseImage)
+ public renderImageDiff() {
+ render(
+ html`
+ ${this.useNewImageDiffUi
+ ? html`
+ <gr-diff-image-new
+ .automaticBlink=${this.autoBlink()}
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-new>
+ `
+ : html`
+ <gr-diff-image-old
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-old>
+ `}
+ `,
+ this.outputEl
);
- endpointDomApi.appendChild(
- this._createEndpointParam('revisionImage', this._revisionImage)
- );
- td.appendChild(endpointDomApi);
- tr.appendChild(td);
- tbody.appendChild(tr);
- return tbody;
}
- private _createEndpointParam(name: string, value: ImageInfo | null) {
- const endpointParam = createElementDiff(
- 'gr-endpoint-param'
- ) as GrEndpointParam;
- endpointParam.name = name;
- endpointParam.value = value;
- return endpointParam;
- }
-
- private _emitImageViewer(section: HTMLElement) {
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
- // TODO(hermannloose): Support blame for image diffs, see above.
- td.setAttribute('colspan', '4');
- const imageViewer = createElementDiff('gr-image-viewer') as GrImageViewer;
-
- imageViewer.baseUrl = this._getImageSrc(this._baseImage);
- imageViewer.revisionUrl = this._getImageSrc(this._revisionImage);
- imageViewer.automaticBlink =
- !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
-
- td.appendChild(imageViewer);
- tr.appendChild(td);
- section.appendChild(tr);
- }
-
- private _getImageSrc(image: ImageInfo | null): string {
- return image && IMAGE_MIME_PATTERN.test(image.type)
- ? `data:${image.type};base64,${image.body}`
- : '';
- }
-
- private _emitImagePair(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- tr.appendChild(this._createImageCell(this._baseImage, 'left', section));
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- tr.appendChild(
- this._createImageCell(this._revisionImage, 'right', section)
- );
-
- section.appendChild(tr);
- }
-
- private _createImageCell(
- image: ImageInfo | null,
- className: string,
- section: HTMLElement
- ) {
- const td = createElementDiff('td', className);
- const src = this._getImageSrc(image);
- if (image && src) {
- const imageEl = createElementDiff('img') as HTMLImageElement;
- imageEl.onload = () => {
- image._height = imageEl.naturalHeight;
- image._width = imageEl.naturalWidth;
- this._updateImageLabel(section, className, image);
- };
- imageEl.addEventListener('error', (e: Event) => {
- imageEl.remove();
- td.textContent = '[Image failed to load] ' + e.type;
- });
- imageEl.setAttribute('src', src);
- td.appendChild(imageEl);
- }
- return td;
- }
-
- private _updateImageLabel(
- section: HTMLElement,
- className: string,
- image: ImageInfo
- ) {
- const label = section.querySelector(
- '.' + className + ' span.label'
- ) as HTMLElement;
- this._setLabelText(label, image);
- }
-
- private _setLabelText(label: HTMLElement, image: ImageInfo | null) {
- label.textContent = getImageLabel(image);
- }
-
- private _emitImageLabels(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- let addNamesInLabel = false;
-
- if (
- this._baseImage &&
- this._revisionImage &&
- this._baseImage._name !== this._revisionImage._name
- ) {
- addNamesInLabel = true;
- }
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- let td = createElementDiff('td', 'left');
- let label = createElementDiff('label');
- let nameSpan;
- let labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._baseImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._baseImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- td = createElementDiff('td', 'right');
- label = createElementDiff('label');
- labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._revisionImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._revisionImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- section.appendChild(tr);
+ private autoBlink(): boolean {
+ return !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
}
override updateRenderPrefs(renderPrefs: RenderPreferences) {
- const imageViewer = this.outputEl.querySelector(
- 'gr-image-viewer'
- ) as GrImageViewer;
- if (this._useNewImageDiffUi && imageViewer) {
- imageViewer.automaticBlink =
- !!renderPrefs?.image_diff_prefs?.automatic_blink;
- }
+ this.renderPrefs = renderPrefs;
+
+ // We have to update `imageDiff.automaticBlink` manually, because `this` is
+ // not a LitElement.
+ const imageDiff = this.outputEl.querySelector(
+ 'gr-diff-image-new'
+ ) as GrDiffImageNew;
+ if (imageDiff) imageDiff.automaticBlink = this.autoBlink();
}
}
-function getImageLabel(image: ImageInfo | null) {
- if (image) {
- const type = image.type ?? image._expectedType;
- if (image._width && image._height) {
- return `${image._width}×${image._height} ${type}`;
- } else {
- return type;
- }
+@customElement('gr-diff-image-new')
+class GrDiffImageNew extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @property() automaticBlink = false;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
}
- return 'No image';
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-image-viewer
+ class="gr-diff"
+ .baseUrl=${imageSrc(this.baseImage)}
+ .revisionUrl=${imageSrc(this.revisionImage)}
+ .automaticBlink=${this.automaticBlink}
+ >
+ </gr-image-viewer>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+}
+
+@customElement('gr-diff-image-old')
+class GrDiffImageOld extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @query('img.left') baseImageEl?: HTMLImageElement;
+
+ @query('img.right') revisionImageEl?: HTMLImageElement;
+
+ @state() baseError?: string;
+
+ @state() revisionError?: string;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
+ }
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ ${this.renderImagePairRow()} ${this.renderImageLabelRow()}
+ </tbody>
+ ${this.renderEndpoint()}
+ `;
+ }
+
+ private renderEndpoint() {
+ return html`
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ ${this.renderEndpointParam('baseImage', this.baseImage)}
+ ${this.renderEndpointParam('revisionImage', this.revisionImage)}
+ </gr-endpoint-decorator>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+
+ private renderEndpointParam(name: string, value: unknown) {
+ if (!value) return nothing;
+ return html`
+ <gr-endpoint-param class="gr-diff" name=${name} .value=${value}>
+ </gr-endpoint-param>
+ `;
+ }
+
+ private renderImagePairRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">${this.renderImage(Side.LEFT)}</td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">${this.renderImage(Side.RIGHT)}</td>
+ </tr>
+ `;
+ }
+
+ private renderImage(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ if (!image) return nothing;
+ const error = side === Side.LEFT ? this.baseError : this.revisionError;
+ if (error) return error;
+ const src = imageSrc(image);
+ if (!src) return nothing;
+
+ return html`
+ <img
+ class="gr-diff ${side}"
+ src=${src}
+ @load=${this.handleLoad}
+ @error=${(e: Event) => this.handleError(e, side)}
+ >
+ </img>
+ `;
+ }
+
+ private handleLoad() {
+ this.requestUpdate();
+ }
+
+ private handleError(e: Event, side: Side) {
+ const msg = `[Image failed to load] ${e.type}`;
+ if (side === Side.LEFT) this.baseError = msg;
+ if (side === Side.RIGHT) this.revisionError = msg;
+ }
+
+ private renderImageLabelRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">
+ <label class="gr-diff">
+ ${this.renderName(this.baseImage?._name ?? '')}
+ <span class="gr-diff label">${this.imageLabel(Side.LEFT)}</span>
+ </label>
+ </td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">
+ <label class="gr-diff">
+ ${this.renderName(this.revisionImage?._name ?? '')}
+ <span class="gr-diff label"> ${this.imageLabel(Side.RIGHT)} </span>
+ </label>
+ </td>
+ </tr>
+ `;
+ }
+
+ private renderName(name?: string) {
+ const addNamesInLabel =
+ this.baseImage &&
+ this.revisionImage &&
+ this.baseImage._name !== this.revisionImage._name;
+ if (!addNamesInLabel) return nothing;
+ return html`
+ <span class="gr-diff name">${name}</span><br class="gr-diff" />
+ `;
+ }
+
+ private imageLabel(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ const imageEl =
+ side === Side.LEFT ? this.baseImageEl : this.revisionImageEl;
+ if (image) {
+ const type = image.type ?? image._expectedType;
+ if (imageEl?.naturalWidth && imageEl.naturalHeight) {
+ return `${imageEl?.naturalWidth}×${imageEl.naturalHeight} ${type}`;
+ } else {
+ return type;
+ }
+ }
+ return 'No image';
+ }
+}
+
+function imageSrc(image?: ImageInfo): string {
+ return image && IMAGE_MIME_PATTERN.test(image.type)
+ ? `data:${image.type};base64,${image.body}`
+ : '';
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-diff-image-new': GrDiffImageNew;
+ 'gr-diff-image-old': GrDiffImageOld;
+ }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
index 2c9f210..090d125 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
@@ -226,6 +226,7 @@
}
const cell = createElementDiff('td', 'dividerCell');
+ // Note that <td> table cells that have `display: none` don't count!
const colspan = this.renderPrefs?.show_sign_col ? '5' : '3';
cell.setAttribute('colspan', colspan);
row.appendChild(cell);
@@ -363,23 +364,26 @@
}
td.classList.add(line.type);
- const {beforeNumber, afterNumber} = line;
- if (beforeNumber !== 'FILE' && beforeNumber !== 'LOST') {
+ const lineNumber = side ? line.lineNumber(side) : 0;
+ if (lineNumber === 'FILE') {
+ td.classList.add('file');
+ } else if (lineNumber === 'LOST') {
+ td.classList.add('lost');
+ } else {
const responsiveMode = getResponsiveMode(this._prefs, this.renderPrefs);
+ const contentId =
+ side && lineNumber > 0 ? `${side}-content-${lineNumber}` : '';
const contentText = formatText(
line.text,
responsiveMode,
this._prefs.tab_size,
this._prefs.line_length,
- side === Side.LEFT
- ? `left-content-${beforeNumber}`
- : `right-content-${afterNumber}`
+ contentId
);
if (side) {
contentText.setAttribute('data-side', side);
- const number = side === Side.LEFT ? beforeNumber : afterNumber;
- this.addLineNumberMouseEvents(td, number, side);
+ this.addLineNumberMouseEvents(td, lineNumber, side);
}
if (lineNumberEl && side) {
@@ -393,11 +397,9 @@
}
td.appendChild(contentText);
- } else if (line.beforeNumber === 'FILE') td.classList.add('file');
- else if (line.beforeNumber === 'LOST') td.classList.add('lost');
+ }
- if (side && line.lineNumber(side)) {
- const lineNumber = line.lineNumber(side);
+ if (side && lineNumber) {
const threadGroupEl = document.createElement('div');
threadGroupEl.className = 'thread-group';
threadGroupEl.setAttribute('data-side', side);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
index 10a947d..6ae7d4cd 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
@@ -83,14 +83,14 @@
colgroup.appendChild(createElementDiff('col', 'left'));
// Add right-side line number.
- col = document.createElement('col');
+ col = createElementDiff('col', 'right');
col.setAttribute('width', lineNumberWidth.toString());
colgroup.appendChild(col);
colgroup.appendChild(createElementDiff('col', 'sign right'));
// Add right-side content.
- colgroup.appendChild(document.createElement('col'));
+ colgroup.appendChild(createElementDiff('col', 'right'));
outputEl.appendChild(colgroup);
}
@@ -108,24 +108,17 @@
// pushing browser to compute aria even for tr. This can be removed, once
// browsers will again compute a11y label even for tr when it is focused.
// TODO: Remove when Chrome 102 is out of date for 1 year.
- if (
- leftLine.beforeNumber !== 'FILE' &&
- leftLine.beforeNumber !== 'LOST' &&
- rightLine.beforeNumber !== 'FILE' &&
- rightLine.beforeNumber !== 'LOST'
- ) {
- row.setAttribute(
- 'aria-labelledby',
- [
- leftLine.beforeNumber ? `left-button-${leftLine.beforeNumber}` : '',
- leftLine.beforeNumber ? `left-content-${leftLine.beforeNumber}` : '',
- rightLine.afterNumber ? `right-button-${rightLine.afterNumber}` : '',
- rightLine.afterNumber ? `right-content-${rightLine.afterNumber}` : '',
- ]
- .join(' ')
- .trim()
- );
- }
+ row.setAttribute(
+ 'aria-labelledby',
+ [
+ leftLine.beforeNumber ? `left-button-${leftLine.beforeNumber}` : '',
+ leftLine.beforeNumber ? `left-content-${leftLine.beforeNumber}` : '',
+ rightLine.afterNumber ? `right-button-${rightLine.afterNumber}` : '',
+ rightLine.afterNumber ? `right-content-${rightLine.afterNumber}` : '',
+ ]
+ .join(' ')
+ .trim()
+ );
row.appendChild(this.createBlameCell(leftLine.beforeNumber));
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
index f3c88a9..5ca5197 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
@@ -64,6 +64,16 @@
updateRenderPrefs(renderPrefs: RenderPreferences): void;
}
+export interface ImageDiffBuilder extends DiffBuilder {
+ renderImageDiff(): void;
+}
+
+export function isImageDiffBuilder(
+ x: DiffBuilder | ImageDiffBuilder | undefined
+): x is ImageDiffBuilder {
+ return !!x && !!(x as ImageDiffBuilder).renderImageDiff;
+}
+
/**
* Base class for different diff builders, like side-by-side, unified etc.
*
@@ -82,7 +92,7 @@
// visible for testing
readonly _prefs: DiffPreferencesInfo;
- protected readonly renderPrefs?: RenderPreferences;
+ protected renderPrefs?: RenderPreferences;
protected readonly outputEl: HTMLElement;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
index 35439d6..e5fce12 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -423,7 +423,10 @@
}
_rowHasThread(row: HTMLElement): boolean {
- return !!row.querySelector('.thread-group');
+ const slots = [
+ ...row.querySelectorAll<HTMLSlotElement>('.thread-group > slot'),
+ ];
+ return slots.some(slot => slot.assignedElements().length > 0);
}
/**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index cc7cd49..92175eb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -22,8 +22,14 @@
return this.getStringLength(node.textContent || '');
},
+ /**
+ * Returns the number of Unicode code points in the given string
+ *
+ * This is not necessarily the same as the number of visible symbols.
+ * See https://mathiasbynens.be/notes/javascript-unicode for more details.
+ */
getStringLength(str: string) {
- return str.replace(REGEX_ASTRAL_SYMBOL, '_').length;
+ return [...str].length;
},
/**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
index 6c45f20..fdf1785 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
@@ -327,4 +327,20 @@
assert.equal(el.getAttribute('class'), 'hello world');
});
});
+
+ suite('getStringLength', () => {
+ test('ASCII characters are counted correctly', () => {
+ assert.equal(GrAnnotation.getStringLength('ASCII'), 5);
+ });
+
+ test('Unicode surrogate pairs count as one symbol', () => {
+ assert.equal(GrAnnotation.getStringLength('Unic💢de'), 7);
+ assert.equal(GrAnnotation.getStringLength('💢💢'), 2);
+ });
+
+ test('Grapheme clusters count as multiple symbols', () => {
+ assert.equal(GrAnnotation.getStringLength('man\u0303ana'), 7); // mañana
+ assert.equal(GrAnnotation.getStringLength('q\u0307\u0323'), 3); // q̣̇
+ });
+ });
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 9f874b8..22a71a5 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -21,6 +21,7 @@
import {debounce, DelayedTask} from '../../../utils/async-util';
import {RenderPreferences} from '../../../api/diff';
import {assertIsDefined} from '../../../utils/common-util';
+import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
const WHOLE_FILE = -1;
@@ -639,16 +640,19 @@
rows: string[],
intralineInfos: number[][]
): Highlights[] {
+ // +1 to account for the \n that is not part of the rows passed here
+ const lineLengths = rows.map(r => GrAnnotation.getStringLength(r) + 1);
+
let rowIndex = 0;
let idx = 0;
const normalized = [];
for (const [skipLength, markLength] of intralineInfos) {
- let line = rows[rowIndex] + '\n';
+ let lineLength = lineLengths[rowIndex];
let j = 0;
while (j < skipLength) {
- if (idx === line.length) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
continue;
}
idx++;
@@ -660,10 +664,10 @@
};
j = 0;
- while (line && j < markLength) {
- if (idx === line.length) {
+ while (lineLength && j < markLength) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
normalized.push(lineHighlight);
lineHighlight = {
contentIndex: rowIndex,
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index 6caeb62..f6f7052 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -723,6 +723,25 @@
endIndex: 41,
},
]);
+
+ content = ['🙈 a', '🙉 b', '🙊 c'];
+ highlights = [[2, 7]];
+ results = element.convertIntralineInfos(content, highlights);
+ assert.deepEqual(results, [
+ {
+ contentIndex: 0,
+ startIndex: 2,
+ },
+ {
+ contentIndex: 1,
+ startIndex: 0,
+ },
+ {
+ contentIndex: 2,
+ startIndex: 0,
+ endIndex: 1,
+ },
+ ]);
});
test('scrolling pauses rendering', () => {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index f9a31b4..569de48 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -973,6 +973,8 @@
z-index: 10;
}
+ gr-diff-image-new,
+ gr-diff-image-old,
gr-diff-section,
gr-context-controls-section,
gr-diff-row {
@@ -1004,6 +1006,7 @@
if (this.diffTable && this.diffBuilder) {
this.highlights.init(this.diffTable, this.diffBuilder);
}
+ this.diffBuilder.init();
}
override disconnectedCallback() {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 1db3945..650ff78 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -93,12 +93,13 @@
<col class="gr-diff left" width="48" />
<col class="gr-diff left sign" />
<col class="gr-diff left" />
- <col width="48" />
+ <col class="gr-diff right" width="48" />
<col class="gr-diff right sign" />
- <col />
+ <col class="gr-diff right" />
</colgroup>
<tbody class="both gr-diff section">
<tr
+ aria-labelledby="left-button-LOST left-content-LOST right-button-LOST right-content-LOST"
class="diff-row gr-diff side-by-side"
left-type="both"
right-type="both"
@@ -119,6 +120,7 @@
</tbody>
<tbody class="both gr-diff section">
<tr
+ aria-labelledby="left-button-FILE left-content-FILE right-button-FILE right-content-FILE"
class="diff-row gr-diff side-by-side"
left-type="both"
right-type="both"
@@ -367,11 +369,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="5">
<button
@@ -405,11 +403,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="6">
<button
@@ -443,11 +437,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="7">
<button
@@ -750,11 +740,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -788,11 +774,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -826,11 +808,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -864,11 +842,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1371,11 +1345,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="40">
<button
@@ -1409,11 +1379,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="41">
<button
@@ -1447,11 +1413,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="42">
<button
@@ -1485,11 +1447,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="43">
<button
@@ -1974,14 +1932,14 @@
<td class="blank gr-diff left lineNum"></td>
<td class="gr-diff left">
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
</td>
<td class="blank gr-diff lineNum right"></td>
<td class="gr-diff right">
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
</td>
@@ -2003,6 +1961,22 @@
</tbody>
`
);
+ const endpoint = queryAndAssert(element, 'tbody.endpoint');
+ assert.dom.equal(
+ endpoint,
+ /* HTML */ `
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ <gr-endpoint-param class="gr-diff" name="baseImage">
+ </gr-endpoint-param>
+ <gr-endpoint-param class="gr-diff" name="revisionImage">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </tr>
+ </tbody>
+ `
+ );
});
test('renders image diffs with a different file name', async () => {
@@ -2081,7 +2055,7 @@
rightImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
`
@@ -2115,7 +2089,7 @@
leftImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
`
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 93a3f2a..7370c96 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -750,8 +750,6 @@
type: string;
_name?: string;
_expectedType?: string;
- _width?: number;
- _height?: number;
}
/**
