Image diffing

Review Request #3458 — Created Oct. 26, 2012 and discarded

Information

Review Board
master

Reviewers

- Models and views for image diffs
- Temporary URL for viewing image diffs

Implemented Two-up, Swipe, Onion and Difference views (see file attachments).
Supports diffing images of different sizes.

Future: Resize images that are too large?
Manually tested on Firefox & Chrome with images of the same size and images of different sizes.

Screenshots


Description From Last Updated

Two blank lines.

chipx86chipx86

This isn't valid.

chipx86chipx86

Always limit lines to < 80 chars.

chipx86chipx86

Space before = Do you need "new" and "2" ?

chipx86chipx86

Insert alphabetically.

chipx86chipx86

Same.

chipx86chipx86

I'm not sure why you changed this (and I believe it will break other UIs). Why did you namespace these …

daviddavid

Must be 2 spaces

AA aam1r

I am not sure if you need a new URL and function for image diffing. It seems like the "diff" …

AA aam1r

Must be 2 spaces

AA aam1r

Small nitpick but I would use 6-digit codes instead of 3 (for the sake of clarity). Writing #FF7777 is much …

AA aam1r

It might be better to use variables for these magic numbers to describe what these values are. The number "12" …

AA aam1r

This breaks pep8 max line length of 79 chars; you can break it down to: orig_file_attachment = get_object_or_404(FileAttachment, pk=orig_file_attachment_id) new_file_attachment …

SL slchen

Should this subclass ImageReviewableModel (imageReviewableModel.js) instead?

SL slchen

Should this subclass ImageReviewableView?

SL slchen

Shouldn't we return the base classe's extra context no matter what? Something like: extra_context = super(ImageReviewUI, self).get_extra_context(request) if self.orig_image: extra_context['orig_image'] …

SM smacleod

For all of these rules in here - I think we have a pattern in here we we alphabetically order …

mike_conleymike_conley

Nit: indented too far.

mike_conleymike_conley

You put documentation on the two functions above - you should do it for the rest of these as well.

mike_conleymike_conley

The formatting here looks a bit off - I think this needs to be indented more.

mike_conleymike_conley

Same as above.

mike_conleymike_conley

Nice use of promises. :)

mike_conleymike_conley

Nit - indentation

mike_conleymike_conley

Nit - indentation - plus, I think it needs a newline afterwards.

mike_conleymike_conley

Needs var

mike_conleymike_conley

Setting .src on an Image is asynchronous. I don't think you can trust that .width and .height is available on …

mike_conleymike_conley

Same as above wrt the race.

mike_conleymike_conley

Too much indentation

mike_conleymike_conley

Just wanted to say...this is a really cool function.

mike_conleymike_conley

Nit - spaces on either side of the +'s in here.

mike_conleymike_conley

This should be two lines. self.template_name should be on the same line as render_to_response.

chipx86chipx86

No need for parens.

chipx86chipx86

Can you do "XXX Temp URL ..."? This will show up styled in many editors, making it easier to see, …

chipx86chipx86

Indentation is still off here.

mike_conleymike_conley

These should probably be top-level. I thought we included a jquery-ui stylesheet. Do we not?

chipx86chipx86

Should inherit from FileAttachmentReviewable.

chipx86chipx86

If we're not doing anythign special here, you can remove this.

chipx86chipx86

This can grow out of control. I'd suggest doing something like; this._$image_diffs.children().hide(); this._$view_modes_list.children().removeClass('active');

chipx86chipx86

One var statement only, with each variable on its own line. For example: var image_1 = blah, image_2 = blah, …

chipx86chipx86

vars must be declared at the top of a function. (They can be assigned later). In reality, JavaScript moves vars …

chipx86chipx86

Should be: image_frame_2.css({ 'margin-left': blah, 'margin-right': blah }); Same with the ones below.

chipx86chipx86

Align parameters.

chipx86chipx86

Should do: this._$wipe_view .width(...) .height(...); Only reference the element once. You won't need to append "px" when doing this. It'll …

chipx86chipx86

Should be two lines, with the first parameter on the first line.

chipx86chipx86

Use dictionary form: self.blah .css({ opacity: percent, filter: ... });

chipx86chipx86

Do: this._$onion_view .width(...) .height(...); No "px"

chipx86chipx86

Two lines total.

chipx86chipx86

Why is the text-align an inline style?

mike_conleymike_conley
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
     
     
    I have a change up for review that adds registration. It also adds a basic ImageReviewUI. I recommend creating a branch, applying my change, and basing your work on that one.
  3. reviewboard/reviews/ui/image.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  4. reviewboard/reviews/ui/image.py (Diff revision 1)
     
     
    This isn't valid.
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
     
    Always limit lines to < 80 chars.
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Space before =
    
    Do you need "new" and "2" ?
  7. reviewboard/settings.py (Diff revision 1)
     
     
    Insert alphabetically.
  8. reviewboard/settings.py (Diff revision 1)
     
     
  9. 
      
MI
MI
MI
david
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 3)
     
     
     
    I'm not sure why you changed this (and I believe it will break other UIs). Why did you namespace these instead of just using the orig_image directly?
    1. This was the only way I could figure out how to get it to work. Leaving it the way it was before with get_extra_content returning a dictionary containing orig_image threw an exception saying that the RequestContext was receiving an unexpected argument. Could you show me how I should be adding orig_image?
    2. I suspect that there was some bug but it's hard to guess without seeing exactly what happened. Can you put that code up and pastebin the error?
    3. Error is at http://dpaste.com/hold/829535/
      "Exception Value: __init__() got an unexpected keyword argument 'orig_image'"
      
      Changed the extra context to be an argument of RequestContext instead of an entry in the dictionary.
      
      Could you explain how this would break other UIs? It looks like my code is the only one so far that is overriding get_extra_context. Also, from what I can tell based on some light reading of RequestContext, additional arguments should be related to context processors?
    4. Okay, I think I see what the original intent of this was supposed to be. Let's change it to look like this:
      
      context = {
          'base_template': ...
      }
      context.extend(self.get_extra_context(request))
      
      return render_to_response(
          self.template_name,
          RequestContext(request, context))
  3. 
      
MI
AA
  1. 
      
  2. reviewboard/reviews/ui/image.py (Diff revision 4)
     
     
    You can also write this as:
    
    if self.orig_image:
  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Must be 2 spaces
  4. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     
     
     
    I am not sure if you need a new URL and function for image diffing. It seems like the "diff" function in reviews/views.py finds the review request, gets all the diffsets and then uses the most recent one. 
    
    I am not sure if it can be done but you might be able to do that in the Review UI itself. That would let you cut back the new URL pattern you introduced as well as this function.
    1. I did this was testing purposes at the start. Could you explain how the "diff" function could work instead?
    2. Oh, hmm. I misunderstood what "diff" was doing before when I opened the issue. You're doing a lot more custom stuff, like getting the original file attachment, which I overlooked.
      
      Sorry about that -- you can drop this issue.
  5. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Must be 2 spaces
  6. reviewboard/static/rb/css/reviews.less (Diff revision 4)
     
     
    Small nitpick but I would use 6-digit codes instead of 3 (for the sake of clarity). Writing #FF7777 is much more understandable than #FF7. It will also become consistent with all your other color codes (which are all 6-digit)
  7. It might be better to use variables for these magic numbers to describe what these values are. The number "12" and "30" are repeated often and using a variable will make it much easier for you to modify these values.
  8. 
      
MI
MI
MI
MI
MI
SL
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    This breaks pep8 max line length of 79 chars; you can break it down to:
    
        orig_file_attachment = get_object_or_404(FileAttachment,
                                                 pk=orig_file_attachment_id)
        new_file_attachment = get_object_or_404(FileAttachment,
                                                pk=new_file_attachment_id)
  3. Should this subclass ImageReviewableModel (imageReviewableModel.js) instead?
    1. I didn't subclass imageReviewableModel.js because I wasn't ready to support viewing or adding comments.
  4. Should this subclass ImageReviewableView?
  5. 
      
MI
MI
SM
  1. This looks awesome. Any chance you can post screenshots of the different views when the image remains the same size?
  2. reviewboard/reviews/ui/image.py (Diff revision 8)
     
     
     
     
     
     
    Shouldn't we return the base classe's extra context no matter what? Something like:
    
    extra_context = super(ImageReviewUI, self).get_extra_context(request)
    
    if self.orig_image:
        extra_context['orig_image'] = self.orig_image
    
    return extra_context
    1. Good point. Base class returns empty extra content atm so I didn't think of this.
  3. 
      
mike_conley
  1. Michelle:
    
    This looks *awesome*. I'm really excited to use it. :)
    
    -Mike
  2. reviewboard/static/rb/css/reviews.less (Diff revision 8)
     
     
    For all of these rules in here - I think we have a pattern in here we we alphabetically order the properties in each rule.
  3. reviewboard/static/rb/css/reviews.less (Diff revision 8)
     
     
     
     
    Nit: indented too far.
  4. You put documentation on the two functions above - you should do it for the rest of these as well.
  5. The formatting here looks a bit off - I think this needs to be indented more.
  6. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
    Nice use of promises. :)
    1. Gotta thank stack overflow for showing me how to do this. :)
  7. Nit - indentation - plus, I think it needs a newline afterwards.
  8. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    Setting .src on an Image is asynchronous. I don't think you can trust that .width and .height is available on your images until the images have at least started loading.
    
    I know that, in this case, you'd be fetching images from the cache, but that's still asynchronous, and you're still going to have a tiny race.
    
    You might want to use promises again here.
    1. I actually just forgot to change this to grab the width from the DOM element. The images should already be loaded by the time this method gets called.
  9. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    Same as above wrt the race.
  10. Too much indentation
  11. Just wanted to say...this is a really cool function.
  12. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 8)
     
     
     
     
     
     
     
     
    Nit - spaces on either side of the +'s in here.
  13. 
      
MI
MI
mike_conley
  1. 
      
  2. reviewboard/static/rb/css/reviews.less (Diff revision 9)
     
     
     
     
    Indentation is still off here.
  3. Why is the text-align an inline style?
  4. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 9)
     
     
     
     
    This should be two lines. self.template_name should be on the same line as render_to_response.
  3. reviewboard/reviews/ui/image.py (Diff revision 9)
     
     
    No need for parens.
  4. reviewboard/reviews/urls.py (Diff revision 9)
     
     
    Can you do "XXX Temp URL ..."? This will show up styled in many editors, making it easier to see, since this change will probably start off in a pushed branch for now.
  5. reviewboard/static/rb/css/reviews.less (Diff revision 9)
     
     
     
     
     
    These should probably be top-level.
    
    I thought we included a jquery-ui stylesheet. Do we not?
    1. It isn't included; my sliders don't render properly if I don't include these rules.
  6. reviewboard/static/rb/js/models/imageDiffReviewableModel.js (Diff revision 9)
     
     
     
     
     
     
     
    Should inherit from FileAttachmentReviewable.
  7. If we're not doing anythign special here, you can remove this.
  8. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
    This can grow out of control. I'd suggest doing something like;
    
    this._$image_diffs.children().hide();
    this._$view_modes_list.children().removeClass('active');
  9. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 9)
     
     
     
     
     
     
     
     
    One var statement only, with each variable on its own line. For example:
    
    var image_1 = blah,
        image_2 = blah,
        image_width_1 = blah;
  10. vars must be declared at the top of a function. (They can be assigned later).
    
    In reality, JavaScript moves vars under the hood to the top anyway, so this is to reflect it and prevent certain types of problems.
    
    Can you go through the rest of the code and make sure the vars behave the correct ways? One var per scope level, defined at the top of the scope.
  11. Should be:
    
    image_frame_2.css({
        'margin-left': blah,
        'margin-right': blah
    });
    
    Same with the ones below.
  12. Align parameters.
  13. Should do:
    
    this._$wipe_view
        .width(...)
        .height(...);
    
    Only reference the element once.
    
    You won't need to append "px" when doing this. It'll handle that itself.
  14. Should be two lines, with the first parameter on the first line.
  15. Use dictionary form:
    
    self.blah
        .css({
            opacity: percent,
            filter: ...
        });
  16. reviewboard/static/rb/js/views/imageDiffReviewableView.js (Diff revision 9)
     
     
     
     
     
     
    Do:
    
    this._$onion_view
        .width(...)
        .height(...);
    
    No "px"
  17. Two lines total.
  18. 
      
MI
MI
chipx86
  1. Hi Michelle,
    
    Just a question that came up while we were discussing this change offline. Is the logic for your image diffing based on any existing code out there? If so, do you know what the license is to that code?
    1. Almost everything was reverse-engineered by looking at what Github had and tweaking with CSS to make it happen. There's the bit of CSS that was copy-pasted from the jQuery UI CSS. I did read up on Difference blending from multiple places, and saw some Ruby code that helped me understand it. Don't know if that counts.
      
      http://jeffkreeftmeijer.com/2011/comparing-images-and-creating-image-diffs/
    2. Okay, so no GitHub code was taken then? Just looking at the feature set? I think if the code from jQuery UI and from that blog post are the only bits of actual code taken, it should be okay.
      
      (This is only about getting it included in the codebase, since we just have to be careful with licensing issues and such. Either way, there's no reflection on your project/grade :)
    3. Thought you'd like to know that this is finally landing for the big 2.0 release! I'm actively working to integrate it with our new binary file diff support.
  2. 
      
MI
Review request changed

Status: Discarded

Change Summary:

Reworked and closed in favor of /r/4493/
Loading...