Merge changes I5a7749fc,Ia8703c0d
* changes:
Do not fail with ISE when an invalid base is specified on push
Fix GetRelated if multiple changes exist for the same commit
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/README.md b/README.md
index 4df9271..c8f0b70 100644
--- a/README.md
+++ b/README.md
@@ -3,7 +3,7 @@
[Gerrit](https://www.gerritcodereview.com) is a code review and project
management tool for Git based projects.
-[](https://gerrit-ci.gerritforge.com/job/Gerrit-bazel-java11-master/)
+[](https://gerrit-ci.gerritforge.com/view/Gerrit/job/Gerrit-bazel-master/)
## Objective
diff --git a/contrib/find-duplicate-usernames.sh b/contrib/find-duplicate-usernames.sh
index b59e5be..7a5750f 100755
--- a/contrib/find-duplicate-usernames.sh
+++ b/contrib/find-duplicate-usernames.sh
@@ -29,6 +29,18 @@
usage
fi
+if [ -z "$(git ls-remote . refs/meta/external-ids)" ]; then
+ cat <<EOF
+Could not find 'refs/meta/external-ids' in the local repository.
+
+Please fetch it using:
+
+ git fetch "$(git remote)" refs/meta/external-ids:refs/meta/external-ids
+
+EOF
+ exit 1
+fi
+
# 1. find lines with user name and subsequent line in external-ids notes branch
# example output of git grep -A1 "\[externalId \"username:" refs/meta/external-ids:
# refs/meta/external-ids:00/1d/abd037e437f71d42134e6ad532a06948a2ba:[externalId "username:johndoe"]
@@ -45,12 +57,12 @@
# 7. flip columns
# 8. uniq case-insensitive, only show duplicates, avoid comparing first field
# 9. flip columns back
-git grep -A1 "\[externalId \"$1:" refs/meta/external-ids \
+git grep -A1 "\[externalId \"$1:" refs/meta/external-ids -- \
| sed -E "/$1/,/accountId/!d" \
| paste -d ' ' - - \
| tr \"= : \
| cut -d: --output-delimiter="" -f 5,8 \
| sort -f \
- | sed -E "s/(.*) (.*)/\2 \1/" \
+ | sed -E "s/(.*) ([0-9]+)/\2 \1/" \
| uniq -Di -f1 \
- | sed -E "s/(.*) (.*)/\2 \1/"
+ | sed -E "s/([0-9]+) (.*)/\2 \1/"
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/common/UsedAt.java b/java/com/google/gerrit/common/UsedAt.java
index de71b3c..1b87f32 100644
--- a/java/com/google/gerrit/common/UsedAt.java
+++ b/java/com/google/gerrit/common/UsedAt.java
@@ -45,6 +45,7 @@
PLUGIN_MULTI_SITE,
PLUGIN_SERVICEUSER,
PLUGIN_WEBSESSION_FLATFILE,
+ MODULE_GIT_REFS_FILTER
}
/** Reference to the project that uses the method annotated with this annotation. */
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/httpd/GitOverHttpServlet.java b/java/com/google/gerrit/httpd/GitOverHttpServlet.java
index 86c514b..e513a72 100644
--- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java
+++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java
@@ -14,7 +14,6 @@
package com.google.gerrit.httpd;
-import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import static org.eclipse.jgit.http.server.GitSmartHttpTools.sendError;
import com.google.common.annotations.VisibleForTesting;
@@ -508,7 +507,7 @@
if (!rsp.isCommitted()) {
rsp.reset();
String msg = e instanceof PackProtocolException ? e.getMessage() : null;
- sendError(req, rsp, SC_INTERNAL_SERVER_ERROR, msg);
+ sendError(req, rsp, UploadPackErrorHandler.statusCodeForThrowable(e), msg);
}
}
}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 23de6db..396ba74 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -30,7 +30,6 @@
import static com.google.common.net.HttpHeaders.CONTENT_TYPE;
import static com.google.common.net.HttpHeaders.ORIGIN;
import static com.google.common.net.HttpHeaders.VARY;
-import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG;
import static java.math.RoundingMode.CEILING;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -120,9 +119,7 @@
import com.google.gerrit.server.cancellation.RequestStateContext;
import com.google.gerrit.server.cancellation.RequestStateProvider;
import com.google.gerrit.server.change.ChangeFinder;
-import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.group.GroupAuditService;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.PerformanceLogContext;
@@ -270,7 +267,6 @@
final PluginSetContext<ExceptionHook> exceptionHooks;
final Injector injector;
final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
- final ExperimentFeatures experimentFeatures;
final DeadlineChecker.Factory deadlineCheckerFactory;
final CancellationMetrics cancellationMetrics;
@@ -291,7 +287,6 @@
PluginSetContext<ExceptionHook> exceptionHooks,
Injector injector,
DynamicMap<DynamicOptions.DynamicBean> dynamicBeans,
- ExperimentFeatures experimentFeatures,
DeadlineChecker.Factory deadlineCheckerFactory,
CancellationMetrics cancellationMetrics) {
this.currentUser = currentUser;
@@ -310,7 +305,6 @@
allowOrigin = makeAllowOrigin(config);
this.injector = injector;
this.dynamicBeans = dynamicBeans;
- this.experimentFeatures = experimentFeatures;
this.deadlineCheckerFactory = deadlineCheckerFactory;
this.cancellationMetrics = cancellationMetrics;
}
@@ -862,11 +856,6 @@
TraceContext.newTimer(
"RestApiServlet#getEtagWithRetry:resource",
Metadata.builder().restViewName(rsrc.getClass().getSimpleName()).build())) {
- if (rsrc instanceof RevisionResource
- && globals.experimentFeatures.isFeatureEnabled(
- GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG)) {
- return null;
- }
return invokeRestEndpointWithRetry(
req,
traceContext,
diff --git a/java/com/google/gerrit/index/testing/BUILD b/java/com/google/gerrit/index/testing/BUILD
index 9af2598..44bf70d 100644
--- a/java/com/google/gerrit/index/testing/BUILD
+++ b/java/com/google/gerrit/index/testing/BUILD
@@ -10,13 +10,9 @@
deps = [
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/entities",
- "//java/com/google/gerrit/exceptions",
"//java/com/google/gerrit/index",
- "//java/com/google/gerrit/index:query_exception",
"//java/com/google/gerrit/index/project",
- "//java/com/google/gerrit/proto",
"//java/com/google/gerrit/server",
- "//java/com/google/gerrit/server/logging",
"//lib:guava",
"//lib:jgit",
"//lib:protobuf",
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/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index efc8d54..5a4580c 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -24,9 +24,6 @@
public static String UI_FEATURE_SUBMIT_REQUIREMENTS_UI = "UiFeature__submit_requirements_ui";
- public static String GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG =
- "GerritBackendRequestFeature__remove_revision_etag";
-
public static String GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION =
"GerritBackendFeature__attach_nonce_to_documentation";
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index 03f4c50..340d956 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -335,7 +335,7 @@
int size = sr.all().size();
if (size > 0) {
changeCount.addAndGet(size);
- int slices = 1 + size / PROJECT_SLICE_MAX_REFS;
+ int slices = 1 + (size - 1) / PROJECT_SLICE_MAX_REFS;
if (slices > 1) {
verboseWriter.println(
"Submitting " + name + " for indexing in " + slices + " slices");
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index ecdda71..52f540d 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -37,6 +37,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.errorprone.annotations.FormatMethod;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.BranchNameKey;
@@ -138,6 +139,25 @@
return createChecked(c.getProject(), c.getId());
}
+ /**
+ * Load the change-notes associated to a project/change-id using an existing open repository
+ *
+ * @param repo existing open repository
+ * @param project project associated with the repository
+ * @param changeId change-id associated with the change-notes to load
+ * @param metaRevId version of the change-id to load, null for loading the latest
+ * @return change-notes object for the change
+ */
+ @UsedAt(UsedAt.Project.MODULE_GIT_REFS_FILTER)
+ public ChangeNotes createChecked(
+ Repository repo,
+ Project.NameKey project,
+ Change.Id changeId,
+ @Nullable ObjectId metaRevId) {
+ Change change = newChange(project, changeId);
+ return new ChangeNotes(args, change, true, null, metaRevId).load(repo);
+ }
+
public ChangeNotes createChecked(
Project.NameKey project, Change.Id changeId, @Nullable ObjectId metaRevId) {
Change change = newChange(project, changeId);
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/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index 4c84bd3..e0805c0 100644
--- a/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -41,6 +41,8 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryableAction.ActionType;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
@@ -163,6 +165,8 @@
@Inject private PatchSetParser psParser;
+ @Inject private RetryHelper retryHelper;
+
private Map<Option, LabelSetter> optionMap;
private Map<String, Short> customLabels;
@@ -243,11 +247,19 @@
}
}
- private void applyReview(PatchSet patchSet, ReviewInput review) throws RestApiException {
- gApi.changes()
- .id(patchSet.id().changeId().get())
- .revision(patchSet.commitId().name())
- .review(review);
+ private void applyReview(PatchSet patchSet, ReviewInput review) throws Exception {
+ retryHelper
+ .action(
+ ActionType.CHANGE_UPDATE,
+ "applyReview",
+ () -> {
+ gApi.changes()
+ .id(patchSet.id().changeId().get())
+ .revision(patchSet.commitId().name())
+ .review(review);
+ return null;
+ })
+ .call();
}
private ReviewInput reviewFromJson() throws UnloggedFailure {
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/AbstractGitOverHttpServlet.java b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java
index a22759f..a7e673a 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.FakeGroupAuditService;
@@ -26,6 +27,7 @@
import com.google.inject.Inject;
import java.util.Optional;
import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jgit.api.errors.TransportException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
@@ -106,6 +108,38 @@
uploadPackAuditEventLog(remote, Optional.empty());
}
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void wantNotValidErrorOverHTTPShouldResultIn200OKHttpStatus() throws Exception {
+ String remote = "origin";
+ String uri = admin.getHttpUrl(server) + "/a/" + project.get();
+ cfg.setString("remote", remote, "url", uri);
+ cfg.setString("remote", remote, "fetch", "+refs/heads/*:refs/remotes/origin/*");
+ String wantNotValidCommit = "554013834d49a69a2f3c494de195ee606dd6d035";
+
+ auditService.drainHttpAuditEvents();
+
+ TransportException thrown =
+ assertThrows(
+ TransportException.class,
+ () ->
+ testRepo
+ .git()
+ .fetch()
+ .setRemote(remote)
+ .setRefSpecs(new RefSpec(wantNotValidCommit))
+ .call());
+
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(String.format("want %s not valid", wantNotValidCommit));
+
+ assertThat(
+ auditService.drainHttpAuditEvents().stream()
+ .allMatch(e -> e.httpStatus == HttpServletResponse.SC_OK))
+ .isTrue();
+ }
+
/**
* Git client use Protocol V2 fetch by default, see https://git.eclipse.org/r/c/jgit/jgit/+/172595
* See {@code org.eclipse.jgit.transport.BasePackFetchConnection#doFetchV2} for the negotiation
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index db53555..b738324 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/json/BUILD b/javatests/com/google/gerrit/json/BUILD
index 575f575..a242b0e 100644
--- a/javatests/com/google/gerrit/json/BUILD
+++ b/javatests/com/google/gerrit/json/BUILD
@@ -5,7 +5,6 @@
srcs = glob(["*.java"]),
deps = [
"//java/com/google/gerrit/json",
- "//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:gson",
"//lib:guava",
diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD
index c708e09..275f2ec 100644
--- a/javatests/com/google/gerrit/server/cache/BUILD
+++ b/javatests/com/google/gerrit/server/cache/BUILD
@@ -7,11 +7,9 @@
deps = [
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/testing:gerrit-test-util",
- "//javatests/com/google/gerrit/util/http/testutil",
"//lib:junit",
"//lib/truth",
"//lib/truth:truth-java8-extension",
- "@servlet-api//jar",
],
)
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/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index 1b286d1..37d8468 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -127,6 +127,8 @@
protected Injector injector;
private String systemTimeZone;
+ @Inject protected ChangeNotes.Factory changeNotesFactory;
+
@Before
public void setUpTestEnvironment() throws Exception {
setupTestPrerequisites();
diff --git a/javatests/com/google/gerrit/server/notedb/OpenRepoTest.java b/javatests/com/google/gerrit/server/notedb/OpenRepoTest.java
index 507b71f..323aee9 100644
--- a/javatests/com/google/gerrit/server/notedb/OpenRepoTest.java
+++ b/javatests/com/google/gerrit/server/notedb/OpenRepoTest.java
@@ -244,6 +244,19 @@
}
}
+ @Test
+ public void canCreateChangeNotesFromOpenRepoAndChangeid() throws Exception {
+ try (OpenRepo openRepo = openRepo()) {
+ Change change = newChange();
+
+ ChangeNotes changeNotes =
+ changeNotesFactory.createChecked(openRepo.repo, project, change.getId(), null);
+
+ assertThat(changeNotes).isNotNull();
+ assertThat(changeNotes.getChangeId()).isEqualTo(change.getId());
+ }
+ }
+
private void addToAttentionSet(ChangeUpdate update) {
AttentionSetUpdate attentionSetUpdate =
AttentionSetUpdate.createForWrite(
diff --git a/lib/auto/BUILD b/lib/auto/BUILD
index e01d91b..6e5418e 100644
--- a/lib/auto/BUILD
+++ b/lib/auto/BUILD
@@ -101,8 +101,6 @@
],
visibility = ["//visibility:public"],
exports = [
- "@auto-value-gson-extension//jar",
- "@auto-value-gson-factory//jar",
"@auto-value-gson-runtime//jar",
],
)
diff --git a/modules/jgit b/modules/jgit
index 2021ce3..b39972f 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit 2021ce3423a7db6949b9e0a71a8c15e5826ccc4c
+Subproject commit b39972f8a20e2d9ad62629029b85e443d1bd082d
diff --git a/polygerrit-ui/app/api/annotation.ts b/polygerrit-ui/app/api/annotation.ts
index c670c58..c394ef7 100644
--- a/polygerrit-ui/app/api/annotation.ts
+++ b/polygerrit-ui/app/api/annotation.ts
@@ -3,7 +3,7 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {CoverageRange, Side} from './diff';
+import {CoverageRange} from './diff';
import {ChangeInfo} from './rest-api';
/**
@@ -28,18 +28,5 @@
* providers are not supported. A second call will just overwrite the
* provider of the first call.
*/
- setCoverageProvider(coverageProvider: CoverageProvider): AnnotationPluginApi;
-
- /**
- * For plugins notifying Gerrit about new annotations being ready to be
- * applied for a certain range. Gerrit will then re-render the relevant lines
- * of the diff and call back to the layer annotation function that was
- * registered in addLayer().
- *
- * @param path The file path whose listeners should be notified.
- * @param start The line where the update starts.
- * @param end The line where the update ends.
- * @param side The side of the update ('left' or 'right').
- */
- notify(path: string, start: number, end: number, side: Side): void;
+ setCoverageProvider(coverageProvider: CoverageProvider): void;
}
diff --git a/polygerrit-ui/app/api/plugin.ts b/polygerrit-ui/app/api/plugin.ts
index ef70a75..733f112 100644
--- a/polygerrit-ui/app/api/plugin.ts
+++ b/polygerrit-ui/app/api/plugin.ts
@@ -30,7 +30,6 @@
REVERT = 'revert',
REVERT_SUBMISSION = 'revert_submission',
POST_REVERT = 'postrevert',
- ANNOTATE_DIFF = 'annotatediff',
ADMIN_MENU_LINKS = 'admin-menu-links',
}
diff --git a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.ts b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.ts
index 87761a6..8824009 100644
--- a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.ts
+++ b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.ts
@@ -3,7 +3,6 @@
* Copyright 2017 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '../../shared/gr-account-label/gr-account-label';
import '../../shared/gr-autocomplete/gr-autocomplete';
import '../../shared/gr-button/gr-button';
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
index 2a648b1..bb1e926 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
@@ -3,7 +3,6 @@
* Copyright 2017 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
import '../../plugins/gr-endpoint-param/gr-endpoint-param';
import '../../shared/gr-button/gr-button';
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar.ts
index 318a33b..a49a95e 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar.ts
@@ -14,7 +14,7 @@
import '../gr-change-list-bulk-vote-flow/gr-change-list-bulk-vote-flow';
import '../gr-change-list-topic-flow/gr-change-list-topic-flow';
import '../gr-change-list-hashtag-flow/gr-change-list-hashtag-flow';
-
+import '../gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow';
/**
* An action bar for the top of a <gr-change-list-section> element. Assumes it
* will be used inside a <tr> element.
@@ -77,6 +77,7 @@
<gr-change-list-topic-flow></gr-change-list-topic-flow>
<gr-change-list-hashtag-flow></gr-change-list-hashtag-flow>
<gr-change-list-reviewer-flow></gr-change-list-reviewer-flow>
+ <gr-change-list-bulk-abandon-flow></gr-change-list-bulk-abandon-flow>
</div>
</div>
</td>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar_test.ts
index 8badced..7a4f97c 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-action-bar/gr-change-list-action-bar_test.ts
@@ -68,6 +68,7 @@
<gr-change-list-topic-flow></gr-change-list-topic-flow>
<gr-change-list-hashtag-flow></gr-change-list-hashtag-flow>
<gr-change-list-reviewer-flow></gr-change-list-reviewer-flow>
+ <gr-change-list-bulk-abandon-flow></gr-change-list-bulk-abandon-flow>
</div>
</div>
</td>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts
new file mode 100644
index 0000000..11de37e
--- /dev/null
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts
@@ -0,0 +1,158 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {customElement, state, query} from 'lit/decorators.js';
+import {LitElement, html, css} from 'lit';
+import {resolve} from '../../../models/dependency';
+import {bulkActionsModelToken} from '../../../models/bulk-actions/bulk-actions-model';
+import {NumericChangeId, ChangeInfo, ChangeStatus} from '../../../api/rest-api';
+import {subscribe} from '../../lit/subscription-controller';
+import {ProgressStatus} from '../../../constants/constants';
+import '../../shared/gr-dialog/gr-dialog';
+import {fireAlert, fireReload} from '../../../utils/event-util';
+import {modalStyles} from '../../../styles/gr-modal-styles';
+
+@customElement('gr-change-list-bulk-abandon-flow')
+export class GrChangeListBulkAbandonFlow extends LitElement {
+ private readonly getBulkActionsModel = resolve(this, bulkActionsModelToken);
+
+ @state() selectedChanges: ChangeInfo[] = [];
+
+ @state() progress: Map<NumericChangeId, ProgressStatus> = new Map();
+
+ @query('#actionModal') actionModal!: HTMLDialogElement;
+
+ static override get styles() {
+ return [
+ modalStyles,
+ css`
+ section {
+ padding: var(--spacing-l);
+ }
+ `,
+ ];
+ }
+
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getBulkActionsModel().selectedChanges$,
+ selectedChanges => (this.selectedChanges = selectedChanges)
+ );
+ }
+
+ override render() {
+ return html`
+ <gr-button
+ id="abandon"
+ flatten
+ .disabled=${!this.isEnabled()}
+ @click=${() => this.actionModal.showModal()}
+ >Abandon</gr-button
+ >
+ <dialog id="actionModal" tabindex="-1">
+ <gr-dialog
+ .disableCancel=${!this.isCancelEnabled()}
+ .disabled=${!this.isConfirmEnabled()}
+ @confirm=${() => this.handleConfirm()}
+ @cancel=${() => this.handleClose()}
+ .cancelLabel=${'Close'}
+ >
+ <div slot="header">
+ ${this.selectedChanges.length} changes to abandon
+ </div>
+ <div slot="main">
+ <table>
+ <thead>
+ <tr>
+ <th>Subject</th>
+ <th>Status</th>
+ </tr>
+ </thead>
+ <tbody>
+ ${this.selectedChanges.map(
+ change => html`
+ <tr>
+ <td>Change: ${change.subject}</td>
+ <td id="status">
+ Status: ${this.getStatus(change._number)}
+ </td>
+ </tr>
+ `
+ )}
+ </tbody>
+ </table>
+ </div>
+ </gr-dialog>
+ </dialog>
+ `;
+ }
+
+ private getStatus(changeNum: NumericChangeId) {
+ return this.progress.has(changeNum)
+ ? this.progress.get(changeNum)
+ : ProgressStatus.NOT_STARTED;
+ }
+
+ private isEnabled() {
+ return this.selectedChanges.every(
+ change =>
+ !!change.actions?.abandon || change.status === ChangeStatus.ABANDONED
+ );
+ }
+
+ private isConfirmEnabled() {
+ // Action is allowed if none of the changes have any bulk action performed
+ // on them. In case an error happens then we keep the button disabled.
+ for (const status of this.progress.values()) {
+ if (status !== ProgressStatus.NOT_STARTED) return false;
+ }
+ return true;
+ }
+
+ private isCancelEnabled() {
+ for (const status of this.progress.values()) {
+ if (status === ProgressStatus.RUNNING) return false;
+ }
+ return true;
+ }
+
+ private handleConfirm() {
+ this.progress.clear();
+ for (const change of this.selectedChanges) {
+ this.progress.set(change._number, ProgressStatus.RUNNING);
+ }
+ this.requestUpdate();
+ const errFn = (changeNum: NumericChangeId) => {
+ throw new Error(`request for ${changeNum} failed`);
+ };
+ const promises = this.getBulkActionsModel().abandonChanges('', errFn);
+ for (let index = 0; index < promises.length; index++) {
+ const changeNum = this.selectedChanges[index]._number;
+ promises[index]
+ .then(() => {
+ this.progress.set(changeNum, ProgressStatus.SUCCESSFUL);
+ this.requestUpdate();
+ })
+ .catch(() => {
+ this.progress.set(changeNum, ProgressStatus.FAILED);
+ this.requestUpdate();
+ });
+ }
+ }
+
+ private handleClose() {
+ this.actionModal.close();
+ fireAlert(this, 'Reloading page..');
+ fireReload(this, true);
+ }
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-change-list-bulk-abandon-flow': GrChangeListBulkAbandonFlow;
+ }
+}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts
new file mode 100644
index 0000000..df7a6ce
--- /dev/null
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts
@@ -0,0 +1,344 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {createChange} from '../../../test/test-data-generators';
+import {
+ NumericChangeId,
+ ChangeInfo,
+ ChangeStatus,
+ HttpMethod,
+ PatchSetNum,
+} from '../../../api/rest-api';
+import {GrChangeListBulkAbandonFlow} from './gr-change-list-bulk-abandon-flow';
+import '../../../test/common-test-setup';
+import {
+ BulkActionsModel,
+ bulkActionsModelToken,
+ LoadingState,
+} from '../../../models/bulk-actions/bulk-actions-model';
+import './gr-change-list-bulk-abandon-flow';
+import {fixture, waitUntil, assert} from '@open-wc/testing';
+import {wrapInProvider} from '../../../models/di-provider-element';
+import {html} from 'lit';
+import {getAppContext} from '../../../services/app-context';
+import {
+ waitUntilObserved,
+ stubRestApi,
+ queryAndAssert,
+ mockPromise,
+ query,
+} from '../../../test/test-utils';
+import {GrButton} from '../../shared/gr-button/gr-button';
+import {ProgressStatus} from '../../../constants/constants';
+import {RequestPayload} from '../../../types/common';
+import {ErrorCallback} from '../../../api/rest';
+
+const change1: ChangeInfo = {...createChange(), _number: 1 as NumericChangeId};
+const change2: ChangeInfo = {...createChange(), _number: 2 as NumericChangeId};
+
+suite('gr-change-list-bulk-abandon-flow tests', () => {
+ let element: GrChangeListBulkAbandonFlow;
+ let model: BulkActionsModel;
+ let getChangesStub: sinon.SinonStub;
+
+ async function selectChange(change: ChangeInfo) {
+ model.addSelectedChangeNum(change._number);
+ await waitUntilObserved(model.selectedChangeNums$, selectedChangeNums =>
+ selectedChangeNums.includes(change._number)
+ );
+ await element.updateComplete;
+ }
+
+ setup(async () => {
+ model = new BulkActionsModel(getAppContext().restApiService);
+ getChangesStub = stubRestApi('getDetailedChangesWithActions');
+
+ element = (
+ await fixture(
+ wrapInProvider(
+ html`<gr-change-list-bulk-abandon-flow></gr-change-list-bulk-abandon-flow>`,
+ bulkActionsModelToken,
+ model
+ )
+ )
+ ).querySelector('gr-change-list-bulk-abandon-flow')!;
+ await element.updateComplete;
+ });
+
+ test('render', async () => {
+ const changes: ChangeInfo[] = [{...change1, actions: {abandon: {}}}];
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await element.updateComplete;
+
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <gr-button
+ aria-disabled="false"
+ flatten=""
+ id="abandon"
+ role="button"
+ tabindex="0"
+ >
+ Abandon
+ </gr-button>
+ <dialog id="actionModal" tabindex="-1">
+ <gr-dialog role="dialog">
+ <div slot="header">1 changes to abandon</div>
+ <div slot="main">
+ <table>
+ <thead>
+ <tr>
+ <th>Subject</th>
+ <th>Status</th>
+ </tr>
+ </thead>
+ <tbody>
+ <tr>
+ <td>Change: Test subject</td>
+ <td id="status">Status: NOT STARTED</td>
+ </tr>
+ </tbody>
+ </table>
+ </div>
+ </gr-dialog>
+ </dialog>
+ `
+ );
+ });
+
+ test('button state updates as changes are updated', async () => {
+ const changes: ChangeInfo[] = [{...change1, actions: {abandon: {}}}];
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await element.updateComplete;
+
+ assert.isFalse(queryAndAssert<GrButton>(element, '#abandon').disabled);
+
+ changes.push({...change2, actions: {}});
+ getChangesStub.restore();
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change2);
+ await element.updateComplete;
+
+ assert.isTrue(queryAndAssert<GrButton>(element, '#abandon').disabled);
+ });
+
+ test('abandon button is enabled if change is already abandoned', async () => {
+ const changes: ChangeInfo[] = [
+ {...change1, actions: {}, status: ChangeStatus.ABANDONED},
+ ];
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await element.updateComplete;
+
+ assert.isFalse(queryAndAssert<GrButton>(element, '#abandon').disabled);
+
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').click();
+
+ await waitUntil(
+ () =>
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim() === `Status: ${ProgressStatus.SUCCESSFUL}`
+ );
+ });
+
+ test('progress updates as request is resolved', async () => {
+ const changes: ChangeInfo[] = [{...change1, actions: {abandon: {}}}];
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await element.updateComplete;
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.NOT_STARTED}`
+ );
+
+ const executeChangeAction = mockPromise<Response>();
+ stubRestApi('executeChangeAction').returns(executeChangeAction);
+
+ assert.isNotOk(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').disabled
+ );
+ assert.isNotOk(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#cancel').disabled
+ );
+
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').click();
+ await element.updateComplete;
+
+ assert.isTrue(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').disabled
+ );
+ assert.isTrue(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#cancel').disabled
+ );
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.RUNNING}`
+ );
+
+ executeChangeAction.resolve({...new Response(), status: 200});
+ await waitUntil(
+ () =>
+ element.progress.get(1 as NumericChangeId) === ProgressStatus.SUCCESSFUL
+ );
+
+ assert.isTrue(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').disabled
+ );
+ assert.isNotOk(
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#cancel').disabled
+ );
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.SUCCESSFUL}`
+ );
+ });
+
+ test('failures are reflected to the progress dialog', async () => {
+ const changes: ChangeInfo[] = [{...change1, actions: {abandon: {}}}];
+ getChangesStub.returns(changes);
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await element.updateComplete;
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.NOT_STARTED}`
+ );
+
+ stubRestApi('executeChangeAction').callsFake(
+ (
+ _changeNum: NumericChangeId,
+ _method: HttpMethod | undefined,
+ _endpoint: string,
+ _patchNum?: PatchSetNum,
+ _payload?: RequestPayload,
+ errFn?: ErrorCallback
+ ) =>
+ Promise.resolve(new Response()).then(res => {
+ errFn && errFn();
+ return res;
+ })
+ );
+
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').click();
+ await element.updateComplete;
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.RUNNING}`
+ );
+
+ await waitUntil(
+ () => element.progress.get(1 as NumericChangeId) === ProgressStatus.FAILED
+ );
+
+ assert.equal(
+ queryAndAssert<HTMLTableDataCellElement>(
+ element,
+ '#status'
+ ).innerText.trim(),
+ `Status: ${ProgressStatus.FAILED}`
+ );
+ });
+
+ test('closing dialog triggers a reload', async () => {
+ const changes: ChangeInfo[] = [
+ {...change1, actions: {abandon: {}}},
+ {...change2, actions: {abandon: {}}},
+ ];
+ getChangesStub.returns(changes);
+
+ const fireStub = sinon.stub(element, 'dispatchEvent');
+
+ stubRestApi('executeChangeAction').callsFake(
+ (
+ _changeNum: NumericChangeId,
+ _method: HttpMethod | undefined,
+ _endpoint: string,
+ _patchNum?: PatchSetNum,
+ _payload?: RequestPayload,
+ errFn?: ErrorCallback
+ ) =>
+ Promise.resolve(new Response()).then(res => {
+ errFn && errFn();
+ return res;
+ })
+ );
+
+ model.sync(changes);
+ await waitUntilObserved(
+ model.loadingState$,
+ state => state === LoadingState.LOADED
+ );
+ await selectChange(change1);
+ await selectChange(change2);
+ await element.updateComplete;
+
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#confirm').click();
+
+ await waitUntil(
+ () => element.progress.get(2 as NumericChangeId) === ProgressStatus.FAILED
+ );
+
+ assert.isFalse(fireStub.called);
+
+ queryAndAssert<GrButton>(query(element, 'gr-dialog'), '#cancel').click();
+
+ await waitUntil(() => fireStub.called);
+ assert.equal(fireStub.lastCall.args[0].type, 'reload');
+ });
+});
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
index d9982b1..f206a9a 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
@@ -156,7 +156,7 @@
override willUpdate(changedProperties: PropertyValues<this>) {
// When the cursor selects this item, give it focus so that the item is read
// out by screen readers and lets users start tabbing through the item
- if (this.selected && !changedProperties.get('selected')) {
+ if (this.selected && changedProperties.has('selected')) {
this.focus();
}
}
@@ -276,6 +276,7 @@
.selectionLabel {
padding: 10px;
margin: -10px;
+ display: block;
}
.cell.label {
font-weight: var(--font-weight-normal);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
index cbb0d36..e8f7fc3 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
@@ -109,7 +109,7 @@
const checkbox = queryAndAssert<HTMLInputElement>(
element,
- '.selection > label > input'
+ '.selection > .selectionLabel > input'
);
checkbox.click();
let selectedChangeNums = await waitUntilObserved(
@@ -138,7 +138,7 @@
const checkbox = queryAndAssert<HTMLInputElement>(
element,
- '.selection > label > input'
+ '.selection > .selectionLabel > input'
);
checkbox.click();
await element.updateComplete;
@@ -161,7 +161,7 @@
const checkbox = queryAndAssert<HTMLInputElement>(
element,
- '.selection > label > input'
+ '.selection > .selectionLabel > input'
);
assert.isTrue(checkbox.checked);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
index 399631e..f4bd8bd 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
@@ -75,7 +75,7 @@
await changeListItemEl.updateComplete;
let checkbox = queryAndAssert<HTMLInputElement>(
changeListItemEl,
- '.selection > label > input'
+ '.selection > .selectionLabel > input'
);
checkbox.click();
await waitUntil(() => checkbox.checked);
@@ -88,7 +88,7 @@
query(query(element, 'gr-change-list'), 'gr-change-list-section'),
'gr-change-list-item'
),
- '.selection > label > input'
+ '.selection > .selectionLabel > input'
);
assert.isTrue(checkbox.checked);
});
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 44c0e0c..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: {
@@ -1886,8 +1887,7 @@
/* If there is only 1 change then gerrit will automatically
redirect to that change */
const topic = revertSubmistionInfo.revert_changes[0].topic;
- const query = `topic:${topic}`;
- if (topic) this.getNavigation().setUrl(createSearchUrl({query}));
+ this.getNavigation().setUrl(createSearchUrl({topic}));
break;
}
default:
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 3ba04f7..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
@@ -2494,7 +2527,7 @@
new Response()
);
assert.isTrue(setUrlStub.called);
- assert.equal(setUrlStub.lastCall.args[0], '/q/topic:T');
+ assert.equal(setUrlStub.lastCall.args[0], '/q/topic:"T"');
});
});
@@ -2533,7 +2566,7 @@
);
assert.isFalse(showActionDialogStub.called);
assert.isTrue(setUrlStub.called);
- assert.equal(setUrlStub.lastCall.args[0], '/q/topic:T');
+ assert.equal(setUrlStub.lastCall.args[0], '/q/topic:"T"');
});
});
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 18c1455..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()}
@@ -462,7 +462,14 @@
this.computeShowRepoBranchTogether(),
() =>
html`<section class=${this.computeDisplayState(Metadata.REPO_BRANCH)}>
- <span class="title">Repo | Branch</span>
+ <span class="title">
+ <gr-tooltip-content
+ has-tooltip
+ title="Repository and branch that the change will be merged into if submitted."
+ >
+ Repo | Branch
+ </gr-tooltip-content>
+ </span>
<span class="value">
<a href=${this.computeProjectUrl(change.project)}
>${change.project}</a
@@ -474,10 +481,17 @@
</span>
</section>`,
- () => html` <section
+ () => html`<section
class=${this.computeDisplayState(Metadata.REPO_BRANCH)}
>
- <span class="title">Repo</span>
+ <span class="title">
+ <gr-tooltip-content
+ has-tooltip
+ title="Repository that the change will be merged into if submitted."
+ >
+ Repo
+ </gr-tooltip-content>
+ </span>
<span class="value">
<a href=${this.computeProjectUrl(change.project)}>
<gr-limited-text
@@ -488,7 +502,14 @@
</span>
</section>
<section class=${this.computeDisplayState(Metadata.REPO_BRANCH)}>
- <span class="title">Branch</span>
+ <span class="title">
+ <gr-tooltip-content
+ has-tooltip
+ title="Branch that the change will be merged into if submitted."
+ >
+ Branch
+ </gr-tooltip-content>
+ </span>
<span class="value">
<a href=${this.computeBranchUrl(change.project, change.branch)}>
<gr-limited-text
@@ -540,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-change-metadata/gr-change-metadata_test.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts
index 50bb9d9..e45de51 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts
@@ -165,7 +165,12 @@
</section>
<section>
<span class="title">
- Repo | Branch
+ <gr-tooltip-content
+ has-tooltip=""
+ title="Repository and branch that the change will be merged into if submitted."
+ >
+ Repo | Branch
+ </gr-tooltip-content>
</span>
<span class="value">
<a href="/q/project:test-project">
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index 259154f..ffb180d 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -16,6 +16,7 @@
RepoName,
CommitId,
ChangeInfoId,
+ TopicName,
} from '../../../types/common';
import {customElement, property, query, state} from 'lit/decorators.js';
import {
@@ -582,8 +583,9 @@
if (!failedOrPending) {
// This needs some more work, as the new topic may not always be
// created, instead we may end up creating a new patchset */
- const query = `topic: "${topic}"`;
- this.getNavigation().setUrl(createSearchUrl({query}));
+ this.getNavigation().setUrl(
+ createSearchUrl({topic: topic as TopicName})
+ );
}
});
});
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/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 29bd1d9..11406b4 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -111,7 +111,7 @@
LabelNameToValuesMap,
PatchSetNumber,
} from '../../../api/rest-api';
-import {css, html, PropertyValues, LitElement} from 'lit';
+import {css, html, PropertyValues, LitElement, nothing} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
import {when} from 'lit/directives/when.js';
import {classMap} from 'lit/directives/class-map.js';
@@ -606,6 +606,16 @@
.patchsetLevelContainer.unresolved {
background-color: var(--unresolved-comment-background-color);
}
+ .privateVisiblityInfo {
+ display: flex;
+ justify-content: center;
+ background-color: var(--info-background);
+ padding: var(--spacing-s) 0;
+ }
+ .privateVisiblityInfo gr-icon {
+ margin-right: var(--spacing-m);
+ color: var(--info-foreground);
+ }
`,
];
@@ -791,6 +801,7 @@
<gr-endpoint-slot name="below"></gr-endpoint-slot>
</gr-endpoint-decorator>
${this.renderCCList()} ${this.renderReviewConfirmation()}
+ ${this.renderPrivateVisiblityInfo()}
</section>
<section class="labelsContainer">${this.renderLabels()}</section>
<section class="newReplyDialog textareaContainer">
@@ -893,6 +904,22 @@
`;
}
+ private renderPrivateVisiblityInfo() {
+ const addedAccounts = [
+ ...(this.reviewersList?.additions() ?? []),
+ ...(this.ccsList?.additions() ?? []),
+ ];
+ if (!this.change?.is_private || !addedAccounts.length) return nothing;
+ return html`
+ <div class="privateVisiblityInfo">
+ <gr-icon icon="info"></gr-icon>
+ <div>
+ Adding a reviewer/CC will make this private change visible to them
+ </div>
+ </div>
+ `;
+ }
+
private renderLabels() {
if (!this.change || !this.account || !this.permittedLabels) return;
return html`
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 633c5b0..52ea252 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -211,10 +211,7 @@
<div class="peopleListLabel">CC</div>
<gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
</div>
- <dialog
- tabindex="-1"
- id="reviewerConfirmationModal"
- >
+ <dialog tabindex="-1" id="reviewerConfirmationModal">
<div class="reviewerConfirmation">
Group
<span class="groupName"> </span>
@@ -232,7 +229,7 @@
No
</gr-button>
</div>
- </gr-overlay>
+ </dialog>
</section>
<section class="labelsContainer">
<gr-endpoint-decorator name="reply-label-scores">
@@ -330,6 +327,123 @@
);
});
+ test('renders private change info when reviewer is added', async () => {
+ element.change!.is_private = true;
+ element.requestUpdate();
+ await element.updateComplete;
+ const peopleContainer = queryAndAssert<HTMLDivElement>(
+ element,
+ '.peopleContainer'
+ );
+
+ // Info is rendered only if reviewer is added
+ assert.dom.equal(
+ peopleContainer,
+ `
+ <section class="peopleContainer">
+ <gr-endpoint-decorator name="reply-reviewers">
+ <gr-endpoint-param name="change"> </gr-endpoint-param>
+ <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
+ <div class="peopleList">
+ <div class="peopleListLabel">Reviewers</div>
+ <gr-account-list id="reviewers"> </gr-account-list>
+ <gr-endpoint-slot name="right"> </gr-endpoint-slot>
+ </div>
+ <gr-endpoint-slot name="below"> </gr-endpoint-slot>
+ </gr-endpoint-decorator>
+ <div class="peopleList">
+ <div class="peopleListLabel">CC</div>
+ <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
+ </div>
+ <dialog
+ tabindex="-1"
+ id="reviewerConfirmationModal"
+ >
+ <div class="reviewerConfirmation">
+ Group
+ <span class="groupName"> </span>
+ has
+ <span class="groupSize"> </span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="reviewerConfirmationButtons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </dialog>
+ </section>
+ `
+ );
+
+ const account = createAccountWithId(22);
+ element.reviewersList!.accounts = [];
+ element.reviewersList!.addAccountItem({account, count: 1});
+ element.reviewersList!.dispatchEvent(
+ new CustomEvent('account-added', {
+ detail: {account},
+ })
+ );
+ element.requestUpdate();
+ await element.updateComplete;
+
+ assert.dom.equal(
+ peopleContainer,
+ `
+ <section class="peopleContainer">
+ <gr-endpoint-decorator name="reply-reviewers">
+ <gr-endpoint-param name="change"> </gr-endpoint-param>
+ <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
+ <div class="peopleList">
+ <div class="peopleListLabel">Reviewers</div>
+ <gr-account-list id="reviewers"> </gr-account-list>
+ <gr-endpoint-slot name="right"> </gr-endpoint-slot>
+ </div>
+ <gr-endpoint-slot name="below"> </gr-endpoint-slot>
+ </gr-endpoint-decorator>
+ <div class="peopleList">
+ <div class="peopleListLabel">CC</div>
+ <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
+ </div>
+ <dialog
+ tabindex="-1"
+ id="reviewerConfirmationModal"
+ >
+ <div class="reviewerConfirmation">
+ Group
+ <span class="groupName"> </span>
+ has
+ <span class="groupSize"> </span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="reviewerConfirmationButtons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </dialog>
+ <div class="privateVisiblityInfo">
+ <gr-icon icon="info">
+ </gr-icon>
+ <div>
+ Adding a reviewer/CC will make this private change visible to them
+ </div>
+ </div>
+ </section>
+ `
+ );
+ });
+
test('default to publishing draft comments with reply', async () => {
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
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/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index f547cc7..8f1b7a2 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -521,6 +521,20 @@
this.redirect(url);
}
+ private dispatchLocationChangeEvent() {
+ const detail: LocationChangeEventDetail = {
+ hash: window.location.hash,
+ pathname: window.location.pathname,
+ };
+ document.dispatchEvent(
+ new CustomEvent('location-change', {
+ detail,
+ composed: true,
+ bubbles: true,
+ })
+ );
+ }
+
startRouter() {
const base = getBaseUrl();
if (base) {
@@ -568,17 +582,7 @@
// Fire asynchronously so that the URL is changed by the time the event
// is processed.
setTimeout(() => {
- const detail: LocationChangeEventDetail = {
- hash: window.location.hash,
- pathname: window.location.pathname,
- };
- document.dispatchEvent(
- new CustomEvent('location-change', {
- detail,
- composed: true,
- bubbles: true,
- })
- );
+ this.dispatchLocationChangeEvent();
}, 1);
next();
});
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index f1e70b0..bbca52d 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -35,6 +35,9 @@
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
import {userModelToken} from '../../../models/user/user-model';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {GrSyntaxLayerWorker} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
+import {highlightServiceToken} from '../../../services/highlight/highlight-service';
+import {anyLineTooLong} from '../../../embed/diff/gr-diff/gr-diff-utils';
interface FilePreview {
filepath: string;
@@ -94,15 +97,22 @@
private readonly getNavigation = resolve(this, navigationToken);
+ private readonly syntaxLayer = new GrSyntaxLayerWorker(
+ resolve(this, highlightServiceToken),
+ () => getAppContext().reportingService
+ );
+
constructor() {
super();
subscribe(
this,
() => this.getUserModel().preferences$,
preferences => {
+ const layers: DiffLayer[] = [this.syntaxLayer];
if (!preferences?.disable_token_highlighting) {
- this.layers = [new TokenHighlightLayer(this)];
+ layers.push(new TokenHighlightLayer(this));
}
+ this.layers = layers;
}
);
subscribe(
@@ -111,6 +121,7 @@
diffPreferences => {
if (!diffPreferences) return;
this.diffPrefs = diffPreferences;
+ this.syntaxLayer.setEnabled(!!this.diffPrefs.syntax_highlighting);
}
);
}
@@ -173,19 +184,25 @@
<div class="file-name">
<span>${item.filepath}</span>
</div>
- <div class="diffContainer">
- <gr-diff
- .prefs=${this.overridePartialDiffPrefs()}
- .path=${item.filepath}
- .diff=${item.preview}
- .layers=${this.layers}
- ></gr-diff>
- </div>
+ <div class="diffContainer">${this.renderDiff(item)}</div>
`
);
return html`<div slot="main">${items}</div>`;
}
+ private renderDiff(preview: FilePreview) {
+ const diff = preview.preview;
+ if (!anyLineTooLong(diff)) {
+ this.syntaxLayer.process(diff);
+ }
+ return html`<gr-diff
+ .prefs=${this.overridePartialDiffPrefs()}
+ .path=${preview.filepath}
+ .diff=${diff}
+ .layers=${this.layers}
+ ></gr-diff>`;
+ }
+
private renderFooter() {
const id = this.selectedFixIdx;
const fixCount = this.fixSuggestions?.length ?? 0;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index f0fbd64..eb2b494 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -94,6 +94,7 @@
debounceP,
DelayedPromise,
DELAYED_CANCELLATION,
+ noAwait,
} from '../../../utils/async-util';
import {subscribe} from '../../lit/subscription-controller';
import {GeneratedWebLink} from '../../../utils/weblink-util';
@@ -554,17 +555,16 @@
async initLayers() {
const preferencesPromise = this.restApiService.getPreferences();
- await this.getPluginLoader().awaitPluginsLoaded();
const prefs = await preferencesPromise;
const enableTokenHighlight = !prefs?.disable_token_highlighting;
assertIsDefined(this.path, 'path');
- this.layers = this.getLayers(this.path, enableTokenHighlight);
+ this.layers = this.getLayers(enableTokenHighlight);
this.coverageRanges = [];
// We kick off fetching the data here, but we don't return the promise,
// so awaiting initLayers() will not wait for coverage data to be
// completely loaded.
- this.getCoverageData();
+ noAwait(this.getCoverageData());
}
/**
@@ -711,20 +711,16 @@
};
}
- private getLayers(path: string, enableTokenHighlight: boolean): DiffLayer[] {
+ private getLayers(enableTokenHighlight: boolean): DiffLayer[] {
const layers = [];
if (enableTokenHighlight) {
layers.push(new TokenHighlightLayer(this));
}
layers.push(this.syntaxLayer);
- // Get layers from plugins (if any).
- layers.push(...this.getPluginLoader().jsApiService.getDiffLayers(path));
return layers;
}
clear() {
- if (this.path)
- this.getPluginLoader().jsApiService.disposeDiffLayers(this.path);
this.layers = [];
}
@@ -839,7 +835,7 @@
return el;
}
- private getCoverageData() {
+ private async getCoverageData() {
assertIsDefined(this.changeNum, 'changeNum');
assertIsDefined(this.change, 'change');
assertIsDefined(this.path, 'path');
@@ -854,58 +850,39 @@
const basePatchNum = toNumberOnly(this.patchRange.basePatchNum);
const patchNum = toNumberOnly(this.patchRange.patchNum);
- this.getPluginLoader()
- .jsApiService.getCoverageAnnotationApis()
- .then(coverageAnnotationApis => {
- coverageAnnotationApis.forEach(coverageAnnotationApi => {
- const provider = coverageAnnotationApi.getCoverageProvider();
- if (!provider) return;
- provider(changeNum, path, basePatchNum, patchNum, change)
- .then(coverageRanges => {
- assertIsDefined(this.patchRange, 'patchRange');
- if (
- !coverageRanges ||
- changeNum !== this.changeNum ||
- change !== this.change ||
- path !== this.path ||
- basePatchNum !== toNumberOnly(this.patchRange.basePatchNum) ||
- patchNum !== toNumberOnly(this.patchRange.patchNum)
- ) {
- return;
- }
-
- const existingCoverageRanges = this.coverageRanges;
- this.coverageRanges = coverageRanges;
-
- // Notify with existing coverage ranges in case there is some
- // existing coverage data that needs to be removed
- existingCoverageRanges.forEach(range => {
- coverageAnnotationApi.notify(
- path,
- range.code_range.start_line,
- range.code_range.end_line,
- range.side
- );
- });
-
- // Notify with new coverage data
- coverageRanges.forEach(range => {
- coverageAnnotationApi.notify(
- path,
- range.code_range.start_line,
- range.code_range.end_line,
- range.side
- );
- });
- })
- .catch(err => {
- this.reporting.error('GrDiffHost Coverage', err);
- });
- });
- })
- .catch(err => {
- this.reporting.error('GrDiffHost Coverage', err);
- });
+ // We are simply waiting here for all plugins to be loaded. Ideally we would
+ // just react to state changes, but plugins are loaded quickly once at app
+ // startup, and coordinating incoming coverage providers with the reloading
+ // process seems to be complex enough to avoid it for the time being.
+ await this.getPluginLoader().awaitPluginsLoaded();
+ const plugins =
+ this.getPluginLoader().pluginsModel.getState().coveragePlugins;
+ const providers = plugins.map(p => p.provider);
+ for (const provider of providers) {
+ try {
+ const coverageRanges = await provider(
+ changeNum,
+ path,
+ basePatchNum,
+ patchNum,
+ change
+ );
+ assertIsDefined(this.patchRange, 'patchRange');
+ if (
+ !coverageRanges ||
+ changeNum !== this.changeNum ||
+ change !== this.change ||
+ path !== this.path ||
+ basePatchNum !== toNumberOnly(this.patchRange.basePatchNum) ||
+ patchNum !== toNumberOnly(this.patchRange.patchNum)
+ ) {
+ continue;
+ }
+ this.coverageRanges = coverageRanges;
+ } catch (e) {
+ if (e instanceof Error) this.reporting.error('GrDiffHost Coverage', e);
+ }
+ }
}
private computeFileThreads(
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
index 14c2e00..be03afd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
@@ -50,7 +50,6 @@
import {RunResult} from '../../../models/checks/checks-model';
import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread';
import {assertIsDefined} from '../../../utils/common-util';
-import {GrAnnotationActionsInterface} from '../../shared/gr-js-api-interface/gr-annotation-actions-js-api';
import {fixture, html, assert} from '@open-wc/testing';
import {EventType} from '../../../types/events';
import {testResolver} from '../../../test/common-test-setup';
@@ -77,28 +76,6 @@
userModel = testResolver(userModelToken);
});
- suite('plugin layers', () => {
- let getDiffLayersStub: sinon.SinonStub;
- const pluginLayers = [{annotate: () => {}}, {annotate: () => {}}];
- setup(async () => {
- element = await fixture(html`<gr-diff-host></gr-diff-host>`);
- getDiffLayersStub = sinon
- .stub(testResolver(pluginLoaderToken).jsApiService, 'getDiffLayers')
- .returns(pluginLayers);
- element.changeNum = 123 as NumericChangeId;
- element.change = createChange();
- element.patchRange = createPatchRange();
- element.path = 'some/path';
- await element.updateComplete;
- });
-
- test('plugin layers requested', async () => {
- getDiffRestApiStub.returns(Promise.resolve(createDiff()));
- await element.reload();
- assert(getDiffLayersStub.called);
- });
- });
-
suite('render reporting', () => {
test('ends total and syntax timer after syntax layer', async () => {
const displayedStub = stubReporting('diffViewContentDisplayed');
@@ -1584,9 +1561,7 @@
});
suite('coverage layer', () => {
- let notifyStub: SinonStub;
let coverageProviderStub: SinonStub;
- let getCoverageAnnotationApisStub: SinonStub;
const exampleRanges = [
{
type: CoverageType.COVERED,
@@ -1607,7 +1582,6 @@
];
setup(async () => {
- notifyStub = sinon.stub();
coverageProviderStub = sinon
.stub()
.returns(Promise.resolve(exampleRanges));
@@ -1632,40 +1606,13 @@
content: [{a: ['foo']}],
})
);
- getCoverageAnnotationApisStub = sinon
- .stub(
- testResolver(pluginLoaderToken).jsApiService,
- 'getCoverageAnnotationApis'
- )
- .returns(
- Promise.resolve([
- {
- notify: notifyStub,
- getCoverageProvider() {
- return coverageProviderStub;
- },
- } as unknown as GrAnnotationActionsInterface,
- ])
- );
+ testResolver(pluginLoaderToken).pluginsModel.coverageRegister({
+ pluginName: 'test-coverage-plugin',
+ provider: coverageProviderStub,
+ });
await element.reload();
});
- test('getCoverageAnnotationApis should be called', async () => {
- await element.waitForReloadToRender();
- assert.isTrue(getCoverageAnnotationApisStub.calledOnce);
- });
-
- test('coverageRangeChanged should be called', async () => {
- await element.waitForReloadToRender();
- assert.equal(notifyStub.callCount, 2);
- assert.isTrue(
- notifyStub.calledWithExactly('some/path', 1, 2, Side.RIGHT)
- );
- assert.isTrue(
- notifyStub.calledWithExactly('some/path', 3, 4, Side.RIGHT)
- );
- });
-
test('provider is called with appropriate params', async () => {
element.patchRange = createPatchRange(1, 3);
await element.updateComplete;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 7d3fe6f..36c9397 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -698,6 +698,7 @@
);
this.addEventListener('open-fix-preview', e => this.onOpenFixPreview(e));
this.cursor = new GrDiffCursor();
+ if (this.diffHost) this.reInitCursor();
}
override disconnectedCallback() {
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 2a5bcf5..c4533de 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -181,7 +181,7 @@
this.addEventListener(EventType.DIALOG_CHANGE, e => {
this.handleDialogChange(e as CustomEvent<DialogChangeEventDetail>);
});
- this.addEventListener(EventType.LOCATION_CHANGE, () =>
+ document.addEventListener(EventType.LOCATION_CHANGE, () =>
this.handleLocationChange()
);
this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
diff --git a/polygerrit-ui/app/elements/gr-app_test.ts b/polygerrit-ui/app/elements/gr-app_test.ts
index 730548c..05ee9ed 100644
--- a/polygerrit-ui/app/elements/gr-app_test.ts
+++ b/polygerrit-ui/app/elements/gr-app_test.ts
@@ -20,6 +20,26 @@
import {resolve} from '../models/dependency';
import {removeRequestDependencyListener} from '../test/common-test-setup';
+suite('gr-app callback tests', () => {
+ const handleLocationChangeSpy = sinon.spy(
+ GrAppElement.prototype,
+ <any>'handleLocationChange'
+ );
+ const dispatchLocationChangeEventSpy = sinon.spy(
+ GrRouter.prototype,
+ <any>'dispatchLocationChangeEvent'
+ );
+
+ setup(async () => {
+ await fixture<GrApp>(html`<gr-app id="app"></gr-app>`);
+ });
+
+ test("handleLocationChange in gr-app-element is called after dispatching 'location-change' event in gr-router", () => {
+ dispatchLocationChangeEventSpy();
+ assert.isTrue(handleLocationChangeSpy.calledOnce);
+ });
+});
+
suite('gr-app tests', () => {
let grApp: GrApp;
const config = createServerInfo();
diff --git a/polygerrit-ui/app/elements/gr-css-mixins.ts b/polygerrit-ui/app/elements/gr-css-mixins.ts
index 733b386..523a056 100644
--- a/polygerrit-ui/app/elements/gr-css-mixins.ts
+++ b/polygerrit-ui/app/elements/gr-css-mixins.ts
@@ -77,6 +77,10 @@
--paper-listbox: {
padding: 0;
};
+ --iron-autogrow-textarea: {
+ box-sizing: border-box;
+ padding: var(--spacing-s);
+ };
}
</style>
`;
diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
index f63f34e..1549da5 100644
--- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
+++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
@@ -3,6 +3,7 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
+import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '@polymer/iron-input/iron-input';
import '../../shared/gr-avatar/gr-avatar';
import '../../shared/gr-date-formatter/gr-date-formatter';
@@ -87,6 +88,13 @@
border-radius: var(--border-radius);
box-shadow: var(--elevation-level-5);
}
+ iron-autogrow-textarea {
+ background-color: var(--view-background-color);
+ color: var(--primary-text-color);
+ }
+ .lengthCounter {
+ font-weight: var(--font-weight-normal);
+ }
`,
];
@@ -200,25 +208,26 @@
</span>
</section>
<section>
- <label class="title" for="statusInput">About me (e.g. employer)</label>
+ <span class="title">
+ <label for="statusInput">About me (e.g. employer)</label>
+ <div class="lengthCounter">
+ ${this.account.status?.length ?? 0}/140
+ </div>
+ </span>
<span class="value">
- <iron-input
- id="statusIronInput"
- @keydown=${this.handleKeydown}
- .bindValue=${this.account?.status}
+ <iron-autogrow-textarea
+ id="statusInput"
+ .name=${'statusInput'}
+ ?disabled=${this.saving}
+ maxlength="140"
+ .value=${this.account?.status}
@bind-value-changed=${(e: BindValueChangeEvent) => {
const oldAccount = this.account;
if (!oldAccount || oldAccount.status === e.detail.value) return;
this.account = {...oldAccount, status: e.detail.value};
this.hasStatusChange = true;
}}
- >
- <input
- id="statusInput"
- ?disabled=${this.saving}
- @keydown=${this.handleKeydown}
- />
- </iron-input>
+ ></iron-autogrow-textarea>
</span>
</section>
<section>
diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
index f7e5dee..e968b12 100644
--- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
+++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
@@ -5,7 +5,12 @@
*/
import '../../../test/common-test-setup';
import './gr-account-info';
-import {query, queryAll, stubRestApi} from '../../../test/test-utils';
+import {
+ query,
+ queryAll,
+ queryAndAssert,
+ stubRestApi,
+} from '../../../test/test-utils';
import {GrAccountInfo} from './gr-account-info';
import {AccountDetailInfo, ServerInfo} from '../../../types/common';
import {
@@ -20,6 +25,7 @@
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {EditableAccountField} from '../../../api/rest-api';
import {fixture, html, assert} from '@open-wc/testing';
+import {IronAutogrowTextareaElement} from '@polymer/iron-autogrow-textarea';
suite('gr-account-info tests', () => {
let element!: GrAccountInfo;
@@ -100,13 +106,16 @@
</span>
</section>
<section>
- <label class="title" for="statusInput">
- About me (e.g. employer)
- </label>
+ <span class="title">
+ <label for="statusInput">About me (e.g. employer)</label>
+ <div class="lengthCounter">0/140</div>
+ </span>
<span class="value">
- <iron-input id="statusIronInput">
- <input id="statusInput" />
- </iron-input>
+ <iron-autogrow-textarea
+ aria-disabled="false"
+ id="statusInput"
+ maxlength="140"
+ />
</span>
</section>
<section>
@@ -276,8 +285,11 @@
test('status', async () => {
assert.isFalse(element.hasUnsavedChanges);
- const statusInputEl = queryIronInput('#statusIronInput');
- statusInputEl.bindValue = 'new status';
+ const statusTextarea = queryAndAssert<IronAutogrowTextareaElement>(
+ element,
+ '#statusInput'
+ );
+ statusTextarea.value = 'new status';
await element.updateComplete;
assert.isFalse(element.hasNameChange);
assert.isTrue(element.hasStatusChange);
@@ -320,8 +332,11 @@
await element.updateComplete;
assert.isTrue(element.hasNameChange);
- const statusInputEl = queryIronInput('#statusIronInput');
- statusInputEl.bindValue = 'new status';
+ const statusTextarea = queryAndAssert<IronAutogrowTextareaElement>(
+ element,
+ '#statusInput'
+ );
+ statusTextarea.value = 'new status';
await element.updateComplete;
assert.isTrue(element.hasStatusChange);
@@ -366,8 +381,11 @@
assert.equal(displaySpan.textContent, account.name);
assert.isUndefined(inputSpan);
- const inputEl = queryIronInput('#statusIronInput');
- inputEl.bindValue = 'new status';
+ const statusTextarea = queryAndAssert<IronAutogrowTextareaElement>(
+ element,
+ '#statusInput'
+ );
+ statusTextarea.value = 'new status';
await element.updateComplete;
assert.isTrue(element.hasStatusChange);
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 3fd1b82..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
@@ -79,6 +79,11 @@
css`
:host {
z-index: 100;
+ box-shadow: var(--elevation-level-2);
+ overflow: auto;
+ background: var(--dropdown-background-color);
+ border-radius: var(--border-radius);
+ max-height: 50vh;
}
:host([is-hidden]) {
display: none;
@@ -105,13 +110,6 @@
li.selected {
background-color: var(--hover-background-color);
}
- .dropdown-content {
- background: var(--dropdown-background-color);
- box-shadow: var(--elevation-level-2);
- border-radius: var(--border-radius);
- max-height: 50vh;
- overflow: auto;
- }
@media only screen and (max-height: 35em) {
.dropdown-content {
max-height: 80vh;
@@ -167,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,
@@ -211,7 +204,7 @@
}
setPositionTarget(target: HTMLElement) {
- this.fitController?.setPositionTarget(target);
+ this.fitController.setPositionTarget(target);
}
private handleUp() {
@@ -302,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/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index d9004ae..5306cea 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -3,7 +3,6 @@
* Copyright 2015 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '../../../styles/shared-styles';
import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
import '../../plugins/gr-endpoint-param/gr-endpoint-param';
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
index dc08648..abc7f3c 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
@@ -157,6 +157,9 @@
.reason {
padding-top: var(--spacing-s);
}
+ .status .value {
+ white-space: pre-wrap;
+ }
`,
];
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
index 7ab4689..4b5913c 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
@@ -3,132 +3,25 @@
* Copyright 2017 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {DiffLayer, DiffLayerListener} from '../../../types/types';
-import {Side} from '../../../constants/constants';
-import {EventType, PluginApi} from '../../../api/plugin';
-import {getAppContext} from '../../../services/app-context';
import {AnnotationPluginApi, CoverageProvider} from '../../../api/annotation';
+import {PluginApi} from '../../../api/plugin';
+import {PluginsModel} from '../../../models/plugins/plugins-model';
+import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
export class GrAnnotationActionsInterface implements AnnotationPluginApi {
- /**
- * Collect all annotation layers instantiated by createLayer. This is only
- * used for being able to look up the appropriate layer when notify() is
- * being called by plugins.
- */
- private annotationLayers: AnnotationLayer[] = [];
-
- private coverageProvider?: CoverageProvider;
-
- private readonly reporting = getAppContext().reportingService;
-
- constructor(private readonly plugin: PluginApi) {
+ constructor(
+ private readonly reporting: ReportingService,
+ private readonly pluginsModel: PluginsModel,
+ private readonly plugin: PluginApi
+ ) {
this.reporting.trackApi(this.plugin, 'annotation', 'constructor');
- plugin.on(EventType.ANNOTATE_DIFF, this);
}
- setCoverageProvider(
- coverageProvider: CoverageProvider
- ): GrAnnotationActionsInterface {
+ setCoverageProvider(provider: CoverageProvider) {
this.reporting.trackApi(this.plugin, 'annotation', 'setCoverageProvider');
- if (this.coverageProvider) {
- this.reporting.error(
- 'Annotation Plugin',
- new Error(
- `Overwriting coverage provider: ${this.plugin.getPluginName()}`
- )
- );
- }
- this.coverageProvider = coverageProvider;
- return this;
- }
-
- /**
- * Used by Gerrit to look up the coverage provider. Not intended to be called
- * by plugins.
- */
- getCoverageProvider() {
- return this.coverageProvider;
- }
-
- notify(path: string, start: number, end: number, side: Side) {
- this.reporting.trackApi(this.plugin, 'annotation', 'notify');
- for (const annotationLayer of this.annotationLayers) {
- // Notify only the annotation layer that is associated with the specified
- // path.
- if (annotationLayer.path === path) {
- annotationLayer.notifyListeners(start, end, side);
- }
- }
- }
-
- /**
- * Factory method called by Gerrit for creating a DiffLayer for each diff that
- * is rendered.
- *
- * Don't forget to also call disposeLayer().
- */
- createLayer(path: string) {
- const annotationLayer = new AnnotationLayer(path);
- this.annotationLayers.push(annotationLayer);
- return annotationLayer;
- }
-
- /**
- * Called by Gerrit for each diff renderer that had called createLayer().
- */
- disposeLayer(path: string) {
- this.annotationLayers = this.annotationLayers.filter(
- annotationLayer => annotationLayer.path !== path
- );
- }
-}
-
-/**
- * An AnnotationLayer exists for each file that is being rendered. This class is
- * not exposed to plugins, but being used by Gerrit's diff rendering.
- */
-export class AnnotationLayer implements DiffLayer {
- private listeners: DiffLayerListener[] = [];
-
- /**
- * Used to create an instance of the Annotation Layer interface.
- *
- * @param path The file path (eg: /COMMIT_MSG').
- */
- constructor(readonly path: string) {
- this.listeners = [];
- }
-
- /**
- * Register a listener for layer updates.
- * Don't forget to removeListener when you stop using layer.
- *
- * @param fn The update handler function.
- * Should accept as arguments the line numbers for the start and end of
- * the update and the side as a string.
- */
- addListener(listener: DiffLayerListener) {
- this.listeners.push(listener);
- }
-
- removeListener(listener: DiffLayerListener) {
- this.listeners = this.listeners.filter(f => f !== listener);
- }
-
- annotate() {}
-
- /**
- * Notify layer listeners (which typically is just Gerrit's diff renderer) of
- * changes to annotations after the diff rendering had already completed. This
- * is indirectly called by plugins using the AnnotationPluginApi.notify().
- *
- * @param start The line where the update starts.
- * @param end The line where the update ends.
- * @param side The side of the update. ('left' or 'right')
- */
- notifyListeners(start: number, end: number, side: Side) {
- for (const listener of this.listeners) {
- listener(start, end, side);
- }
+ this.pluginsModel.coverageRegister({
+ pluginName: this.plugin.getPluginName(),
+ provider,
+ });
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.ts
deleted file mode 100644
index 6e63b0d..0000000
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.ts
+++ /dev/null
@@ -1,86 +0,0 @@
-/**
- * @license
- * Copyright 2016 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import '../../../test/common-test-setup';
-import '../../change/gr-change-actions/gr-change-actions';
-import {assert} from '@open-wc/testing';
-import {PluginApi} from '../../../api/plugin';
-import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api';
-import {Side} from '../../../constants/constants';
-import {DiffLayerListener} from '../../../types/types';
-
-suite('gr-annotation-actions-js-api tests', () => {
- let annotationActions: GrAnnotationActionsInterface;
- let plugin: PluginApi;
-
- setup(() => {
- window.Gerrit.install(
- p => {
- plugin = p;
- },
- '0.1',
- 'http://test.com/plugins/testplugin/static/test.js'
- );
- // The object is exposed as type AnnotationPluginApi, but the tests need the
- // implementation class to arrange test setups using `.createLayer()`.
- annotationActions = plugin.annotationApi() as GrAnnotationActionsInterface;
- });
-
- test('add notifier', () => {
- const path1 = '/dummy/path1';
- const path2 = '/dummy/path2';
- const annotationLayer1 = annotationActions.createLayer(path1);
- const annotationLayer2 = annotationActions.createLayer(path2);
- const layer1Spy = sinon.spy(annotationLayer1, 'notifyListeners');
- const layer2Spy = sinon.spy(annotationLayer2, 'notifyListeners');
-
- // Assert that no layers are invoked with a different path.
- annotationActions.notify('/dummy/path3', 0, 10, Side.RIGHT);
- assert.isFalse(layer1Spy.called);
- assert.isFalse(layer2Spy.called);
-
- // Assert that only the 1st layer is invoked with path1.
- annotationActions.notify(path1, 0, 10, Side.RIGHT);
- assert.isTrue(layer1Spy.called);
- assert.isFalse(layer2Spy.called);
-
- // Reset spies.
- layer1Spy.resetHistory();
- layer2Spy.resetHistory();
-
- // Assert that only the 2nd layer is invoked with path2.
- annotationActions.notify(path2, 0, 20, Side.LEFT);
- assert.isFalse(layer1Spy.called);
- assert.isTrue(layer2Spy.called);
- });
-
- test('layer notify listeners', () => {
- const annotationLayer = annotationActions.createLayer('/dummy/path');
- let listenerCalledTimes = 0;
- const startRange = 10;
- const endRange = 20;
- const side = Side.RIGHT;
- const listener: DiffLayerListener = (st, end, s) => {
- listenerCalledTimes++;
- assert.equal(st, startRange);
- assert.equal(end, endRange);
- assert.equal(s, side);
- };
-
- // Notify with 0 listeners added.
- annotationLayer.notifyListeners(startRange, endRange, side);
- assert.equal(listenerCalledTimes, 0);
-
- // Add 1 listener.
- annotationLayer.addListener(listener);
- annotationLayer.notifyListeners(startRange, endRange, side);
- assert.equal(listenerCalledTimes, 1);
-
- // Add 1 more listener. Total 2 listeners.
- annotationLayer.addListener(listener);
- annotationLayer.notifyListeners(startRange, endRange, side);
- assert.equal(listenerCalledTimes, 3);
- });
-});
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index 9f2301d..a16a1d1 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -10,7 +10,6 @@
ReviewInput,
RevisionInfo,
} from '../../../types/common';
-import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api';
import {GrAdminApi} from '../../plugins/gr-admin-api/gr-admin-api';
import {
JsApiService,
@@ -19,7 +18,7 @@
ShowRevisionActionsDetail,
} from './gr-js-api-types';
import {EventType, TargetElement} from '../../../api/plugin';
-import {DiffLayer, ParsedChangeInfo} from '../../../types/types';
+import {ParsedChangeInfo} from '../../../types/types';
import {MenuLink} from '../../../api/admin';
import {Finalizable} from '../../../services/registry';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
@@ -207,58 +206,6 @@
return revertSubmissionMsg;
}
- getDiffLayers(path: string) {
- const layers: DiffLayer[] = [];
- for (const cb of this._getEventCallbacks(EventType.ANNOTATE_DIFF)) {
- const annotationApi = cb as unknown as GrAnnotationActionsInterface;
- try {
- const layer = annotationApi.createLayer(path);
- if (layer) layers.push(layer);
- } catch (err: unknown) {
- this.reporting.error(
- 'GrJsApiInterface',
- new Error('getDiffLayers callback error'),
- err
- );
- }
- }
- return layers;
- }
-
- disposeDiffLayers(path: string) {
- for (const cb of this._getEventCallbacks(EventType.ANNOTATE_DIFF)) {
- try {
- const annotationApi = cb as unknown as GrAnnotationActionsInterface;
- annotationApi.disposeLayer(path);
- } catch (err: unknown) {
- this.reporting.error(
- 'GrJsApiInterface',
- new Error('disposeDiffLayers callback error'),
- err
- );
- }
- }
- }
-
- /**
- * Retrieves coverage data possibly provided by a plugin.
- *
- * Will wait for plugins to be loaded. If multiple plugins offer a coverage
- * provider, the first one is returned. If no plugin offers a coverage provider,
- * will resolve to null.
- */
- getCoverageAnnotationApis(): Promise<GrAnnotationActionsInterface[]> {
- return this.waitForPluginsToLoad().then(() => {
- const providers: GrAnnotationActionsInterface[] = [];
- this._getEventCallbacks(EventType.ANNOTATE_DIFF).forEach(cb => {
- const annotationApi = cb as unknown as GrAnnotationActionsInterface;
- const provider = annotationApi.getCoverageProvider();
- if (provider) providers.push(annotationApi);
- });
- return providers;
- });
- }
-
getAdminMenuLinks(): MenuLink[] {
const links: MenuLink[] = [];
for (const cb of this._getEventCallbacks(EventType.ADMIN_MENU_LINKS)) {
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
index c7a5eae..a10beab 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
@@ -12,8 +12,7 @@
} from '../../../types/common';
import {Finalizable} from '../../../services/registry';
import {EventType, TargetElement} from '../../../api/plugin';
-import {DiffLayer, ParsedChangeInfo} from '../../../types/types';
-import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api';
+import {ParsedChangeInfo} from '../../../types/types';
import {MenuLink} from '../../../api/admin';
export interface ShowChangeDetail {
@@ -47,9 +46,6 @@
origMsg: string
): string;
addElement(key: TargetElement, el: HTMLElement): void;
- getDiffLayers(path: string): DiffLayer[];
- disposeDiffLayers(path: string): void;
- getCoverageAnnotationApis(): Promise<GrAnnotationActionsInterface[]>;
getAdminMenuLinks(): MenuLink[];
handleCommitMessage(change: ChangeInfo | ParsedChangeInfo, msg: string): void;
canSubmitChange(change: ChangeInfo, revision?: RevisionInfo | null): boolean;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
index 6e638bd..2ed3794 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
@@ -215,7 +215,11 @@
}
annotationApi(): AnnotationPluginApi {
- return new GrAnnotationActionsInterface(this);
+ return new GrAnnotationActionsInterface(
+ this.report,
+ this.pluginsModel,
+ this
+ );
}
changeActions(): ChangeActionsPluginApi {
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index b38d71b..169cd59 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -396,10 +396,11 @@
}
}
- private setValue(text: string) {
+ private async setValue(text: string) {
if (this.specialCharIndex === -1) {
return;
}
+ const specialCharIndex = this.specialCharIndex;
if (this.isEmojiDropdownActive()) {
this.text = this.addValueToText(text);
this.reporting.reportInteraction('select-emoji', {type: text});
@@ -407,9 +408,13 @@
this.text = this.addValueToText('@' + text);
this.reporting.reportInteraction('select-mention', {type: text});
}
-
- this.textarea!.selectionStart = this.specialCharIndex + 1;
- this.textarea!.selectionEnd = this.specialCharIndex + 1;
+ // iron-autogrow-textarea unfortunately sets the cursor at the end when
+ // it's value is changed, which means the setting of selectionStart
+ // below needs to happen after iron-autogrow-textarea has set the
+ // incorrect value.
+ await this.updateComplete;
+ this.textarea!.selectionStart = specialCharIndex + 1;
+ this.textarea!.selectionEnd = specialCharIndex + 1;
this.resetDropdown();
}
diff --git a/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer.ts b/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer.ts
index dae5c03..859a49d 100644
--- a/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer.ts
+++ b/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer.ts
@@ -4,7 +4,12 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {Side} from '../../../api/diff';
-import {CoverageRange, CoverageType, DiffLayer} from '../../../types/types';
+import {
+ CoverageRange,
+ CoverageType,
+ DiffLayer,
+ DiffLayerListener,
+} from '../../../types/types';
const TOOLTIP_MAP = new Map([
[CoverageType.COVERED, 'Covered by tests.'],
@@ -13,6 +18,31 @@
[CoverageType.NOT_INSTRUMENTED, 'Not instrumented by any tests.'],
]);
+// Ranges are considered half-open: [start, end)
+export type Range = {start: number; end: number};
+
+export function mergeRanges(ranges: Range[]): Range[] {
+ ranges.sort((a, b) => a.start - b.start);
+
+ if (ranges.length <= 1) {
+ return ranges;
+ }
+
+ const stack: Range[] = [];
+ stack.push(ranges[0]);
+
+ for (let j = 1; j < ranges.length; j++) {
+ const interval = ranges[j];
+ const top = stack[stack.length - 1];
+ if (top.end < interval.start) {
+ stack.push(interval);
+ } else if (top.end < interval.end) {
+ top.end = interval.end;
+ }
+ }
+ return stack;
+}
+
export class GrCoverageLayer implements DiffLayer {
/**
* Must be sorted by code_range.start_line.
@@ -35,14 +65,56 @@
*/
private index = 0;
+ /**
+ * Has any line been annotated already in the lifetime of this layer?
+ * If not, then `setRanges()` does not have to call `notify()` and thus
+ * trigger re-rendering of the affected diff rows.
+ */
+ // visible for testing
+ annotated = false;
+
+ private listeners: DiffLayerListener[] = [];
+
constructor(private readonly side: Side) {}
+ addListener(listener: DiffLayerListener) {
+ this.listeners.push(listener);
+ }
+
+ removeListener(listener: DiffLayerListener) {
+ this.listeners = this.listeners.filter(f => f !== listener);
+ }
+
/**
* Must be sorted by code_range.start_line.
* Must only contain ranges that match the side.
*/
setRanges(ranges: CoverageRange[]) {
+ const oldRanges = this.coverageRanges;
+ if (oldRanges.length === 0 && ranges.length === 0) return;
this.coverageRanges = ranges;
+
+ // If ranges are set before any diff row was rendered, then great, no need
+ // to notify and re-render.
+ if (this.annotated) this.notify([...oldRanges, ...ranges]);
+ }
+
+ /**
+ * Notify listeners (should be just gr-diff triggering a re-render).
+ *
+ * We are optimizing the notification calls by converting the coverange ranges
+ * to an array of [start, end) ranges and then merging them to non-overlapping
+ * set of ranges.
+ */
+ private notify(ranges: CoverageRange[]) {
+ const notifyRanges = mergeRanges(
+ ranges.map(r => {
+ return {start: r.code_range.start_line, end: r.code_range.end_line + 1};
+ })
+ );
+ for (const r of notifyRanges) {
+ for (const l of this.listeners) l(r.start, r.end - 1, this.side);
+ }
}
/**
@@ -74,6 +146,7 @@
this.index = 0;
}
this.lastLineNumber = elementLineNumber;
+ this.annotated = true;
// We simply loop through all the coverage ranges until we find one that
// matches the line number.
diff --git a/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer_test.ts b/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer_test.ts
index c1a123e..a8cdff6 100644
--- a/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-coverage-layer/gr-coverage-layer_test.ts
@@ -4,51 +4,108 @@
* SPDX-License-Identifier: Apache-2.0
*/
import '../../../test/common-test-setup';
-import {CoverageRange, CoverageType, Side} from '../../../api/diff';
-import {GrCoverageLayer} from './gr-coverage-layer';
+import {CoverageType, Side} from '../../../api/diff';
+import {GrCoverageLayer, mergeRanges} from './gr-coverage-layer';
import {assert} from '@open-wc/testing';
+import {SinonStub} from 'sinon';
+
+const RANGES = [
+ {
+ type: CoverageType.COVERED,
+ side: Side.RIGHT,
+ code_range: {
+ start_line: 1,
+ end_line: 2,
+ },
+ },
+ {
+ type: CoverageType.NOT_COVERED,
+ side: Side.RIGHT,
+ code_range: {
+ start_line: 3,
+ end_line: 4,
+ },
+ },
+ {
+ type: CoverageType.PARTIALLY_COVERED,
+ side: Side.RIGHT,
+ code_range: {
+ start_line: 5,
+ end_line: 6,
+ },
+ },
+ {
+ type: CoverageType.NOT_INSTRUMENTED,
+ side: Side.RIGHT,
+ code_range: {
+ start_line: 8,
+ end_line: 9,
+ },
+ },
+];
suite('gr-coverage-layer', () => {
let layer: GrCoverageLayer;
- setup(() => {
- const initialCoverageRanges: CoverageRange[] = [
- {
- type: CoverageType.COVERED,
- side: Side.RIGHT,
- code_range: {
- start_line: 1,
- end_line: 2,
- },
- },
- {
- type: CoverageType.NOT_COVERED,
- side: Side.RIGHT,
- code_range: {
- start_line: 3,
- end_line: 4,
- },
- },
- {
- type: CoverageType.PARTIALLY_COVERED,
- side: Side.RIGHT,
- code_range: {
- start_line: 5,
- end_line: 6,
- },
- },
- {
- type: CoverageType.NOT_INSTRUMENTED,
- side: Side.RIGHT,
- code_range: {
- start_line: 8,
- end_line: 9,
- },
- },
- ];
+ test('mergeRanges', () => {
+ assert.deepEqual(mergeRanges([]), []);
+ assert.deepEqual(mergeRanges([{start: 1, end: 2}]), [{start: 1, end: 2}]);
+ assert.deepEqual(
+ mergeRanges([
+ {start: 1, end: 2},
+ {start: 2, end: 3},
+ ]),
+ [{start: 1, end: 3}]
+ );
+ assert.deepEqual(
+ mergeRanges([
+ {start: 2, end: 3},
+ {start: 1, end: 2},
+ ]),
+ [{start: 1, end: 3}]
+ );
+ assert.deepEqual(
+ mergeRanges([
+ {start: 1, end: 3},
+ {start: 4, end: 5},
+ ]),
+ [
+ {start: 1, end: 3},
+ {start: 4, end: 5},
+ ]
+ );
+ });
- layer = new GrCoverageLayer(Side.RIGHT);
- layer.setRanges(initialCoverageRanges);
+ suite('setRanges and notify', () => {
+ let listener: SinonStub;
+
+ setup(() => {
+ layer = new GrCoverageLayer(Side.RIGHT);
+ listener = sinon.stub();
+ layer.addListener(listener);
+ });
+
+ test('empty ranges do not notify', () => {
+ layer.annotated = true;
+ layer.setRanges([]);
+ assert.isFalse(listener.called);
+ });
+
+ test('do not notify while annotated is false', () => {
+ layer.setRanges(RANGES);
+ assert.isFalse(listener.called);
+ });
+
+ test('RANGES', () => {
+ layer.annotated = true;
+ layer.setRanges(RANGES);
+ assert.isTrue(listener.called);
+ assert.equal(listener.callCount, 2);
+ assert.equal(listener.getCall(0).args[0], 1);
+ assert.equal(listener.getCall(0).args[1], 6);
+ assert.equal(listener.getCall(1).args[0], 8);
+ assert.equal(listener.getCall(1).args[1], 9);
+ });
});
suite('annotate', () => {
@@ -73,6 +130,11 @@
assert.isTrue(contains);
}
+ setup(() => {
+ layer = new GrCoverageLayer(Side.RIGHT);
+ layer.setRanges(RANGES);
+ });
+
test('line 1-2 are covered', () => {
checkLine(1, 'COVERED');
checkLine(2, 'COVERED');
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 92396b0..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
@@ -169,6 +174,9 @@
}
render(keyLocations: KeyLocations): Promise<void> {
+ assertIsDefined(this.diff, 'diff');
+ assertIsDefined(this.diffElement, 'diff table');
+
// Setting up annotation layers must happen after plugins are
// installed, and |render| satisfies the requirement, however,
// |attached| doesn't because in the diff view page, the element is
@@ -178,22 +186,13 @@
this.showTabs = this.prefs.show_tabs;
this.showTrailingWhitespace = this.prefs.show_whitespace_errors;
- // Stop the processor if it's running.
- this.cancel();
-
- this.builder?.clear();
- assertIsDefined(this.diff, 'diff');
- assertIsDefined(this.diffElement, 'diff table');
+ this.cleanup();
this.builder = this.getDiffBuilder();
+ this.init();
this.processor.context = this.prefs.context;
this.processor.keyLocations = keyLocations;
- this.diffElement.addEventListener(
- 'diff-context-expanded',
- this.onDiffContextExpanded
- );
-
this.clearDiffContent();
this.builder.addColumns(
this.diffElement,
@@ -212,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');
@@ -370,9 +369,32 @@
});
}
- cancel() {
+ /**
+ * This is meant to be called when the gr-diff component re-connects, or when
+ * the diff is (re-)rendered.
+ *
+ * Make sure that this method is symmetric with cleanup(), which is called
+ * when gr-diff disconnects.
+ */
+ init() {
+ this.cleanup();
+ this.diffElement?.addEventListener(
+ 'diff-context-expanded',
+ this.onDiffContextExpanded
+ );
+ this.builder?.init();
+ }
+
+ /**
+ * This is meant to be called when the gr-diff component disconnects, or when
+ * the diff is (re-)rendered.
+ *
+ * Make sure that this method is symmetric with init(), which is called when
+ * gr-diff re-connects.
+ */
+ cleanup() {
this.processor.cancel();
- this.builder?.clear();
+ this.builder?.cleanup();
this.cancelableRenderPromise?.cancel();
this.cancelableRenderPromise = null;
this.diffElement?.removeEventListener(
@@ -501,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 2cfb895..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
@@ -71,7 +71,7 @@
const text = 'a'.repeat(51);
const expected = 'a'.repeat(50) + WBR_HTML + 'a';
const result = builder.createTextEl(null, line(text)).firstElementChild
- ?.innerHTML;
+ ?.firstElementChild?.innerHTML;
assert.equal(result, expected);
});
@@ -80,7 +80,7 @@
const text = 'a'.repeat(51);
const expected = 'a'.repeat(50) + LINE_BREAK_HTML + 'a';
const result = builder.createTextEl(null, line(text)).firstElementChild
- ?.innerHTML;
+ ?.firstElementChild?.innerHTML;
assert.equal(result, expected);
});
@@ -119,7 +119,7 @@
assert.equal(el.innerText, text);
// With line length 10 and tab size 4, there should be a line break
// after every two tabs.
- const newlineEl = el.querySelector('.contentText > .br');
+ const newlineEl = el.querySelector('.contentText .br');
assert.isOk(newlineEl);
assert.equal(
el.querySelector('.contentText .tab:nth-child(2)')?.nextSibling,
@@ -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);
@@ -648,7 +649,7 @@
test('cancel cancels the processor', () => {
const processorCancelStub = sinon.stub(element.processor, 'cancel');
- element.cancel();
+ element.cleanup();
assert.isTrue(processorCancelStub.called);
});
});
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 8176e14..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,8 +397,17 @@
}
td.appendChild(contentText);
- } else if (line.beforeNumber === 'FILE') td.classList.add('file');
- else if (line.beforeNumber === 'LOST') td.classList.add('lost');
+ }
+
+ if (side && lineNumber) {
+ const threadGroupEl = document.createElement('div');
+ threadGroupEl.className = 'thread-group';
+ threadGroupEl.setAttribute('data-side', side);
+ const slot = document.createElement('slot');
+ slot.name = `${side}-${lineNumber}`;
+ threadGroupEl.appendChild(slot);
+ td.appendChild(threadGroupEl);
+ }
return td;
}
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 0006f26..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
@@ -38,7 +38,8 @@
* of this class is. There are no plans for adding separate implementations.
*/
export interface DiffBuilder {
- clear(): void;
+ init(): void;
+ cleanup(): void;
addGroups(groups: readonly GrDiffGroup[]): void;
clearGroups(): void;
replaceGroup(
@@ -63,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.
*
@@ -81,7 +92,7 @@
// visible for testing
readonly _prefs: DiffPreferencesInfo;
- protected readonly renderPrefs?: RenderPreferences;
+ protected renderPrefs?: RenderPreferences;
protected readonly outputEl: HTMLElement;
@@ -127,6 +138,18 @@
end: LineNumber,
side: Side
) => this.renderContentByRange(start, end, side);
+ this.init();
+ }
+
+ /**
+ * This is meant to be called when the gr-diff component re-connects, or when
+ * the diff is (re-)rendered.
+ *
+ * Make sure that this method is symmetric with cleanup(), which is called
+ * when gr-diff disconnects.
+ */
+ init() {
+ this.cleanup();
for (const layer of this.layers) {
if (layer.addListener) {
layer.addListener(this.layerUpdateListener);
@@ -134,7 +157,14 @@
}
}
- clear() {
+ /**
+ * This is meant to be called when the gr-diff component disconnects, or when
+ * the diff is (re-)rendered.
+ *
+ * Make sure that this method is symmetric with init(), which is called when
+ * gr-diff re-connects.
+ */
+ cleanup() {
for (const layer of this.layers) {
if (layer.removeListener) {
layer.removeListener(this.layerUpdateListener);
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-utils.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
index 8a17611..f583c2e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
@@ -274,6 +274,12 @@
elementId: string
): HTMLElement {
const contentText = createElementDiff('div', 'contentText');
+ // <gr-legacy-text> is not defined anywhere, so this behave just as a <div>
+ // would. We use this during the migration to lit based diff elements to
+ // match <gr-diff-text>. We define a css rule with `display:contents` making
+ // sure that this extra element is basically a no-op.
+ const legacyText = document.createElement('gr-legacy-text');
+ contentText.appendChild(legacyText);
contentText.id = elementId;
let columnPos = 0;
let textOffset = 0;
@@ -285,16 +291,16 @@
let rowStart = 0;
let rowEnd = lineLimit - columnPos;
while (rowEnd < segment.length) {
- contentText.appendChild(
+ legacyText.appendChild(
document.createTextNode(segment.substring(rowStart, rowEnd))
);
- contentText.appendChild(createLineBreak(responsiveMode));
+ legacyText.appendChild(createLineBreak(responsiveMode));
columnPos = 0;
rowStart = rowEnd;
rowEnd += lineLimit;
}
// Append the last part of |segment|, which fits on the current line.
- contentText.appendChild(
+ legacyText.appendChild(
document.createTextNode(segment.substring(rowStart))
);
columnPos += segment.length - rowStart;
@@ -306,20 +312,20 @@
// Append a single '\t' character.
let effectiveTabSize = tabSize - (columnPos % tabSize);
if (columnPos + effectiveTabSize > lineLimit) {
- contentText.appendChild(createLineBreak(responsiveMode));
+ legacyText.appendChild(createLineBreak(responsiveMode));
columnPos = 0;
effectiveTabSize = tabSize;
}
- contentText.appendChild(createTabWrapper(effectiveTabSize));
+ legacyText.appendChild(createTabWrapper(effectiveTabSize));
columnPos += effectiveTabSize;
textOffset++;
} else {
// Append a single surrogate pair.
if (columnPos >= lineLimit) {
- contentText.appendChild(createLineBreak(responsiveMode));
+ legacyText.appendChild(createLineBreak(responsiveMode));
columnPos = 0;
}
- contentText.appendChild(
+ legacyText.appendChild(
document.createTextNode(text.substring(textOffset, textOffset + 2))
);
textOffset += 2;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
index 7e8eb4c..98b4586 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
@@ -20,10 +20,13 @@
test('formatText newlines 1', () => {
let text = 'abcdef';
- assert.equal(formatText(text, 'NONE', 4, 10, '').innerHTML, text);
+ assert.equal(
+ formatText(text, 'NONE', 4, 10, '').firstElementChild?.innerHTML,
+ text
+ );
text = 'a'.repeat(20);
assert.equal(
- formatText(text, 'NONE', 4, 10, '').innerHTML,
+ formatText(text, 'NONE', 4, 10, '').firstElementChild?.innerHTML,
'a'.repeat(10) + LINE_BREAK_HTML + 'a'.repeat(10)
);
});
@@ -31,7 +34,7 @@
test('formatText newlines 2', () => {
const text = '<span class="thumbsup">👍</span>';
assert.equal(
- formatText(text, 'NONE', 4, 10, '').innerHTML,
+ formatText(text, 'NONE', 4, 10, '').firstElementChild?.innerHTML,
'<span clas' +
LINE_BREAK_HTML +
's="thumbsu' +
@@ -45,7 +48,7 @@
test('formatText newlines 3', () => {
const text = '01234\t56789';
assert.equal(
- formatText(text, 'NONE', 4, 10, '').innerHTML,
+ formatText(text, 'NONE', 4, 10, '').firstElementChild?.innerHTML,
'01234' + createTabWrapper(3).outerHTML + '56' + LINE_BREAK_HTML + '789'
);
});
@@ -53,7 +56,7 @@
test('formatText newlines 4', () => {
const text = '👍'.repeat(58);
assert.equal(
- formatText(text, 'NONE', 4, 20, '').innerHTML,
+ formatText(text, 'NONE', 4, 20, '').firstElementChild?.innerHTML,
'👍'.repeat(20) +
LINE_BREAK_HTML +
'👍'.repeat(20) +
@@ -82,7 +85,8 @@
assert.ok(wrapper);
assert.equal(wrapper.innerText, '\t');
assert.equal(
- formatText(html, 'NONE', tabSize, Infinity, '').innerHTML,
+ formatText(html, 'NONE', tabSize, Infinity, '').firstElementChild
+ ?.innerHTML,
'abc' + wrapper.outerHTML + 'def'
);
});
@@ -91,31 +95,22 @@
let input = '<script>alert("XSS");<' + '/script>';
let expected = '<script>alert("XSS");</script>';
- let result = formatText(
- input,
- 'NONE',
- 1,
- Number.POSITIVE_INFINITY,
- ''
- ).innerHTML;
+ let result = formatText(input, 'NONE', 1, Number.POSITIVE_INFINITY, '')
+ .firstElementChild?.innerHTML;
assert.equal(result, expected);
input = '& < > " \' / `';
expected = '& < > " \' / `';
- result = formatText(
- input,
- 'NONE',
- 1,
- Number.POSITIVE_INFINITY,
- ''
- ).innerHTML;
+ result = formatText(input, 'NONE', 1, Number.POSITIVE_INFINITY, '')
+ .firstElementChild?.innerHTML;
assert.equal(result, expected);
});
test('text length with tabs and unicode', () => {
function expectTextLength(text: string, tabSize: number, expected: number) {
// Formatting to |expected| columns should not introduce line breaks.
- const result = formatText(text, 'NONE', tabSize, expected, '');
+ const result = formatText(text, 'NONE', tabSize, expected, '')
+ .firstElementChild!;
assert.isNotOk(
result.querySelector('.contentText > .br'),
' Expected the result of: \n' +
@@ -126,19 +121,22 @@
// Increasing the line limit should produce the same markup.
assert.equal(
- formatText(text, 'NONE', tabSize, Infinity, '').innerHTML,
+ formatText(text, 'NONE', tabSize, Infinity, '').firstElementChild
+ ?.innerHTML,
result.innerHTML
);
assert.equal(
- formatText(text, 'NONE', tabSize, expected + 1, '').innerHTML,
+ formatText(text, 'NONE', tabSize, expected + 1, '').firstElementChild
+ ?.innerHTML,
result.innerHTML
);
// Decreasing the line limit should introduce line breaks.
if (expected > 0) {
- const tooSmall = formatText(text, 'NONE', tabSize, expected - 1, '');
+ const tooSmall = formatText(text, 'NONE', tabSize, expected - 1, '')
+ .firstElementChild!;
assert.isOk(
- tooSmall.querySelector('.contentText > .br'),
+ tooSmall.querySelector('.contentText .br'),
' Expected the result of: \n' +
` _formatText(${text}', ${tabSize}, ${expected - 1})\n` +
' to contain a br. But the actual result HTML was:\n' +
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 1a2a8ad..569de48 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -665,6 +665,8 @@
background-color: var(--view-background-color);
}
+ .content .contentText gr-diff-text:empty:after,
+ .content .contentText gr-legacy-text:empty:after,
.content .contentText:empty:after {
/* Newline, to ensure empty lines are one line-height tall. */
content: '\\A';
@@ -971,6 +973,8 @@
z-index: 10;
}
+ gr-diff-image-new,
+ gr-diff-image-old,
gr-diff-section,
gr-context-controls-section,
gr-diff-row {
@@ -996,6 +1000,13 @@
if (this.loggedIn) {
this.addSelectionListeners();
}
+ if (this.diff && this.diffTable) {
+ this.diffSelection.init(this.diff, this.diffTable);
+ }
+ if (this.diffTable && this.diffBuilder) {
+ this.highlights.init(this.diffTable, this.diffBuilder);
+ }
+ this.diffBuilder.init();
}
override disconnectedCallback() {
@@ -1003,7 +1014,7 @@
this.renderDiffTableTask?.cancel();
this.diffSelection.cleanup();
this.highlights.cleanup();
- this.diffBuilder.cancel();
+ this.diffBuilder.cleanup();
super.disconnectedCallback();
}
@@ -1032,9 +1043,6 @@
}
if (changedProperties.has('coverageRanges')) {
this.diffBuilder.updateCoverageRanges(this.coverageRanges);
- if (this.diff) {
- this.debounceRenderDiffTable();
- }
}
if (changedProperties.has('lineOfInterest')) {
this.lineOfInterestChanged();
@@ -1267,7 +1275,7 @@
/** Cancel any remaining diff builder rendering work. */
cancel() {
- this.diffBuilder.cancel();
+ this.diffBuilder.cleanup();
this.renderDiffTableTask?.cancel();
}
@@ -1407,23 +1415,6 @@
);
}
- /**
- * Gets or creates a comment thread group for a specific line and side on a
- * diff.
- * Private but used in tests.
- */
- getOrCreateThreadGroup(contentEl: Element, commentSide: Side) {
- // Check if thread group exists.
- let threadGroupEl = contentEl.querySelector('.thread-group');
- if (!threadGroupEl) {
- threadGroupEl = document.createElement('div');
- threadGroupEl.className = 'thread-group';
- threadGroupEl.setAttribute('data-side', commentSide);
- contentEl.appendChild(threadGroupEl);
- }
- return threadGroupEl;
- }
-
private getCommentSideByLineAndContent(
lineEl: Element,
contentEl: Element
@@ -1693,7 +1684,6 @@
if (lineNum === 'LOST') {
this.insertPortedCommentsWithoutRangeMessage(contentEl);
}
- const threadGroupEl = this.getOrCreateThreadGroup(contentEl, commentSide);
const slotAtt = threadEl.getAttribute('slot');
if (range && isLongCommentRange(range) && slotAtt) {
@@ -1706,16 +1696,6 @@
this.insertBefore(longRangeCommentHint, threadEl);
this.redispatchHoverEvents(longRangeCommentHint, threadEl);
}
-
- // Create a slot for the thread and attach it to the thread group.
- // The Polyfill has some bugs and this only works if the slot is
- // attached to the group after the group is attached to the DOM.
- // The thread group may already have a slot with the right name, but
- // that is okay because the first matching slot is used and the rest
- // are ignored.
- const slot = document.createElement('slot');
- if (slotAtt) slot.name = slotAtt;
- threadGroupEl.appendChild(slot);
}
for (const threadEl of removedThreadEls) {
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 f657973..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
@@ -6,7 +6,6 @@
import '../../../test/common-test-setup';
import {createDiff} from '../../../test/test-data-generators';
import './gr-diff';
-import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image';
import {getComputedStyleValue} from '../../../utils/dom-util';
import '@polymer/paper-button/paper-button';
import {
@@ -94,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"
@@ -108,18 +108,19 @@
<td class="blame gr-diff" data-line-number="LOST"></td>
<td class="gr-diff left lineNum" data-value="LOST"></td>
<td class="gr-diff left no-intraline-info sign"></td>
- <td
- class="both content gr-diff left lost no-intraline-info"
- ></td>
+ <td class="both content gr-diff left lost no-intraline-info">
+ <div class="thread-group" data-side="left"></div>
+ </td>
<td class="gr-diff lineNum right" data-value="LOST"></td>
<td class="gr-diff no-intraline-info right sign"></td>
- <td
- class="both content gr-diff lost no-intraline-info right"
- ></td>
+ <td class="both content gr-diff lost no-intraline-info right">
+ <div class="thread-group" data-side="right"></div>
+ </td>
</tr>
</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"
@@ -138,9 +139,9 @@
</button>
</td>
<td class="gr-diff left no-intraline-info sign"></td>
- <td
- class="both content file gr-diff left no-intraline-info"
- ></td>
+ <td class="both content file gr-diff left no-intraline-info">
+ <div class="thread-group" data-side="left"></div>
+ </td>
<td class="gr-diff lineNum right" data-value="FILE">
<button
aria-label="Add file comment"
@@ -153,9 +154,9 @@
</button>
</td>
<td class="gr-diff no-intraline-info right sign"></td>
- <td
- class="both content file gr-diff no-intraline-info right"
- ></td>
+ <td class="both content file gr-diff no-intraline-info right">
+ <div class="thread-group" data-side="right"></div>
+ </td>
</tr>
</tbody>
<tbody class="both gr-diff section">
@@ -184,9 +185,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-1"
- >
- Lorem ipsum dolor sit amet, suspendisse inceptos vehicula.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="1">
<button
@@ -205,9 +205,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-1"
- >
- Lorem ipsum dolor sit amet, suspendisse inceptos vehicula.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -235,9 +234,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-2"
- >
- Mattis lectus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="2">
<button
@@ -256,9 +254,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-2"
- >
- Mattis lectus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -286,9 +283,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-3"
- >
- Sodales duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="3">
<button
@@ -307,9 +303,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-3"
- >
- Sodales duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -337,9 +332,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-4"
- >
- Orci a faucibus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="4">
<button
@@ -358,9 +352,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-4"
- >
- Orci a faucibus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -376,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
@@ -399,9 +388,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-5"
- >
- Nullam neque, ligula ac, id blandit.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -415,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
@@ -438,9 +422,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-6"
- >
- Sagittis tincidunt torquent, tempor nunc amet.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -454,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
@@ -477,9 +456,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-7"
- >
- At rhoncus id.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -509,9 +487,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-5"
- >
- Sem nascetur, erat ut, non in.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="8">
<button
@@ -530,9 +507,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-8"
- >
- Sem nascetur, erat ut, non in.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -560,9 +536,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-6"
- >
- A donec, venenatis pellentesque dis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="9">
<button
@@ -581,9 +556,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-9"
- >
- A donec, venenatis pellentesque dis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -611,9 +585,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-7"
- >
- Mauris mauris.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="10">
<button
@@ -632,9 +605,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-10"
- >
- Mauris mauris.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -662,9 +634,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-8"
- >
- Quisque nisl duis, facilisis viverra.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="11">
<button
@@ -683,9 +654,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-11"
- >
- Quisque nisl duis, facilisis viverra.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -713,9 +683,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-9"
- >
- Justo purus, semper eget et.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="12">
<button
@@ -734,9 +703,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-12"
- >
- Justo purus, semper eget et.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -766,18 +734,13 @@
class="contentText gr-diff"
data-side="left"
id="left-content-10"
- >
- Est amet, vestibulum pellentesque.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<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
@@ -805,18 +768,13 @@
class="contentText gr-diff"
data-side="left"
id="left-content-11"
- >
- Erat ligula.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<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
@@ -844,18 +802,13 @@
class="contentText gr-diff"
data-side="left"
id="left-content-12"
- >
- Justo eros.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<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
@@ -883,18 +836,13 @@
class="contentText gr-diff"
data-side="left"
id="left-content-13"
- >
- Fringilla quisque.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<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>
@@ -924,9 +872,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-14"
- >
- Arcu eget, rhoncus amet cursus, ipsum elementum.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="13">
<button
@@ -945,9 +892,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-13"
- >
- Arcu eget, rhoncus amet cursus, ipsum elementum.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -975,9 +921,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-15"
- >
- Eros suspendisse.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="14">
<button
@@ -996,9 +941,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-14"
- >
- Eros suspendisse.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1028,10 +972,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-16"
- >
- Rhoncus tempor, ultricies
- <hl class="gr-diff intraline"> aliquam </hl> ipsum.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="15">
<button
@@ -1050,10 +992,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-15"
- >
- Rhoncus tempor, ultricies
- <hl class="gr-diff intraline"> praesent </hl> ipsum.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1083,9 +1023,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-17"
- >
- Sollicitudin duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="16">
<button
@@ -1104,9 +1043,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-16"
- >
- Sollicitudin duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1134,9 +1072,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-18"
- >
- Blandit blandit, ante nisl fusce.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="17">
<button
@@ -1155,9 +1092,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-17"
- >
- Blandit blandit, ante nisl fusce.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1185,9 +1121,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-19"
- >
- Felis ac at, tellus consectetuer.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="18">
<button
@@ -1206,9 +1141,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-18"
- >
- Felis ac at, tellus consectetuer.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1276,9 +1210,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-38"
- >
- Ullamcorper nunc ante, nec imperdiet felis, consectetur.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="37">
<button
@@ -1297,9 +1230,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-37"
- >
- Ullamcorper nunc ante, nec imperdiet felis, consectetur.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1327,9 +1259,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-39"
- >
- Ac eget.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="38">
<button
@@ -1348,9 +1279,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-38"
- >
- Ac eget.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1378,9 +1308,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-40"
- >
- Vel fringilla, interdum pellentesque placerat, proin ante.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="39">
<button
@@ -1399,9 +1328,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-39"
- >
- Vel fringilla, interdum pellentesque placerat, proin ante.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1417,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
@@ -1440,9 +1364,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-40"
- >
- Eu congue risus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1456,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
@@ -1479,9 +1398,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-41"
- >
- Enim ac, quis elementum.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1495,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
@@ -1518,9 +1432,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-42"
- >
- Non et elit.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1534,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
@@ -1557,9 +1466,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-43"
- >
- Etiam aliquam, diam vel nunc.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1589,9 +1497,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-41"
- >
- Nec at.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="44">
<button
@@ -1610,9 +1517,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-44"
- >
- Nec at.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1640,9 +1546,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-42"
- >
- Arcu mauris, venenatis lacus fermentum, praesent duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="45">
<button
@@ -1661,9 +1566,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-45"
- >
- Arcu mauris, venenatis lacus fermentum, praesent duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1691,9 +1595,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-43"
- >
- Pellentesque amet et, tellus duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="46">
<button
@@ -1712,9 +1615,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-46"
- >
- Pellentesque amet et, tellus duis.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1742,9 +1644,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-44"
- >
- Ipsum arcu vitae, justo elit, sed libero tellus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="47">
<button
@@ -1763,9 +1664,8 @@
class="contentText gr-diff"
data-side="right"
id="right-content-47"
- >
- Ipsum arcu vitae, justo elit, sed libero tellus.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
<tr
@@ -1793,9 +1693,8 @@
class="contentText gr-diff"
data-side="left"
id="left-content-45"
- >
- Metus rutrum euismod, vivamus sodales, vel arcu nisl.
- </div>
+ ></div>
+ <div class="thread-group" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="48">
<button
@@ -1814,15 +1713,17 @@
class="contentText gr-diff"
data-side="right"
id="right-content-48"
- >
- Metus rutrum euismod, vivamus sodales, vel arcu nisl.
- </div>
+ ></div>
+ <div class="thread-group" data-side="right"></div>
</td>
</tr>
</tbody>
</table>
</div>
- `
+ `,
+ {
+ ignoreTags: ['gr-legacy-text', 'slot'],
+ }
);
};
});
@@ -1857,9 +1758,9 @@
});
test('cancel', () => {
- const cancelStub = sinon.stub(element.diffBuilder, 'cancel');
+ const cleanupStub = sinon.stub(element.diffBuilder, 'cleanup');
element.cancel();
- assert.isTrue(cancelStub.calledOnce);
+ assert.isTrue(cleanupStub.calledOnce);
});
test('line limit with line_wrapping', async () => {
@@ -1969,25 +1870,6 @@
assert.isTrue(container.classList.contains('displayLine'));
});
- test('thread groups', () => {
- const contentEl = document.createElement('div');
-
- element.path = 'file.txt';
-
- // No thread groups.
- assert.equal(contentEl.querySelectorAll('.thread-group').length, 0);
-
- // A thread group gets created.
- const threadGroupEl = element.getOrCreateThreadGroup(
- contentEl,
- Side.LEFT
- );
- assert.isOk(threadGroupEl);
-
- // The new thread group can be fetched.
- assert.equal(contentEl.querySelectorAll('.thread-group').length, 1);
- });
-
suite('image diffs', () => {
let mockFile1: ImageInfo;
let mockFile2: ImageInfo;
@@ -2021,7 +1903,7 @@
};
});
- test('renders image diffs with same file name', async () => {
+ test('render image diff', async () => {
element.baseImage = mockFile1;
element.revisionImage = mockFile2;
element.diff = {
@@ -2039,39 +1921,62 @@
content: [{skip: 66}],
binary: true,
};
+
await waitForEventOnce(element, 'render');
-
- // Recognizes that it should be an image diff.
- assert.isTrue(element.isImageDiff);
- assert.instanceOf(element.diffBuilder.builder, GrDiffBuilderImage);
-
- // Left image rendered with the parent commit's version of the file.
- assertIsDefined(element.diffTable);
- const diffTable = element.diffTable;
- const leftImage = queryAndAssert(diffTable, 'td.left img');
- const leftLabel = queryAndAssert(diffTable, 'td.left label');
- const leftLabelContent = queryAndAssert(leftLabel, '.label');
- const leftLabelName = query(leftLabel, '.name');
-
- const rightImage = queryAndAssert(diffTable, 'td.right img');
- const rightLabel = queryAndAssert(diffTable, 'td.right label');
- const rightLabelContent = queryAndAssert(rightLabel, '.label');
- const rightLabelName = query(rightLabel, '.name');
-
- assert.isNotOk(rightLabelName);
- assert.isNotOk(leftLabelName);
-
- assert.equal(
- leftImage.getAttribute('src'),
- 'data:image/bmp;base64,' + mockFile1.body
+ const imageDiffSection = queryAndAssert(element, 'tbody.image-diff');
+ assert.lightDom.equal(
+ imageDiffSection,
+ /* HTML */ `
+ <tbody class="gr-diff image-diff">
+ <tr class="gr-diff">
+ <td class="blank gr-diff left lineNum"></td>
+ <td class="gr-diff left">
+ <img
+ 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 right"
+ src="data:image/bmp;base64,${mockFile2.body}"
+ />
+ </td>
+ </tr>
+ <tr class="gr-diff">
+ <td class="blank gr-diff left lineNum"></td>
+ <td class="gr-diff left">
+ <label class="gr-diff">
+ <span class="gr-diff label"> image/bmp </span>
+ </label>
+ </td>
+ <td class="blank gr-diff lineNum right"></td>
+ <td class="gr-diff right">
+ <label class="gr-diff">
+ <span class="gr-diff label"> image/bmp </span>
+ </label>
+ </td>
+ </tr>
+ </tbody>
+ `
);
- assert.isTrue(leftLabelContent.textContent?.includes('image/bmp'));
-
- assert.equal(
- rightImage.getAttribute('src'),
- 'data:image/bmp;base64,' + mockFile2.body
+ 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>
+ `
);
- assert.isTrue(rightLabelContent.textContent?.includes('image/bmp'));
});
test('renders image diffs with a different file name', async () => {
@@ -2096,43 +2001,31 @@
element.revisionImage = mockFile2;
element.revisionImage._name = mockDiff.meta_b!.name;
element.diff = mockDiff;
+
await waitForEventOnce(element, 'render');
-
- // Recognizes that it should be an image diff.
- assert.isTrue(element.isImageDiff);
- assert.instanceOf(element.diffBuilder.builder, GrDiffBuilderImage);
-
- // Left image rendered with the parent commit's version of the file.
- assertIsDefined(element.diffTable);
- const diffTable = element.diffTable;
- const leftImage = queryAndAssert(diffTable, 'td.left img');
- const leftLabel = queryAndAssert(diffTable, 'td.left label');
- const leftLabelContent = queryAndAssert(leftLabel, '.label');
- const leftLabelName = queryAndAssert(leftLabel, '.name');
-
- const rightImage = queryAndAssert(diffTable, 'td.right img');
- const rightLabel = queryAndAssert(diffTable, 'td.right label');
- const rightLabelContent = queryAndAssert(rightLabel, '.label');
- const rightLabelName = queryAndAssert(rightLabel, '.name');
-
- assert.isOk(rightLabelName);
- assert.isOk(leftLabelName);
- assert.equal(leftLabelName.textContent, mockDiff.meta_a?.name);
- assert.equal(rightLabelName.textContent, mockDiff.meta_b?.name);
-
- assert.isOk(leftImage);
- assert.equal(
- leftImage.getAttribute('src'),
- 'data:image/bmp;base64,' + mockFile1.body
+ const imageDiffSection = queryAndAssert(element, 'tbody.image-diff');
+ const leftLabel = queryAndAssert(imageDiffSection, 'td.left label');
+ const rightLabel = queryAndAssert(imageDiffSection, 'td.right label');
+ assert.dom.equal(
+ leftLabel,
+ /* HTML */ `
+ <label class="gr-diff">
+ <span class="gr-diff name"> carrot.jpg </span>
+ <br class="gr-diff" />
+ <span class="gr-diff label"> image/bmp </span>
+ </label>
+ `
);
- assert.isTrue(leftLabelContent.textContent?.includes('image/bmp'));
-
- assert.isOk(rightImage);
- assert.equal(
- rightImage.getAttribute('src'),
- 'data:image/bmp;base64,' + mockFile2.body
+ assert.dom.equal(
+ rightLabel,
+ /* HTML */ `
+ <label class="gr-diff">
+ <span class="gr-diff name"> carrot2.jpg </span>
+ <br class="gr-diff" />
+ <span class="gr-diff label"> image/bmp </span>
+ </label>
+ `
);
- assert.isTrue(rightLabelContent.textContent?.includes('image/bmp'));
});
test('renders added image', async () => {
@@ -2150,26 +2043,23 @@
content: [{skip: 66}],
binary: true,
};
-
- const promise = mockPromise();
- function rendered() {
- promise.resolve();
- }
- element.addEventListener('render', rendered);
-
element.revisionImage = mockFile2;
element.diff = mockDiff;
- await promise;
- element.removeEventListener('render', rendered);
- // Recognizes that it should be an image diff.
- assert.isTrue(element.isImageDiff);
- assert.instanceOf(element.diffBuilder.builder, GrDiffBuilderImage);
- assertIsDefined(element.diffTable);
- const diffTable = element.diffTable;
- const leftImage = query(diffTable, 'td.left img');
+ await waitForEventOnce(element, 'render');
+ const imageDiffSection = queryAndAssert(element, 'tbody.image-diff');
+ const leftImage = query(imageDiffSection, 'td.left img');
+ const rightImage = queryAndAssert(imageDiffSection, 'td.right img');
assert.isNotOk(leftImage);
- queryAndAssert(diffTable, 'td.right img');
+ assert.dom.equal(
+ rightImage,
+ /* HTML */ `
+ <img
+ class="gr-diff right"
+ src="data:image/bmp;base64,${mockFile2.body}"
+ />
+ `
+ );
});
test('renders removed image', async () => {
@@ -2187,25 +2077,23 @@
content: [{skip: 66}],
binary: true,
};
- const promise = mockPromise();
- function rendered() {
- promise.resolve();
- }
- element.addEventListener('render', rendered);
-
element.baseImage = mockFile1;
element.diff = mockDiff;
- await promise;
- element.removeEventListener('render', rendered);
- // Recognizes that it should be an image diff.
- assert.isTrue(element.isImageDiff);
- assert.instanceOf(element.diffBuilder.builder, GrDiffBuilderImage);
- assertIsDefined(element.diffTable);
- const diffTable = element.diffTable;
- queryAndAssert(diffTable, 'td.left img');
- const rightImage = query(diffTable, 'td.right img');
+ await waitForEventOnce(element, 'render');
+ const imageDiffSection = queryAndAssert(element, 'tbody.image-diff');
+ const leftImage = queryAndAssert(imageDiffSection, 'td.left img');
+ const rightImage = query(imageDiffSection, 'td.right img');
assert.isNotOk(rightImage);
+ assert.dom.equal(
+ leftImage,
+ /* HTML */ `
+ <img
+ class="gr-diff left"
+ src="data:image/bmp;base64,${mockFile1.body}"
+ />
+ `
+ );
});
test('does not render disallowed image type', async () => {
@@ -2228,23 +2116,12 @@
binary: true,
};
mockFile1.type = 'image/jpeg-evil';
-
- const promise = mockPromise();
- function rendered() {
- promise.resolve();
- }
- element.addEventListener('render', rendered);
-
element.baseImage = mockFile1;
element.diff = mockDiff;
- await promise;
- element.removeEventListener('render', rendered);
- // Recognizes that it should be an image diff.
- assert.isTrue(element.isImageDiff);
- assert.instanceOf(element.diffBuilder.builder, GrDiffBuilderImage);
- assertIsDefined(element.diffTable);
- const diffTable = element.diffTable;
- const leftImage = query(diffTable, 'td.left img');
+
+ await waitForEventOnce(element, 'render');
+ const imageDiffSection = queryAndAssert(element, 'tbody.image-diff');
+ const leftImage = query(imageDiffSection, 'td.left img');
assert.isNotOk(leftImage);
});
});
@@ -2582,45 +2459,30 @@
test('large render w/ context = 10', async () => {
element.prefs = {...MINIMAL_PREFS, context: 10};
- const promise = mockPromise();
- function rendered() {
- assert.isTrue(renderStub.called);
- assert.isFalse(element.showWarning);
- promise.resolve();
- element.removeEventListener('render', rendered);
- }
- element.addEventListener('render', rendered);
element.renderDiffTable();
- await promise;
+ await waitForEventOnce(element, 'render');
+
+ assert.isTrue(renderStub.called);
+ assert.isFalse(element.showWarning);
});
test('large render w/ whole file and bypass', async () => {
element.prefs = {...MINIMAL_PREFS, context: -1};
element.safetyBypass = 10;
- const promise = mockPromise();
- function rendered() {
- assert.isTrue(renderStub.called);
- assert.isFalse(element.showWarning);
- promise.resolve();
- element.removeEventListener('render', rendered);
- }
- element.addEventListener('render', rendered);
element.renderDiffTable();
- await promise;
+ await waitForEventOnce(element, 'render');
+
+ assert.isTrue(renderStub.called);
+ assert.isFalse(element.showWarning);
});
test('large render w/ whole file and no bypass', async () => {
element.prefs = {...MINIMAL_PREFS, context: -1};
- const promise = mockPromise();
- function rendered() {
- assert.isFalse(renderStub.called);
- assert.isTrue(element.showWarning);
- promise.resolve();
- element.removeEventListener('render', rendered);
- }
- element.addEventListener('render', rendered);
element.renderDiffTable();
- await promise;
+ await waitForEventOnce(element, 'render');
+
+ assert.isFalse(renderStub.called);
+ assert.isTrue(element.showWarning);
});
test('toggles expand context using bypass', async () => {
diff --git a/polygerrit-ui/app/models/plugins/plugins-model.ts b/polygerrit-ui/app/models/plugins/plugins-model.ts
index ccca0eb..83235b17 100644
--- a/polygerrit-ui/app/models/plugins/plugins-model.ts
+++ b/polygerrit-ui/app/models/plugins/plugins-model.ts
@@ -12,6 +12,12 @@
} from '../../api/checks';
import {Model} from '../model';
import {select} from '../../utils/observable-util';
+import {CoverageProvider} from '../../api/annotation';
+
+export interface CoveragePlugin {
+ pluginName: string;
+ provider: CoverageProvider;
+}
export interface ChecksPlugin {
pluginName: string;
@@ -28,6 +34,10 @@
/** Application wide state of plugins. */
interface PluginsState {
/**
+ * List of plugins that have called annotationApi().setCoverageProvider().
+ */
+ coveragePlugins: CoveragePlugin[];
+ /**
* List of plugins that have called checks().register().
*/
checksPlugins: ChecksPlugin[];
@@ -50,19 +60,38 @@
public checksPlugins$ = select(this.state$, state => state.checksPlugins);
+ public coveragePlugins$ = select(this.state$, state => state.coveragePlugins);
+
constructor() {
super({
+ coveragePlugins: [],
checksPlugins: [],
});
}
+ coverageRegister(plugin: CoveragePlugin) {
+ const nextState = {...this.getState()};
+ nextState.coveragePlugins = [...nextState.coveragePlugins];
+ const alreadyRegistered = nextState.coveragePlugins.some(
+ p => p.pluginName === plugin.pluginName
+ );
+ if (alreadyRegistered) {
+ console.warn(
+ `${plugin.pluginName} tried to register twice as a coverage provider. Ignored.`
+ );
+ return;
+ }
+ nextState.coveragePlugins.push(plugin);
+ this.setState(nextState);
+ }
+
checksRegister(plugin: ChecksPlugin) {
const nextState = {...this.getState()};
nextState.checksPlugins = [...nextState.checksPlugins];
- const alreadysRegistered = nextState.checksPlugins.some(
+ const alreadyRegistered = nextState.checksPlugins.some(
p => p.pluginName === plugin.pluginName
);
- if (alreadysRegistered) {
+ if (alreadyRegistered) {
console.warn(
`${plugin.pluginName} tried to register twice as a checks provider. Ignored.`
);
diff --git a/polygerrit-ui/app/styles/shared-styles.ts b/polygerrit-ui/app/styles/shared-styles.ts
index 839f612..5a7ca48 100644
--- a/polygerrit-ui/app/styles/shared-styles.ts
+++ b/polygerrit-ui/app/styles/shared-styles.ts
@@ -124,13 +124,8 @@
/* iron-autogrow-textarea has a "-webkit-appearance: textarea" :host
css rule, which prevents overriding the border color. Clear that. */
-webkit-appearance: none;
- --iron-autogrow-textarea: {
- box-sizing: border-box;
- padding: var(--spacing-s);
- };
--iron-autogrow-textarea_-_box-sizing: border-box;
--iron-autogrow-textarea_-_padding: var(--spacing-s);
- --iron-autogrow-textarea_-_white-space: pre-wrap;
}
a {
color: var(--link-color);
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;
}
/**
diff --git a/tools/bzl/pkg_war.bzl b/tools/bzl/pkg_war.bzl
index c9ac0fe..e2be145 100644
--- a/tools/bzl/pkg_war.bzl
+++ b/tools/bzl/pkg_war.bzl
@@ -14,6 +14,8 @@
# War packaging.
+load("//tools:deps.bzl", "AUTO_FACTORY_VERSION", "AUTO_VALUE_GSON_VERSION", "AUTO_VALUE_VERSION")
+
jar_filetype = [".jar"]
LIBS = [
@@ -31,6 +33,13 @@
"//java/com/google/gerrit/pgm",
]
+SKIP_DEPS = [
+ "auto-factory-%s.jar" % AUTO_FACTORY_VERSION,
+ "auto-value-%s.jar" % AUTO_VALUE_VERSION,
+ "auto-value-annotations-%s.jar" % AUTO_VALUE_VERSION,
+ "auto-value-gson-runtime-%s.jar" % AUTO_VALUE_GSON_VERSION,
+]
+
def _add_context(in_file, output):
input_path = in_file.path
return [
@@ -85,6 +94,8 @@
transitive_lib_deps = depset(transitive = transitive_libs)
for dep in transitive_lib_deps.to_list():
+ if dep.basename in SKIP_DEPS:
+ continue
cmd += _add_file(dep, build_output + "/WEB-INF/lib/")
inputs.append(dep)
@@ -95,6 +106,8 @@
transitive_pgmlib_deps = depset(transitive = transitive_pgmlibs)
for dep in transitive_pgmlib_deps.to_list():
+ if dep.basename in SKIP_DEPS:
+ continue
if dep not in inputs:
cmd += _add_file(dep, build_output + "/WEB-INF/pgm-lib/")
inputs.append(dep)
diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl
index cf50499..7f26ef3 100644
--- a/tools/nongoogle.bzl
+++ b/tools/nongoogle.bzl
@@ -67,18 +67,18 @@
sha1 = "cb2f351bf4463751201f43bb99865235d5ba07ca",
)
- SSHD_VERS = "2.9.1"
+ SSHD_VERS = "2.9.2"
maven_jar(
name = "sshd-osgi",
artifact = "org.apache.sshd:sshd-osgi:" + SSHD_VERS,
- sha1 = "9ed1a653da98a1aabe3ae092ee8310299718e914",
+ sha1 = "bac0415734519b2fe433fea196017acf7ed32660",
)
maven_jar(
name = "sshd-sftp",
artifact = "org.apache.sshd:sshd-sftp:" + SSHD_VERS,
- sha1 = "6d01cb8138e60e97e3de08e96cc5a094c8ce2cac",
+ sha1 = "7f9089c87b3b44f19998252fd3b68637e3322920",
)
maven_jar(
@@ -89,14 +89,14 @@
maven_jar(
name = "mina-core",
- artifact = "org.apache.mina:mina-core:2.0.21",
- sha1 = "e1a317689ecd438f54e863747e832f741ef8e092",
+ artifact = "org.apache.mina:mina-core:2.0.23",
+ sha1 = "391228b25d3a24434b205444cd262780a9ea61e7",
)
maven_jar(
name = "sshd-mina",
artifact = "org.apache.sshd:sshd-mina:" + SSHD_VERS,
- sha1 = "5ab797b99630bb0c3e9ebcd8a3a6cad46408a79a",
+ sha1 = "765dced3a2b4069bb0c550e18bda057bad8de26f",
)
maven_jar(
diff --git a/tools/release_noter/release_noter.py b/tools/release_noter/release_noter.py
old mode 100644
new mode 100755
