Add support for image diffs.

Review Request #4493 — Created Aug. 27, 2013 and submitted

Information

Review Board
master

Reviewers

Add support for image diffs.

This greatly enhances the ImageReviewUI to support showing diffs between
images. When generating a binary diff between images, the Review UI will
provide several modes for viewing how the images changed.

The first mode is the Two-Up mode, which just shows the two images
side-by-side.

The second is the Difference mode, which shows a color-based difference
between the images. It's useful for spotting what pixels actually
changed between images, and what remained the same.

The third is the Split mode, which layers the two images and provides a
slider for showing how much of the modified image is visible, vs. the
original. The user can slide back-and-forth to see what changed between
the images.

The fourth is the Onion Skin mode, which layers the two images and
provides a transparency slider. This slider will alter the transparency
of the modified image. This makes it easy to see what pixels may have
moved, or other subtle changes to the image.

All modes allow commenting, and all comments are displayed on all modes.
The comment thumbnail for image diffs will show the commented regions of
the original and modified images side-by-side (similar to the Two-Up
mode).
Tested commenting on each of these modes.

Tested commenting outside the region of one image or another, and made
sure that didn't blow up (particularly when generating thumbnails).

Tested that thumbnails showed up correctly in the review dialog and in
published reviews.

Tested diffing images of different sizes.

Tested all the functionality for traditional image file attachments.

I have *not* tested with very large images. I expect this to be painful
to use, but have some plans to improve very large images in this review UI
in general, so I'm okay with that for now.

Description From Last Updated

This is getting very boxy. Could we maybe move the "Download" buttons up next to the revision names somehow?

daviddavid

What if a particular view has some controls that use images? Should we have a specific class here?

daviddavid

<=? How about === and we log an error if it's <

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/ui/image.py
        reviewboard/settings.py
      Ignored Files:
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/css/image-review-ui.less
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/models/imageReviewableModel.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/reviews/ui/image.py
        reviewboard/settings.py
      Ignored Files:
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/css/image-review-ui.less
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/models/imageReviewableModel.js
    
    
  2. 
      
chipx86
  1. I'm not 100% happy with the name "Split." It's what things like Kaleidoscope and GitHub use, but I don't think it conveys the feature that well. I'm thinking of "Curtain." Is that better or worse?
    1. I'm ok with either.
  2. 
      
david
  1. 
      
  2. Show all issues
    This is getting very boxy. Could we maybe move the "Download" buttons up next to the revision names somehow?
    1. Yeah, that's a future change. I think I mentioned this in the change that introduced the Download links, but they're temporary.
  3. Show all issues
    What if a particular view has some controls that use images? Should we have a specific class here?
    1. They can override render and not call the default in that case. The view that shows just one image does that, for example.
      
      It's a hypothetical right now, so I don't want to over-engineer it, but I think any view that wants non-default behavior can just not use the default behavior.
  4. Show all issues
    <=? How about === and we log an error if it's <
  5. 
      
chipx86
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/ui/image.py
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/css/image-review-ui.less
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/models/imageReviewableModel.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/reviews/ui/image.py
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/css/image-review-ui.less
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/models/imageReviewableModel.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed