Closed Bug 1115707 Opened 10 years ago Closed 8 years ago

Add mode to show comments inline in reviewboard

Categories

(MozReview Graveyard :: General, defect, P1)

Product:
MozReview Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ekr, Assigned: glob)

References

Details

Attachments

(12 files, 10 obsolete files)

mozreview: don't compress javascript inside the dev environment (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Don't ignore .orig or .rej files (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
use querySelector instead of hand-rolled code (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
mozreview: add inline-comments js, html, css (bug 1115707);
58 bytes, text/x-review-board-request
davidwalsh
: review+
smacleod
: review+
Details
add inline-comments admin setting (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
enable inline comments via admin setting (bug 1115707)
58 bytes, text/x-review-board-request
smacleod
: review+
Details
clone for mozreview modification (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
clone for mozreview modification (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
clone for mozreview modification (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
clone for mozreview modification (bug 1115707);
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Add toggle for inline comment visibility (bug 1115707)
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Problem.gif
1.16 MB, image/gif
Details 
Splinter and Rietveld show your actual comments on the code inline. Reviewboard just shows a number at the side. This is harder to read. I get others may like this, but it would be nice to have an option.
This will be fixed in core at some point, but we might fix it ourselves first.
Priority: -- → P3
We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.
Priority: P3 → P4
Product: Developer Services → MozReview
This is now a priority for the MozReview team, per the reviewer panel we talked to over the last couple months.
Assignee: nobody → glob
Priority: P4 → P1
Attachment #8787186 - Attachment is obsolete: true
Attachment #8787187 - Attachment is obsolete: true
Depends on: 1299796
Depends on: 1299797
Attachment #8787188 - Attachment is obsolete: true
Attachment #8787189 - Attachment is obsolete: true
Attachment #8787190 - Attachment is obsolete: true
Attachment #8787191 - Attachment is obsolete: true
Attachment #8787192 - Attachment is obsolete: true
Attachment #8787193 - Attachment is obsolete: true
Attachment #8787194 - Attachment is obsolete: true
sorry about the bug spam - i accidental pushed to the wrong review repo.
i've changed my scripts so that shouldn't happen again.
Attachment #8787205 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 8787202 [details]
mozreview: don't compress javascript inside the dev environment (bug 1115707);

https://reviewboard.mozilla.org/r/76064/#review83042
Attachment #8787202 - Flags: review?(smacleod) → review+
Comment on attachment 8787203 [details]
Don't ignore .orig or .rej files (bug 1115707);

https://reviewboard.mozilla.org/r/76066/#review83044
Attachment #8787203 - Flags: review?(smacleod) → review+
Comment on attachment 8792422 [details]
clone for mozreview modification (bug 1115707);

https://reviewboard.mozilla.org/r/79494/#review83046
Attachment #8792422 - Flags: review?(smacleod) → review+
Comment on attachment 8787204 [details]
use querySelector instead of hand-rolled code (bug 1115707);

https://reviewboard.mozilla.org/r/76068/#review83050

I was super skeptical of this since row vs line in RB and diffs are very different things. The vocabulary here makes it more confusing than it ought to be. Nice simplification :)
Attachment #8787204 - Flags: review?(smacleod) → review+
Comment on attachment 8792423 [details]
clone for mozreview modification (bug 1115707);

https://reviewboard.mozilla.org/r/79496/#review83052
Attachment #8792423 - Flags: review?(smacleod) → review+
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review83054

This looks really good :), I'm going to consider this a tentative r+, as I haven't actually manually tested things yet. I was thinking when you're back / have rebased I'll immediatly test it quite a bit and then we can get this thing landed ASAP.

::: reviewboard/reviewboard/static/rb/js/views/inlineCommentView.js:166
(Diff revision 2)
> +        }
> +        // Keep track of the rows this comment refers to.
> +        var beginLineNum = this.$beginRow.attr('line');
> +        var endLineNum = this.$endRow.attr('line');
> +        this.contextRows = [];
> +        for (var lineNum = beginLineNum; lineNum <= endLineNum; lineNum++) {

You know your brain is fried when you go "Wat? what the hell is an arrow function with the arrow backwards!?"... heh

::: reviewboard/reviewboard/static/rb/js/views/inlineCommentView.js:178
(Diff revision 2)
> +        // Show as much comment context as possible.
> +        // We can't accurately measure this as the comment is within the
> +        // column we'd need to measure.  table/4 results in a reasonable
> +        // result without causing the column size to change.
> +        this.$el
> +            .find('.inlineComment .comment')
> +            .width(this.$el.parents('table').width() / 4);
> +    },

:davidwalsh, any better ideas for this?

::: reviewboard/reviewboard/staticbundles.py:238
(Diff revision 2)
>              'rb/js/views/screenshotThumbnailView.js',
>              'rb/js/views/imageReviewableView.js',
>              'rb/js/views/dummyReviewableView.js',
>              'rb/js/views/textBasedCommentBlockView.js',
>              'rb/js/views/textBasedReviewableView.js',
> +            'rb/js/views/inlineCommentView.js',

Can you please include a mozreview in the name like the ones we fork. As much as I doubt RB will ever get an "inlineCommentView.js" upstream, you never know, heh.
Attachment #8787206 - Flags: review?(smacleod) → review+
Comment on attachment 8787207 [details]
add inline-comments admin setting (bug 1115707);

https://reviewboard.mozilla.org/r/76074/#review83056
Attachment #8787207 - Flags: review?(smacleod) → review+
Comment on attachment 8792424 [details]
clone for mozreview modification (bug 1115707);

https://reviewboard.mozilla.org/r/79498/#review83058
Attachment #8792424 - Flags: review?(smacleod) → review+
Comment on attachment 8787208 [details]
enable inline comments via admin setting (bug 1115707)

https://reviewboard.mozilla.org/r/76076/#review83060

::: reviewboard/reviewboard/templates/diffviewer/view_diff_mozreview.html:33
(Diff revision 2)
> +{# MozReview: add inline comments templates #}
> +{% if siteconfig_settings.diffviewer_show_comments_inline %}
> +{%  include "diffviewer/inline_comments.html" %}
> +{% endif %}
> +<div id="show_comments_inline"
> + data-enabled="{% if siteconfig_settings.diffviewer_show_comments_inline %}true{% else %}false{% endif %}">

`{{siteconfig_settings.diffviewer_show_comments_inline|yesno:"true,false"}}`
Attachment #8787208 - Flags: review?(smacleod) → review+
Comment on attachment 8792425 [details]
clone for mozreview modification (bug 1115707);

https://reviewboard.mozilla.org/r/79500/#review83062
Attachment #8792425 - Flags: review?(smacleod) → review+
Comment on attachment 8792426 [details]
Add toggle for inline comment visibility (bug 1115707)

https://reviewboard.mozilla.org/r/79502/#review83064

Want to file a pre-emptive bug about making the toggling better (or allow individual contract / expand etc)?
Attachment #8792426 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #45)
> Want to file a pre-emptive bug about making the toggling better (or allow
> individual contract / expand etc)?

filed bug 1309121 and bug 1309122
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review83494

I get the feeling I've done something wrong but I'm seeing the following:

http://image.prntscr.com/image/280d4f2c728e422fa0aa17d5ea525654.png

I'm seeing 4 `td`/`th` elements within the inline comment row but each code row only has 2 `td`/`th` elements.

Please advise as to what I could be doing wrong.
Attachment #8787206 - Flags: review?(dwalsh) → review-
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review85668

Tried checking this out but seeing the following bug, so I cannot even see it in practice:  https://bugzilla.mozilla.org/show_bug.cgi?id=1311165
Attachment #8787206 - Flags: review?(dwalsh) → review-
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review85668

i don't understand why this patch got an r- due to unrelated breakage of your development environment.
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review85668

Either it was r- before (which it doesn't look like) or there's a bug that set it to that, because I certainly didn't do so manually.
Attachment #8787206 - Flags: review- → review?(dwalsh)
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review86026

I tried this and first -- well done!  I bet this was really tough to put together!

So I have two issues:

1.  Seeing the inline comments at a glance is really, really tough.  The light grey color doesn't do much to call attention to itself, and now that there's no comment icon to the left of the line, you really need a sharp eye.  Maybe a light blue color would be easier to see?  Maybe a larger font?

2.  The first time I add a comment to an existing comment, new comment displays right under the existing comment.  When I then try to add another comment to another existing comment, it does *not* immediately display under the existing comment, though it does appear to save properly.  Will try to add video.
Attachment #8787206 - Flags: review?(dwalsh) → review-
Attached image Problem.gif — 
Video of second new inline comment not displaying...
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review83054

> :davidwalsh, any better ideas for this?

I think it may be possible to add a CSS class with "width: 25%" instead of this calculation -- that may make it more responsive, less taxing.
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review83054

> I think it may be possible to add a CSS class with "width: 25%" instead of this calculation -- that may make it more responsive, less taxing.

unfortunately this doesn't work; the table column ends up being sized too large, resulting in unbalanced columns.
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review86816

(All diff comments in this review are left on bogus lines, so that I can open issues. Also, they don't necessarily apply to this commit, but I've grouped them here anyway, sorry!)

I spent some time messing around with this and it was pretty good overall (plus it *felt* much snappier... hmmm :D). I have a couple of nits and one case that I think is a more serious bug. But this is pretty much all I was able to find, overall it's looking really good.

::: reviewboard/reviewboard/static/rb/css/pages/diffviewer_mozreview.less:838
(Diff revision 4)
>    margin-top: 1em;
>  }
>  
> +
> +/****************************************************************************
> + * Inline Comments

![Background Bleed](https://people.mozilla.org/~smacleod/mozreview-inline-comments/css-bleed-down.png "Background Bleeding")

When the last line of your comment is on the final line of a chunk the background colour of that chunk bleeds down into the left column. I feel like the actual comment background should go fully across, this is definitely a nit though (feel free to drop).

That being said, if we're going to keep it like this, we should somehow shift the chunk border down to the bottom of the bleed (notice the grey line between the lines of red on the left, that's the chunk delimiter and won't show if your comment is in the middle of a chunk.

This also get's a little weird when you mouseover the comment, as the colour of the bleed doesn't update like the rest of the comment area:

![With Mouseover](https://people.mozilla.org/~smacleod/mozreview-inline-comments/bleed-mouseover.png "Background Bleeding Mouseover")

::: reviewboard/reviewboard/staticbundles.py:1
(Diff revision 4)
>  from __future__ import unicode_literals

![Euqal Chunk Border](https://people.mozilla.org/~smacleod/mozreview-inline-comments/equal-chunk-border-style.png)

The border for comments ending in equal chunks is darker, and seems like the mouseover styling for comments in change chunks.

It's particularly strange when you mouseover one of these equal chunks - the border looks a little screwed up along the bottom:

![Euqal Chunk Mouseover Border](https://people.mozilla.org/~smacleod/mozreview-inline-comments/equal-chunk-mouseover-chunky-border.png)

::: reviewboard/reviewboard/staticbundles.py:2
(Diff revision 4)
>  from __future__ import unicode_literals
>  

This is probably the only non-nit bug I found (and I worry it will be quite difficult to fix, hopefully I'm wrong).

![Comments Out Of Order](https://people.mozilla.org/~smacleod/mozreview-inline-comments/comment-order-wrong.png)

The events leading up to this screenshot are the following, in order:
 - Comment A is left on the line, and published.
 - Comment C is left as a reply to A (In the reviews page) and published.
 - Comment B is left on the same line as a distinct review and published.

So the new comment which is part of a different thread (B) is injected between a comment (A) and a reply to that comment (C). The thing is, this order isn't even chronological... so I'm not sure what's going on here.

We should probably make sure we group the replies and threading properly (possibly even make separate threads more distinct, rather than just stacking them all the same way).
Attachment #8787206 - Flags: review+ → review-
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review86816

> ![Background Bleed](https://people.mozilla.org/~smacleod/mozreview-inline-comments/css-bleed-down.png "Background Bleeding")
> 
> When the last line of your comment is on the final line of a chunk the background colour of that chunk bleeds down into the left column. I feel like the actual comment background should go fully across, this is definitely a nit though (feel free to drop).
> 
> That being said, if we're going to keep it like this, we should somehow shift the chunk border down to the bottom of the bleed (notice the grey line between the lines of red on the left, that's the chunk delimiter and won't show if your comment is in the middle of a chunk.
> 
> This also get's a little weird when you mouseover the comment, as the colour of the bleed doesn't update like the rest of the comment area:
> 
> ![With Mouseover](https://people.mozilla.org/~smacleod/mozreview-inline-comments/bleed-mouseover.png "Background Bleeding Mouseover")

fixed by setting the colour of the left column to match the inline comment background color.

> ![Euqal Chunk Border](https://people.mozilla.org/~smacleod/mozreview-inline-comments/equal-chunk-border-style.png)
> 
> The border for comments ending in equal chunks is darker, and seems like the mouseover styling for comments in change chunks.
> 
> It's particularly strange when you mouseover one of these equal chunks - the border looks a little screwed up along the bottom:
> 
> ![Euqal Chunk Mouseover Border](https://people.mozilla.org/~smacleod/mozreview-inline-comments/equal-chunk-mouseover-chunky-border.png)

i fixed the border colours friday, but didn't push up a revision (didn't want to break review in progress, sorry).

for the hover, i changed from using outline to top/bottom black borders.
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review86816

> This is probably the only non-nit bug I found (and I worry it will be quite difficult to fix, hopefully I'm wrong).
> 
> ![Comments Out Of Order](https://people.mozilla.org/~smacleod/mozreview-inline-comments/comment-order-wrong.png)
> 
> The events leading up to this screenshot are the following, in order:
>  - Comment A is left on the line, and published.
>  - Comment C is left as a reply to A (In the reviews page) and published.
>  - Comment B is left on the same line as a distinct review and published.
> 
> So the new comment which is part of a different thread (B) is injected between a comment (A) and a reply to that comment (C). The thing is, this order isn't even chronological... so I'm not sure what's going on here.
> 
> We should probably make sure we group the replies and threading properly (possibly even make separate threads more distinct, rather than just stacking them all the same way).

investigating.  weirdly i can't reproduce this yet; i see the comments rendered chronologically.
/me cranks up the debugger
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review86816

> investigating.  weirdly i can't reproduce this yet; i see the comments rendered chronologically.
> /me cranks up the debugger

fixed by tracking threads; replies are indented.
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review88124
Attachment #8787206 - Flags: review?(dwalsh) → review+
Comment on attachment 8787206 [details]
mozreview: add inline-comments js, html, css (bug 1115707);

https://reviewboard.mozilla.org/r/76072/#review88460

Wooo :D
Attachment #8787206 - Flags: review?(smacleod) → review+
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/43d600ce9dc6
mozreview: don't compress javascript inside the dev environment ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/0889e939ea4c
Don't ignore .orig or .rej files ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/f3e9a0773c87
clone for mozreview modification ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/06fae359f047
use querySelector instead of hand-rolled code ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/b57df012c25b
clone for mozreview modification ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/b0a07815bc31
mozreview: add inline-comments js, html, css ; r=davidwalsh,smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/9e8f50c9b366
add inline-comments admin setting ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/c4e8fa8c4882
clone for mozreview modification ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/69f535236cad
enable inline comments via admin setting r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/b37fc16455b6
clone for mozreview modification ; r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/d6f138ebc3c7
Add toggle for inline comment visibility r=smacleod
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: