1101611 - Review comments in diff view should be more visible

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: User Interface, defect, P4)

Description

[(not currently active) Ted Mielczarek]

Right now when you add a comment to the diff view and click save it appears only as a tiny number in the left margin, like here:
https://reviewboard.mozilla.org/r/499/diff/

At first I thought my comment had been eaten by gremlins, but the info bar at the top of the page was enough to assuage me there. It would be nice to have a more visible indicator that there's a comment there, however.

Compare Rietveld's presentation:
https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processor/microdump.h#new-line-84

Here, comments are displayed inline, but collapsed to a single line that can be expanded on click.

Comment 1

[Mark Côté [:mcote]]

This should be fixed upstream.  Is there a bug?

Comment 2

[Mark Côté [:mcote]]

On second thought, I'll reopen.  We can leave this open and track any issues that are filed against upstream.

smacleod says that there is some agreement that the current UI isn't great but that there haven't been any good alternatives proposed yet.  In particular, they don't like the rietvald way because it breaks up the code, especially if there are a lot of comments on a particular line.  It also isn't obvious if a comment applies to several lines as it is with the current UI.

Comment 3

[Steven MacLeod [:smacleod]]

I don't think there is any particular bug about this upstream, it's more just known by the core team and we have it on a students project list.

Comment 5

[:glob ✱]

Attachment: mock up

as a stop gap measure until upstream come up with a solution, how about drawing a single line across the diff to draw attention to the comments?

implementation is a single line of js:
$('.commentflag').parents('tr').css('border-top', '2px dotted #444');

Comment 6

[:glob ✱]

smacleod - thoughts?

Comment 7

[Mauro Doglio [:mdoglio]]

Attachment: orange_comment_bubble.jpg

Alternative quickfix proposal: change the background color of the comment bubble to something more visible (orange in this mockup)

Comment 8

[:glob ✱]

(In reply to Mauro Doglio [:mdoglio] from comment #7)
> Alternative quickfix proposal: change the background color of the comment
> bubble to something more visible (orange in this mockup)

the problem i have with that is, especially on wide screens, a small box in the left margin is something you need to know to look for.  the brighter colour is better than what we currently have, but i think it needs something more to draw a new user's eye towards it.

Comment 9

[Steven MacLeod [:smacleod]]

(In reply to Byron Jones ‹:glob› from comment #5)
> Created attachment 8720136 [details]
> mock up
> 
> as a stop gap measure until upstream come up with a solution, how about
> drawing a single line across the diff to draw attention to the comments?
> 
> implementation is a single line of js:
> $('.commentflag').parents('tr').css('border-top', '2px dotted #444');

This seems like a fine temporary fix to me (although implementation wise couldn't we just do that with a css selector?)

Comment 10

[:glob ✱]

(In reply to Steven MacLeod [:smacleod] from comment #9)
> This seems like a fine temporary fix to me (although implementation wise
> couldn't we just do that with a css selector?)

unfortunately not; you can't select an element based on its children, and we need to style the <tr> that contains the .commentflag element.

Comment 11

[:glob ✱]

Attachment: MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

Review comments appear only as a tiny number in the left margin.  This makes
them easy to overlook, especially when viewing side-by-side diffs where your
focus is on the right column.  This adds a dotted line across the diff view
to draw attention to the comments.

Review commit: https://reviewboard.mozilla.org/r/36001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36001/

Comment 12

[Mauro Doglio [:mdoglio]]

Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36001/#review32655

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:47
(Diff revision 1)
> +.comment-block-container {

can you please attach a screenshot?

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:4
(Diff revision 1)
> +  // Adjust the styling of comment blocks to draw attention to their
> +  // existance.  Unfortunately the template is opaque to us, so we
> +  // have to resort to MutationObserver shenanigans.

Is this because we don't know when the diff will be rendered? If so, you can append a task to the `diff_files` funcQueue:
```js
$.funcQueue('diff_files').add(function(){
  // your js code
}
```
[Here](https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0d4ebd4464bd7889ea8c28670fc2c1e9f76c8aee/pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js#22) is a working example in mozreview.

Comment 13

[:glob ✱]

https://reviewboard.mozilla.org/r/36001/#review32655

> can you please attach a screenshot?

it's identical to the mock-up on the bug - https://bug1101611.bmoattachments.org/attachment.cgi?id=8720136

Comment 14

[:glob ✱]

https://reviewboard.mozilla.org/r/36001/#review32655

> Is this because we don't know when the diff will be rendered? If so, you can append a task to the `diff_files` funcQueue:
> ```js
> $.funcQueue('diff_files').add(function(){
>   // your js code
> }
> ```
> [Here](https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0d4ebd4464bd7889ea8c28670fc2c1e9f76c8aee/pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js#22) is a working example in mozreview.

i've been exploring this.

'diff_files' is ok, but it's triggered after all the diffs have been received, so on reviews that touch a lot of files there's a noticeable delay in rendering, and the content is shifted down one pixel for each line we draw.  mutation events occur immediately.

another issue is i cannot find any event that is triggered when a comment is added to a review.

how do you feel about going with mutation events, and i'll pitch this change upstream where it can be implemented correctly?

Comment 15

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/36001/#review32655

> i've been exploring this.
> 
> 'diff_files' is ok, but it's triggered after all the diffs have been received, so on reviews that touch a lot of files there's a noticeable delay in rendering, and the content is shifted down one pixel for each line we draw.  mutation events occur immediately.
> 
> another issue is i cannot find any event that is triggered when a comment is added to a review.
> 
> how do you feel about going with mutation events, and i'll pitch this change upstream where it can be implemented correctly?

Yeah sounds good to me

Comment 16

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/36001/#review32655

> it's identical to the mock-up on the bug - https://bug1101611.bmoattachments.org/attachment.cgi?id=8720136

I wanted to add a comment to the screenshot :p

Comment 17

[:glob ✱]

https://reviewboard.mozilla.org/r/36001/#review32655

> I wanted to add a comment to the screenshot :p

i spoke with mauro about this yesterday - he proposed using the same colours as the comments instead of black (ie. blue for comments, green for drafts).  it's a great improvement and i'll do that in the next revision.  when i push that revision up i'll attach an updated screenshot.

> Yeah sounds good to me

https://hellosplat.com/s/beanbag/tickets/4192/

if they don't like it, i'll propose triggering an event when a comment is added or removed (either during the initial render, or after the page is loaded) that we can listen for.

Comment 18

[:glob ✱]

Attachment: MozReview Request: mozreview: fix indentation (bug 1101611) r?mdoglio

Review commit: https://reviewboard.mozilla.org/r/36547/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36547/

Comment 19

[:glob ✱]

Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36001/diff/1-2/

Comment 20

[Mauro Doglio [:mdoglio]]

Comment on attachment 8723424 [details]
MozReview Request: mozreview: fix indentation (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36547/#review33365

Comment 21

[Mauro Doglio [:mdoglio]]

Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36001/#review33367

Comment 22

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/2d5d156dcb24

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/cc7bde07b24c9e0ce56830a2d4f2bf61be52d144
Backed out changeset 2d5d156dcb24 (bug 1101611)

Comment 24

[Gregory Szorc [:gps]]

I had to back this out due to deployment failure:

TASK: [Generate static Review Board files] ************************************
failed: [reviewboardadm.private.scl3.mozilla.com] => {"changed": true, "cmd": ["/data/reviewboard/src/reviewboard.mozilla.org/venv/bin/rb-site", "manage", "/data/reviewboard/src/reviewboard.mozilla.org/reviewboard", "--", "collectstatic", "--noinput"], "delta": "0:00:09.498140", "end": "2016-03-01 22:30:45.967927", "rc": 1, "start": "2016-03-01 22:30:36.469787", "warnings": []}
stderr: Traceback (most recent call last):
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/bin/rb-site", line 11, in <module>
    sys.exit(main())
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 1922, in main
    command.run()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 1848, in run
    site.run_manage_command(args[0], args[1:])
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 676, in run_manage_command
    execute_from_command_line([__file__, cmd] + params)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/__init__.py", line 399, in execute_from_command_line
    utility.execute()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/__init__.py", line 392, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 242, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 285, in execute
    output = self.handle(*args, **options)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 415, in handle
    return self.handle_noargs(**options)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 173, in handle_noargs
    collected = self.collect()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 119, in collect
    for original_path, processed_path, processed in processor:
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/storage.py", line 32, in post_process
    packager.pack_stylesheets(package)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 94, in pack_stylesheets
    variant=package.variant, **kwargs)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 103, in pack
    paths = self.compile(package.paths, force=True)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 97, in compile
    return self.compiler.compile(paths, force=force)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 55, in compile
    return list(executor.map(_compile, paths))
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/_base.py", line 581, in result_iterator
    yield future.result()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/_base.py", line 405, in result
    return self.__get_result()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 40, in _compile
    outdated=outdated, force=force)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/less.py", line 22, in compile_file
    return self.execute_command(command, cwd=dirname(infile))
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 99, in execute_command
    raise CompilerError(stderr)
pipeline.exceptions.CompilerError: NameError: variable @STATIC_ROOT is undefined in /data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/static/mozreview/css/viewdiff.less on line 1, column 21:
1 @import (reference) "@{STATIC_ROOT}rb/css/defs.less";
2
stdout: Copying '/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/static/rb/css/syntax.css'
Copying '/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/static/rb/css/pages/image-review-ui.css'

Comment 25

[:glob ✱]

> NameError: variable @STATIC_ROOT is undefined

urgh, it works in our dev/test env, but not on production.
i'll inline the colours.

Comment 26

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/e256ef207893