Modernize the display of the "Review request changed" boxes.

Review Request #5499 — Created Feb. 18, 2014 and submitted

Information

Review Board
master
a2f9c31...

Reviewers

The "Review request changed" boxes haven't been touched since they were
first introduced back in 2006 or 2007. Since then, we've added file
attachments, improved the information available on diffs, introduced
Markdown, and added extension-provided fields. All of which the old
boxes did a terrible job at representing.

This introduces a complete overhaul of the design and capabilities of
these boxes. Every bit of information is now more detailed, making it
easier for people to see exactly what has changed at a glance.

Text area fields, like Description, are now rendered as Markdown, but
presented in a unified diff format. It's easy to see what changed in the
text, and to even see small changes like a corrected word within a line.
This works much like the diff viewer, except not side-by-side.

Simpler text fields, like Branch show the before and after, one on top
of the other, helping to see what changed. These are also presented in a
diff-like view, though much smaller. By placing one above the other,
instead of one to the side of the other, it's easier to see what has
changed.

Lists share the same design as the text fields, except there's one row
per removed entry and one per added.

Diffs are greatly improved. An uploaded diff still shows the revision, a
link to the diff, and the "Show changes" link. Along with that, it now
shows the raw insert/delete count, and a partial file listing, complete
with complexity icons. If the diff hasn't been viewed yet, the
complexity icons just show the proportion of inserts/deletes, but after
it's viewed and the lines are calculated, it'll show the same
information that the diff viewer shows.

File attachments now show the thumbnails for the files that were added
or removed. These are just like the ones on the review request file
listing, but without the editing capabilities. They do have reviewing
capabilities, though, so a user can immediately begin reviewing the
added files without going to the main file listing.

Simple text fields (Branch, Summary, Commit, etc.):

  • Changed from blank to a value
  • Changed from a value to another value
  • Changed from a value to blank
  • For Commit, I changed values by posting new diffs.

List fields (Depends On, Reviewers):

  • Same general tests as with simple text fields
  • Deleted multiple items, and saw one per line.
  • Added multiple items, and saw one per line.
  • Saw the review request summaries for Depends On.

Bugs field:

  • Removed/added multiple entries, and saw only one add and one remove line.
    The bugs were comma-separated on a single line, which is nicer, since
    bug IDs are generally smaller.

Text area fields (Description, Testing Done):

  • Entered lots of Markdown, including code samples, links, and images. Saw them
    all render.
  • Changed all those types of content, including changes within code samples
    and changes to lines with images. The formatting didn't break, and the
    changes were properly represented.
  • Changed words in a line, and saw the changed regions highlighted.

Screenshot/file attachment captions:

  • Opened review requests with caption changes, and saw that they were displayed
    correctly, and links all went to the right places.

File attachments:

  • Uploaded file attachments. Saw the thumbnails. I couldn't edit them, but I could
    review them.
  • Tested the Review link and the new Comment link.
  • For the New Comment link, I created a comment and saw the draft banner. Published
    and saw it appear in the review.

Diffs:

  • Uploaded a diff and saw it appear with the information and a file listing.
  • Checked all the links, including those on the file listing.
  • Verified that the order was the same as in the diff viewer, and that file anchors
    were correct even when the visible order changed.
  • Before viewing the diff, I saw that the complexity icons just displayed insert/delete
    counts.
  • After viewing the diff, the icons showed replace lines, and thickness. They were still
    generally in the same proportions as the original icons, with the replace just taking
    parts away from the insert/delete regions.

Status changes:

  • Closed review requests as discarded.
  • Closed review requests as submitted.
  • Re-opened review requests.

Change Description field:

  • Saw the change description text when I entered it.

General:

  • Python unit tests pass.
  • JavaScript unit tests pass.

Description From Last Updated

Can you include the IDs in here? (since that's what's displayed in the review request)

daviddavid

I think I'd like it if the description box was aligned with the others. The description doesn't really need to …

daviddavid

Leading spaces (at least 1 of them) are unnecessary.

daviddavid

filename should be escaped here.

daviddavid

There's no reason to include extra newlines in the output.

daviddavid

These should be escaped, just in case a localization causes issues.

daviddavid

The text should be escaped here.

daviddavid

This should be escaped.

daviddavid

This should be escaped.

daviddavid

There's no reason to include extra newlines in the output.

daviddavid

Can you move this to be defined just above the for loop?

daviddavid

This comment doesn't really add very much.

daviddavid

This would be both fewer lines of code and more efficient as a for loop that appended values.

daviddavid

This would be both fewer lines of code and more efficient as a for loop that appended values.

daviddavid

It was already this way, but this doesn't need the newlines.

daviddavid

Why not import these at the top of the file?

daviddavid

!== false seems like a funky comparison. Can we either use === true or just use truthiness? Maybe even better, …

daviddavid

If options isn't passed in, shouldn't $thumbnail be set to something else?

daviddavid

Same question here about canEdit.

daviddavid

Should this have a colon, to be consistent?

daviddavid

One leading space here is unnecessary.

daviddavid

PEP-8 isn't going to like this alignment. Can you put the first arguments on the next line?

daviddavid
david
  1. Issue 2045?

  2. 
      
david
  1. 
      
  2. Show all issues

    Can you include the IDs in here? (since that's what's displayed in the review request)

  3. Show all issues

    I think I'd like it if the description box was aligned with the others. The description doesn't really need to use the extra width because it's not that wide in the review request box (and no wider than the summary).

    "Description" is also missing a colon.

    1. I went back and forth on this, but ultimately like the proposal. I'll just go over my thoughts I had when writing this, for historical purposes.

      The model I was going for was to have the primary fields that behave like the "main" section of the review request (label and value on separate lines, with no ":" on the label), and secondary inline fields that behave more like the right-hand side of the review request. I wanted the appearance of such fields to be the same on the change descriptions as they are on the review request.

      I changed the Description and File Attachments fields to be of the inline type, and I think it looks fine. I'm tempted to get rid of the other mode, but I'd need to rethink file caption changes, because they look terrible in that mode. I'm not really sold on the look of them anyway, but I think for now, I'll keep that other mode and just change the text areas and file attachments.

  4. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Leading spaces (at least 1 of them) are unnecessary.

    1. It's indented relative to the <table>, for help when viewing the source, but I'll change all this.

  5. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    filename should be escaped here.

  6. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    There's no reason to include extra newlines in the output.

  7. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
     
     
    Show all issues

    These should be escaped, just in case a localization causes issues.

  8. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    The text should be escaped here.

  9. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    This should be escaped.

  10. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    This should be escaped.

  11. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    There's no reason to include extra newlines in the output.

  12. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    Can you move this to be defined just above the for loop?

  13. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
     
    Show all issues

    This comment doesn't really add very much.

    1. I can elaborate, but I meant that it's cheaper than using render_to_string or equivalent.

  14. reviewboard/reviews/fields.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    This would be both fewer lines of code and more efficient as a for loop that appended values.

    1. It's actually a lot less efficient to do the manual loop + append. Twice as slow.

      += is the direct equivalent of extend, which loops through an iterable and runs append on each one. Except that it's handled internally in Python, and is much more efficient.

      As a test for this, I wrote two scripts. One that appends 100 items to a list using append, and repeats that test 500,000 times. Another that appends to a list using += and a list comprehension handling 100 items. The first was twice as slow.

      It's 1 line more than the +=, but I think it's also more readable. It focuses on the loop and what we're doing with each item, and saves a visible append() and a wrapping of the render call. Plus the efficiency.

    2. The more you know.

  15. reviewboard/reviews/fields.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    This would be both fewer lines of code and more efficient as a for loop that appended values.

  16. reviewboard/reviews/fields.py (Diff revision 1)
     
     
    Show all issues

    It was already this way, but this doesn't need the newlines.

  17. reviewboard/reviews/fields.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Why not import these at the top of the file?

    1. Oops, thought I cleared this out. This was when I was experimenting.

  18. Show all issues

    !== false seems like a funky comparison. Can we either use === true or just use truthiness? Maybe even better, do something like if (_.result(this.options.canEdit))?

    1. Nope. canEdit is the default, and if not provided, then we want to allow it. Comparing against true or using _.result wouldn't give us what we need here. Doing a !== false may seem weird, but is not uncommon JavaScript, and is equivalent to === true || === undefined.

  19. Show all issues

    If options isn't passed in, shouldn't $thumbnail be set to something else?

    1. It'll be undefined, which is what we want. We check that case below and construct a thumbnail.

  20. Show all issues

    Same question here about canEdit.

  21. Show all issues

    Should this have a colon, to be consistent?

    1. Going back to my earlier comment (the first one actually).

      The existence of the colon was following the model used on the review request box. Colon only if something was to the immediate right.

      I can change this. Just not sure which I want to be consistent with. My gut feeling is to keep it consistent with the review request page. If I add one here, then "File Captions" should have one, but it's a little more of a section header since things under it can have their own entries with colons.

    2. I think for things which can be considered headers, they can not have a colon, and everything else (labels) should. I don't see a ton of value in trying to be consistent with colons between this and the review request page, because the way the data is presented and the layout is completely different.

    3. Okay. There's nothing in there to distinguish between the case of headers vs. labels, since they're the same thing here.

      I'll go ahead and just add the colon to everything.

  22. 
      
chipx86
chipx86
chipx86
david
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    One leading space here is unnecessary.

  3. reviewboard/reviews/builtin_fields.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    PEP-8 isn't going to like this alignment. Can you put the first arguments on the next line?

  4. 
      
chipx86
david
  1. Ship It!

  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...