58 Commits

Author SHA1 Message Date
Wyatt Allen
18f0354a33 Report time between draft comment actions
Report the time between create, update or discard flows are initiated on
diff draft comments.

Change-Id: I71b2f955bfa65dde57307d67fda45069bffa02c0
2018-06-23 00:45:53 +00:00
Dave Borowitz
8cdc76ba4c Add @license tags to PG HTML and JS assets
These tags are preserved by the Closure compiler and vulcanize in order
to serve the license notices embedded in the outputs. In a standalone
Gerrit server, these license are also covered in the LICENSES.txt served
with the documentation. When serving PG assets from a CDN, it's less
obvious what the corresponding LICENSES.txt file is, since the CDN is
not directly linked to a running Gerrit server. Safer to embed the
licenses in the assets themselves.

Change-Id: Id1add1451fad1baa7916882a6bda02c326ccc988
2018-03-26 10:47:55 -04:00
Becky Siegel
e115b14541 Add events to thread list and diff view
Whenever a comment changes in either the thread view or the diff view,
the other view will need to be notified of the change in order to
re-render its comments, since they do not share a common data source
(The diff view's comments are added manually when the diff is rendered
via the diff builder).

Because the gr-diff-comment-thread is common to both uses, it can fire
events to be consumed in both cases.

From there, gr-diff and gr-list-view each fire a separate event, which
is listened for on the change view (common ancestor). The change view
will then trigger reloading of comments for the element that needs

Change-Id: Ia31c0ef512af408c4d099667558a32f86ed43152
2018-03-11 14:21:01 -07:00
Becky Siegel
98cfaedebe Update thread discarding
Instead of letting the diff discard the threads, let the thread group
take care of it.

Change-Id: I1a21bd6ae02534202b8829e0e6224c4a607c3d4e
2018-03-08 21:06:26 -08:00
Becky Siegel
99990d7fc8 Add optional file link to gr-comment-thread
This will be used by gr-thread-view so that comment threads can have
context. In order to generate URLs, comment threads will also need a
line and change property.

Change-Id: I35d405c8af9f10b4e0c89dd5fcb9e48b7b373534
2018-02-27 10:48:32 -08:00
Becky Siegel
3eac40a5e9 Merge "Add rough component for viewing comment threads in the change view" 2018-02-22 18:32:59 +00:00
Becky Siegel
a7b81d2de5 Add rough component for viewing comment threads in the change view
Future changes will enhance the aesthetics of this component and insert
it as a tab by the messages list

Bug: Issue 8241
Change-Id: Ia68519ba67aa0fa07d1687e731e1ed82e9979369
2018-02-22 01:55:29 +00:00
Wyatt Allen
659f15110f Correct logic for matching a new comment to its thread in a group
When a new comment is being added to a line, Gerrit checks to see if
there is an existing thread on the same line (and same range, if any) so
that the comment can be appended to it if so. Otherwise, a new thread is
created.

However, following I4f7804ac02, the logic to identify the appropriate
thread by range was refactored to not use range location strings, but
use range objects instead. Problematically, there were two flaws in this
code:
1)  The range object references were compared rather than their values.
2)  Only new threads were being rendered with their corresponding ranges
    whereas existing threads were not.

As a result, if the user attempted to add a line comment on a line with
an existing ranged comment, the ranged comment's thread would be
identified as the destination (because the new comment has no range and
the existing thread's range was not being set). Appending the range-less
comment to a ranged thread resulted in incoherent data and the draft
would be unsavable.

With this change, the logic uses value equality to match ranges and the
`gr-diff-comment-thread-group#_getThreads` method is updated to set the
range on existing threads.

Bug: Issue 8410
Change-Id: If34e0d46a5c1af81bec82125217088fb574a2f61
2018-02-21 21:51:56 +00:00
Becky Siegel
bbde672af0 Update comment thread creation/removal to no longer use locationRange
Follow-up to https://gerrit-review.googlesource.com/c/gerrit/+/157590
which no longer used locationRange to group comments together into
threads. However, locationRange was still used for a few other things
including thread removal, which could result in a bug if there were
two threads that had the same locationRange and the wrong one was
found first.

Things included in this change:
1. Determine whether a thread group exists
   - This is identified as it was before. The thread group now gets a
     commentSide attribute, as it can pass it to all of its children
     (the same for all).
2. Determine if a thread exists within the group for a given range in a
   group
   - The range object is now included as an attribute on the comment
     thread instead of the locationRange string.
3. Be able to remove a thread group
   - A rootId attribute is now stored on the comment thread, which is
     data bound to each comment. When a comment is removed, it finds the
     thread with the rootId within the thread group to remove.
   - In the event that a thread group only contains a single draft,
     there is no roottId.

Change-Id: I4f7804ac02f4259b4964c6333d258f0fc3b29d24
2018-02-12 17:36:33 -08:00
Becky Siegel
428ec67a7d Prepare gr-diff-comment-thread for use in comment thread view
- Updaotes the unresolved property to be public, and reflects the
  attribute. This will be used for the toggle to show unresolved threads
  or all threads.
- Update logic to always show drafts at the end of sorted comments.
  Without this, the thread could get in a weird state where you can
  still add another draft, but since they are not threaded, and once the
  comment is published, its date changes to the published date anyway,
  so its ordering will be at the end.
- This change also adds the __draft attribute in gr-diff-comment-api so
  that draft comments will render correctly in the new view.

Change-Id: Icacc46ab5fd643b061ecfac8dcd313f815d17199
2018-02-05 17:16:30 -08:00
Viktar Donich
a671085c3c Expand robot comments by default
Change-Id: I6c55c0dad4cc0bd96013c77ad75b3cd6c6d34cdd
2017-12-11 14:04:48 -08:00
Kasper Nilsson
46a6c8eca2 Allow toggling resolved outside editing state
A recent change modified some comment editing UX. As a consequence, the
save button disabled state was erroneously based entirely on whether or
not the comment message had changed.

With this change:
- Save disabled state on a comment takes into account resolved state
- The resolved checkbox is always visible on an expanded draft comment
- When the resolved checkbox is modified on a non-`editing` draft
  comment, it is saved immediately

Bug: Issue 6023
Change-Id: I40b0b280129e5a76c13e38c97525f91a8e56482d
2017-12-05 13:20:22 -08:00
Becky Siegel
5baba4eef6 Fix draft comment in wrong spot
There was an issue with the sort function that returned 0 sometimes when
it should not have. This change adds an additional condition so that the
correct comparison is made.

Bug: Issue 7813
Change-Id: I5b405ae431edc0b37cef09c53a2e8bb786fdeeed
2017-11-27 16:44:21 -08:00
Wyatt Allen
c56ece73e2 Support creating comments on merge parents
Bug: Issue 4760
Change-Id: I66ced578819b6f48d7d1535a54f1debf0e35374e
2017-11-27 11:09:58 -08:00
Becky Siegel
8d92d53db5 Annotation updates
Change-Id: I146f76b9dcc1a92e18acec01481ad280fb431868
2017-08-12 11:49:52 -07:00
Wyatt Allen
f0eb4bbf53 Harden gr-formatted-text agains slow/failed project configs
Formerly, if a formatted text component tried to render without the
project config (used by inner linked text components) it would
temporarily fall-back to rendering the unformatted (and un-linkified)
text via `.textContent` -- mirroring the behavior of gr-linked-text.

The result is formatted text elements (when rendered without a project
config) appear as one long line of text. Unlike linkification, however,
text can be accurately formatted with or without the project config --
so this disruptive, poor UX is unnecessary.

The formatted text component is updated to format text when the project
config has not provided, and to re-render when the config has been
provided.

In order to propagate project config loads to the formatted text
components hosted by diff comments, the REST calls must be made by the
diff thread component. To make this call, the thread must have the
project's name, so gr-diff-builder is updated to provide this name to
thread components.

Bug: Issue 6686
Change-Id: I8d09c740930500e99cb5f87b92f4d72f3f50a9ce
2017-08-01 16:14:45 -07:00
Becky Siegel
64cede2a14 Fix delete button again
In the previous fix, the function was not called and since it was
the function that set the class to 'hidden' it did not hide.

Change-Id: Ic8f908802cb515086860f20e2c82d0ddd06c0278
2017-05-19 23:08:16 +00:00
Kasper Nilsson
7552b6f3c5 ES6ify /gr-diff-comment-thread/*
Bug: Issue 6179
Change-Id: Ib850e99830ff4a750a57e404d069d7bf364469fe
2017-05-16 16:49:20 -07:00
Viktar Donich
29e1ce5e84 Collection of prospective test flake fixes
Potentially related:
https://github.com/Polymer/web-component-tester/issues/505

Bug: Issue 5792
Change-Id: I9ab6e8e40d9811dd52906335426764c052907609
2017-03-30 13:46:58 -07:00
Becky Siegel
0727efa3d1 Implement isOnParent as functions rather than an attribute on comments
Previously, the rest api interface set '__isOnParent' for comments.
When comments were added, the property from the comment thread
'isOnParent' was passed as a property. When the draft was saved, it was
expecting '__isOnParent' rather than 'isOnParent' and this caused
comments to show up on the wrong side after saved/refreshed, because
'__isOnParent' was undefined.

Instead of changing to either '__isOnParent' which would be strange to
pass as an attribute or 'isOnParent' which would look like it came from
the api directly, this change introduces isOnParent functions so that
translation doesn't need to be done with the API.

Bug: Issue 5831
Change-Id: I3b849ba5878275cda0a39638626a12bd51341a29
2017-03-21 14:57:58 -07:00
Becky Siegel
cd2d4232f1 Reduce confusion between side and commentSide
Use '__isOnParent' as a boolean in place if 'side' ('PARENT vs
'REVISION').  In doing so, it's necessary to convert to/from 'side'
whenever interacting with the REST API.

Change-Id: Ic023c9be1969597e4b9c73a51cfed9f5eb9bc23e
2017-03-15 13:50:21 -07:00
Kasper Nilsson
b5a681c75b Make 'Ack' mark thread as resolved
Bug: Issue 5568
Change-Id: Id8dba066766b5cc8d943bcb5df877c6b66b73ec1
2017-02-16 11:05:30 -08:00
Becky Siegel
a88f8c82d4 Fix corner case where storage gets duplicated for patchsets
Previously, there was an issue where if you create a draft comment in
side by side view and switch to unified view, the comment thinks it's in
the later patch set rather than the earlier one and a second copy gets
added to local storage with the later patchset as a component of the
key.

This was because the the thread group assumed all threads inside of it
had the same patch number. This change fixes that, so in the event that
a user switches from side by side to unified, the patch number will get
taken from the comment rather than the thread group.

Bug: Issue 5493
Change-Id: I7f00997bcb2e6f1001a5d58ac206acf5af3367d2
2017-02-10 14:52:57 -08:00
Becky Siegel
2c602323e1 Move reply buttons to comment thread
Move all buttons that generate a reply of some sort (done, ack, reply,
quote) to the comment thread instead of the comment [1].

When there is a draft for a particular comment thread, all reply buttons
are hidden [2].  For example, if you click reply, you cannot click done
on the same thread, unless you remove the draft.

Each thread can have up to 1 draft. It's also worth noting that if a
thread has a draft, and the user clicks on the line or 'c' at the same
range, the existing draft will switch to 'editing' form.

[1] With the exception of "please fix" for robot comments.
[2] In this case, The please fix button will be disabled when other
reply buttons are hidden.

Feature: Issue 5410
Change-Id: Id847ee0cba0d0ce4e5b6476f58141866d41ffdad
2017-02-09 16:07:32 -08:00
Becky Siegel
1074e868b7 Get unresolved state from thread when it exists for new reply
Currently, when a response is created via keyboard shortcut 'c' or
clicking a line number, the comment is not created with the unresolved
state of the last comment in the thread.

This change checks for the previous state and adds that to the new
draft.

Bug: Issue 5408
Change-Id: I20eb039864120d5175cc016bfc695da564bc174d
2017-02-09 08:02:22 -08:00
Wyatt Allen
23929fc5ff Add null/undefined checks for comment ranges
Change-Id: Ia66529e44b047ca8938708110d75e7bf910e73ae
2017-02-08 10:29:04 -08:00
Wyatt Allen
4a9de6fa4d Merge "Don't merge threads on same line left/right" 2017-02-08 00:30:57 +00:00
Wyatt Allen
049a045247 Merge "Get tests passing in safari/firefox" 2017-02-07 21:18:31 +00:00
Becky Siegel
41bdc04cb8 Don't merge threads on same line left/right
Goes along with c/95273/. Adds commentSide attribute to comments to see
which side of the diff view they belong on. This is also used as part
of the locationRange for the gr-diff-comment-thread-group, so that two
thread groups can be on the same line or range for the unified group (
one for the right, one for the left).

Note: there is already a 'side' attribute on the gr-diff-comment, which
is confusing. This side actually references 'PARENT' or 'REVISION', to
identify whether the comment belongs to the parent or any revision. On
diffs where two revisions are compared to each other, this cannot be
used to determine left/right. However, because 'side' is part of
the CommentInfo entity[1], it is difficult to change the name and make
more sense out of that.

[1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-info

Bug: Issue 5114
Change-Id: I5cc4c17d4bb134e31e5cc07ff9b08ed349488c97
2017-02-07 21:00:52 +00:00
Becky Siegel
41e811942d Get tests passing in safari/firefox
Change-Id: I1d85a24ce12bcd2f556abc14e028d9926180b978
2017-02-06 18:15:14 -08:00
Becky Siegel
8c19a02e67 Fix range issue with comment replies
- Adds range when parent exists when comment POSTed
- Finds range via filtering comments for backfill

Bug: Issue 5459
Change-Id: Ied19cc4b33749ba81fad6d1a5030abab419b1e6f
2017-02-06 17:03:57 -08:00
Becky Siegel
562a30493e Allow multiple threads per line, with different ranges
- Add concept of diff comment thread groups, which are all of the
  threads at a particular line number.

- The thread group is responsible for breaking up comments into threads
  based on the range of the comment.

- Thread groups are ordered by the updated time of the first comment in
  the group.

- Thread groups are given a key, based on comment range, which is used
  to determine what thread group a new comment should go in (or if it
  needs a new one).

Feature: Issue 5292
Change-Id: If544e8bb879262de3ce5397e86124837b66ada04
2017-02-01 16:10:08 -08:00
Kasper Nilsson
87cb82f5a1 Expand unresolved comment threads in diff view by default
This change leverages the unresolved flag to automatically expand only
the last UNRESOLVED_EXPAND_COUNT comments in unresolved threads.

Feature: Issue 4752
Change-Id: Ia23920e1a210246838645d56a6bc81d0dff7da07
2017-01-10 18:42:37 +00:00
Kasper Nilsson
8d1ac7e824 Display comment resolve state
This change tracks and exposes the resolved state of a comment thread
without exposing the UI for modifying that state. This enables features
to be built out while the API request does not exist in the backend.

Feature: Issue 4879
Change-Id: If002035024920a7762519cedf5a869221bbbc3c8
2017-01-09 20:21:48 +00:00
Viktar Donich
d24fa4c371 Merge "Fixes comment issues with multiple editing comments" 2017-01-03 23:15:47 +00:00
Becky Siegel
43c5ed8045 Fixes comment issues with multiple editing comments
This change addresses a few issues that existed due to multiple comments
in an editing state at the same time.

1) Fixes issue where if you create two replies and add text to the
second reply, then delete the first one, the text in the second textarea
gets removed.

2) Fixes issue where if you reply and add text, then reply again with
the first draft still editing, the second draft gets populated with the
message from the first comment.

3) Fixes issue where if you have multiple replies and delete one of
them, local storage gets erased. This change sets local storage to the
value of the first editing message found after deleting the other one
(if it exists).

Bug: Issue 4409
Change-Id: Ib5913a34a79783a4a87b4a298e25b02fc587b8dd
2016-12-28 20:18:41 +00:00
Kasper Nilsson
9e8548b480 Populate in_reply_to field with most recent comment
This change fully removes threading from the PG UI's comments. This is
a side effect of the implementation of resolvable comments.

Change-Id: I67e2a8f896726787857c35552fa8c0a9f629c2ba
2016-12-27 17:48:38 -05:00
Kasper Nilsson
cc518249be Remove threading in comment UI
Sort inline comments in the order that the comments were written.
Formerly comments were sorted in tree order so that reply comments would
appear after their parent. However, this sorting could be confusing
because there would be no other indication that the comments were
associated to a parent. When comments are sorted chronologically, they
appear in the thread position a user would expect.

Change-Id: I4d0722e4511eb53b9b8ab1d61a922d1b142e09ad
2016-12-19 12:03:09 -08:00
Wyatt Allen
f362d246d1 Merge "Add robot comments to PolyGerrit" 2016-12-16 17:23:00 +00:00
Becky Siegel
35a7682262 Add robot comments to PolyGerrit
This change adds an API request to get robot comments for displaying
inline in the diff view. They are styled in a different color, contain
build and robotId information, and a "please fix" action rather than the
standard set of actions.

Feature: Issue 5089
Change-Id: I1f5954a2ed01920bb7c3dc897e3285687ff7d3ca
2016-12-15 11:20:25 -08:00
Kasper Nilsson
fa4b2fe374 Add check for modifiers in keyboard shortcut handlers
Bug: Issue 5079
Change-Id: I221bfcfc42e14159b5457d7177902b947906f75d
2016-12-09 08:36:59 -08:00
Aaron Gable
5be3252ead Add an 'Ack' button next to the 'Done' button
Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=622838
Change-Id: Ie7db71615e2c657bbf24847cd8e6e2fc7606f5a9
2016-11-30 09:46:48 -08:00
Andrew Bonventre
4d22c7e835 Cleanup: use iron-a11y-keys-behavior for keyboard shortcuts
+ This does not cover on-keydown handlers within elements.
  A follow-up change will account for those.
+ Keyboard shortcuts are disabled within gr-overlay, input,
  and textarea elements.
+ Added tests for new behavior (plus some missing ones covering
  broken behavior).
+ Removed blur hacks used on elements to placate the kb
  shortcuts due to restrictions that have been removed.

Bug: Issue 4198
Change-Id: Ide8009a3bfc340a35a8ec8b9189a85b49c8a95aa
2016-11-17 15:27:59 -08:00
Kasper Nilsson
682895271f Remove one space from indentation in comment quote
In GWTUI, quoted comments are prepended with ' > ' on each line. This
was causing strange formatting issues due to strings being trimmed on
the backend (see issue for more details), and also didn't really make
sense (who prefers 3 space tabs over 2 or 4?). This change removes the
leading space and denotes quote levels with '> '.

Bug: Issue 4811
Change-Id: Iee1a85b5b2f1a4ab46a35110be65a6b4f156eb6a
2016-11-10 10:31:14 -08:00
Logan Hanks
610f80d3ec Add namespacing for keyboard shortcut disabling
The gr-overlay element attempts to manage disabling and enabling
keyboard shortcuts. When multiple gr-overlay elements are available
on a page and one of them opens immediately, that overlay tries to
disable keyboard shortcuts, but the other elements initialize as
closed and enable them.

This change offers a new method for disabling keyboard shortcuts.
The caller can pass in an identifier to enable or disable. If keyboard
shortcuts are disabled by one or more identifiers, then they are
suppressed.

Change-Id: I82fe6efd922f09279e76a2f2c8cb5781f3afe395
2016-10-14 14:05:43 -07:00
Becky Siegel
eb4ea184f7 Keyboard shortcuts to expand and collapse all comments
This change adds keyboard shortcuts to the "gr-diff-comment-thread"
expand all comments when 'e' is pressed and collapse all comments when
'shift + e' is pressed. Note that the keyboard event is detected on the
thread instead of the comment to minimize the number of events getting
triggered.

Feature: Issue 4738
Change-Id: Iab77349bd1527d7af5e05a827919a78a86909835
2016-10-12 11:13:56 -07:00
Viktar Donich
a8df3ec075 Add orphan comments as top-level comments
Add comments that are replies to ones missing from comment thread as
top-level comments to the thread instead of discarding.

Change-Id: Ib93fa1c74f1ca004ee2361e017dc1210c42ec399
2016-06-15 16:34:15 -07:00
Viktar Donich
815e82d3c3 Update comment thread model on comment update
Update comments collection in gr-diff-comment-thread when its child
comment changes. Child comment is expected to send comment-update
event.

Change-Id: I8116820679b29b7511719f95ae73b48e2345102c
2016-06-15 09:07:17 -07:00
Viktar Donich
b2198e8233 Ranged comments integration
- gr-file-list recognizes local preferences (for hasRangedComments flag)
- gr-file-list reacts to cursor hotkey only if there is no range
  selected (currently always false).
- Remove dead code from GrDiffBuilderSideBySide, GrDiffBuilder,
  gr-diff-builder.html
- Bugfix: GrDiffBuilder.prototype.getGroupsByLineRange handles one-line
  BOTH code sections correctly. Test updated as well.
- Added utitily methods added to gr-diff-builder.html to reduce
  dependency on DOM structure and reduce amount of code copy-pasting:
  - renderLineRange, getContentByLine, etc
- For gr-diff.js and gr-diff-comment-thread.js addDraft renamed to
  addOrEditDraft because that's what it does.
- For both, addDraft method always creates a draft comment.
- Added support for ranged comments in gr-diff, gr-diff-comment-thread.
- Added mouseenter and mouseout events to gr-comment.js
- Refactored gr-comment.js to reduce code copy-paste, unify event
  payload, and to eliminate need of accessing component instance for
  patchNum. Tests updated as well.
- Refactored gr-diff.js UI data model update using gr-diff-builder.html
  utility methods to make code more readable.
- Added support for creating ranged comments to gr-diff.js.
- gr-selection-action-box now reacts to click and tap to create a 
  comment.

Change-Id: I01480a4c6f460774a8b2826915702800b3f81d25
2016-06-13 22:44:47 +00:00
Viktar Donich
7ad28920e9 Gr-diff retrofit: store comment state
Fixing UI data pipe line, re-rendering:
 - side, draft text and editing status in UI comment objects
 - update gr-diff UI model on comment save/update

Feature: Issue 3910
Change-Id: I96f714c7de9add6e316dcf64bb7d566690b9d3ae
2016-05-27 10:46:54 -07:00