Code Dump: High DPI Image Review

Review Request #7754 — Created Nov. 4, 2015 and discarded

Information

Review Board
master

Reviewers

Code Dump: High DPI Image Review

11/04/2015

WIP of the drop down menu for selecting image scale.
(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)


11/07/2015

Resolved menu positioning problem (Problem # 1)
Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

(RESOLVED - 11/07/2015) Problem # 2 - I put a console.log(this.$el); into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.


11/13/2015

Re-formatted menu html code to be more readable.
Added event binding to the menu options.
Hard coded the event's function to increase image size by 200%.

NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.


11/15/2015

Full menu scaling capabilities added for ImageAttachmentView.
Minor rearrangement of code.
Change menu text color from blue to black.


11/21/2015

WIP on getting scaling to work on OnionDiff mode. It is proving to be more challenging than imageAttachmentView mode because it looks like I'll have to change the CSS of multiple classes rather than just one.

Problem # 3 - When there are diffs, I can't get the menu to appear in line with the image file names. I also tried putting the menu in the bar with the mode selections but that is giving me positional problems too.

Moved var statement for $headerInner to top of function.
Standardized the formatting of function documentation.
Removal of unnecessary lines and statements.


11/28/2015

Full menu scaling capabilities added for ImageTwoUpDiffView and ImageOnionDiffView.
Moved var statement for $headerInner to within a main var statement.
Renamed imgResSel to imgResolutionMenu.


12/03/2015

Full menu scaling capabilities added for all views.
Added a flag, this._isFirstRender, which ensures that the original image dimensions will only be calculated and cached once when user enters the diff modes.
Renamed variables to suit JS naming conventions (changed underscore case to camel case).
Removed redundant parseFloats within functions which were already taking in floats.

Ensured that images are resizing correctly on the browser.


Description From Last Updated

Something is wrong with that tab header. I've attached a screenshot that shows the difference between it and the regular …

brenniebrennie

Can this text be black instead of blue?

brenniebrennie

How about #img-resolution-menu

brenniebrennie

These should be alphabetized.

brenniebrennie

I have a few concerns about this. This is rather unreadable. It would be more readable if we broke it …

brenniebrennie

var statements go at the top of the function

brenniebrennie

Likewise here. We also avoid doing self = this style bindings and prefer using _.bind instead. However, this doesn't appear …

brenniebrennie

/*

brenniebrennie

Needs docstring.

brenniebrennie

Remove blank line.

brenniebrennie

Single var statement at the top of the function. We should be caching the original width and height on the …

brenniebrennie

Currently, each time you press the button, the image is going to be twice as large as the previous press, …

brenniebrennie

Should be formatted as: /* * Single line summary. * * Multi-line description. */

brenniebrennie

Remove this line.

brenniebrennie

See above.

brenniebrennie

Remove this line.

brenniebrennie

Isn't this in your other patch? If so, please remove this.

brenniebrennie

Remove this line.

brenniebrennie

This can all be done in initialize.

brenniebrennie

You should cache the image result, e.g. var img = $('.image-diff-attachment'), width = img.width(), height = img.height(); Also, how does …

brenniebrennie

JavaScript doesn't support multiple returns.

brenniebrennie

Remove this line.

brenniebrennie

Can we change this to changeScale and have it only take the scale? We should be able to get width …

brenniebrennie

If we cache the image on the object as _$img in initialize, we can do: this._$img.css({ width: this._width * scale, …

brenniebrennie

Remove this.

brenniebrennie

Lets call this data-image-scale.

brenniebrennie

ES5 doesn't support destructuring.

brenniebrennie

Needs a doc comment. Can we call it _onChangeScaleClicked ?

brenniebrennie

Please replace this with var scale = parseFloat($(e.target).data('iamge-scale')); This ensures we get the correct attribute (image-scale) vs the first attribute …

brenniebrennie

This needs a better name.

brenniebrennie

This should be one var statement, e.g. var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'), captionItems = [], $header, $headerInner, $revisionLabel, $revisionSelector;

brenniebrennie

Can we not use self = this here? CAn you replace all functions that use self to use this and …

brenniebrennie

How about: $headerInner = $('<div class="review-ui-header-inner" />') .html(this.imgResSel) .appendTo($header);

brenniebrennie

Undo this.

brenniebrennie

Blank line between these.

brenniebrennie

We don't want to append to $header until its ready to display, so we should do: $headerInner = $('...'); if …

brenniebrennie

Blank line between these.

brenniebrennie

Use /* ... */ for multi-line comments.

brenniebrennie

Use /* ... */ for multi-line comments.

brenniebrennie

This should probably return something

daviddavid

Missing a space between * and Changes.

daviddavid

We combine all var assignments into a single statement. That said, it's probably better to do this: var $image = …

daviddavid

We should require that people calling this pass in a float, rather than calling parseFloat here.

daviddavid

Undo this, please.

daviddavid

These should all be a single statement. Also, variable names in javascript code should be origWidth rather than orig_width.

daviddavid

Leftover debug output?

daviddavid

Same here re: parseFloat usage.

daviddavid

Undo this, please.

daviddavid

Multi-line comments should be formatted as: /* * The default ... * original ... * carries ... */

daviddavid

I'd really prefer to not hard-code information about the views here. Since the dimensions of the source images should be …

daviddavid

Use === instead of ==.

daviddavid

Oh, you're already calling parseFloat here, so you can just remove it from the changeScale implementations.

daviddavid

Same here re: multi-line comments. That said, this comment doesn't add a ton of info (maybe where you define this._dimensions …

daviddavid
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
CH
brennie
  1. 
      
  2. Show all issues

    How about #img-resolution-menu

  3. Show all issues

    These should be alphabetized.

    1. Is this what you mean?
      list-style-type: lower-alpha;

    2. He meant alphabetize the rules (float, then list-style, then margin, then padding, then width, then z-index).

    3. Gotcha. Fixed.

  4. Show all issues

    I have a few concerns about this.

    1. This is rather unreadable. It would be more readable if we broke it out into a string on the class and then we can use that to render the menu. Using a template also allows for easy extensibility later on.

    2. The <a> items lack an easy to use selector to hook into. It is probably best to use the same class on each <a> and store the desired resolution in a data- attribute.

    3. The ID #imgrevnav doesn't really explain what this does. This should be something like #img-resolution-selector.

    4. You don't need to use a nested <ul> structure. The menu wrapper can just be a <div>. This will simplify your CSS quite a bit.

    5. $imageScale is also not very descriptive. $imageScaleMenu would be better.

    6. There is no var statement for $imageScale.

    An example of all the suggestions above follows:

    {
        html: [
            '<div id="#img-resolution-selector">',
            ' <a href="#">',
            '  <span class="fa fa-search-plus"></span>',
            '  <span id="#img-resolution-current-zoom>100%</span>',
            ' </a>',
            ' <ul>',
            '  <li><a href="#" class="change-img-resolution" data-resolution="0.33">33%</span></li>',
            '  <li><a href="#" class="change-img-resolution" data-resolution="0.50">50%</span></li>',
            '  <li><a href="#" class="change-img-resolution" data-reolsution="1.0">100$</span></li>',
            ' </ul>',
            '</div>'
        ]).join('')
    }
    
    // ...
    
    $imageScaleMenu = $(this.html).appendTo($header);
    
    1. That should read 100% not 100$ above.

  5. 
      
CH
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
CH
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
CH
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    Something is wrong with that tab header. I've attached a screenshot that shows the difference between it and the regular header:

    screenshot

    1. Not sure what's happening here. Will look into it further.

  3. Show all issues

    Can this text be black instead of blue?

  4. Show all issues

    var statements go at the top of the function

    1. Header disappears when this particular var statement is moved to the top.

    2. You can declare the variable an instantiate it here. It must be at the top.

  5. Show all issues

    Likewise here. We also avoid doing self = this style bindings and prefer using _.bind instead. However, this doesn't appear to be doing anything.

  6. Show all issues

    /*

  7. Show all issues

    Needs docstring.

  8. Show all issues

    Remove blank line.

  9. Show all issues

    Single var statement at the top of the function.

    We should be caching the original width and height on the object (in initialize).

  10. Show all issues

    Currently, each time you press the button, the image is going to be twice as large as the previous press, and each time you press it should get bigger.

  11. 
      
CH
CH
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
CH
brennie
  1. 
      
  2. Show all issues

    Should be formatted as:

    /*
     * Single line summary.
     *
     * Multi-line description.
     */
    
  3. Show all issues

    Remove this line.

  4. Show all issues

    See above.

  5. Show all issues

    Remove this line.

  6. Show all issues

    Isn't this in your other patch? If so, please remove this.

    1. This would mean every jQuery reference to image-diff-attachment in my code for this project would become image-diff-null for things to work properly, and then we'd have to write a patch to fix this after the other patch goes live (presumably before this one).

      I'm new to web dev so I'm having trouble wrapping my head around the whole parallel timeline.

    2. Please ignore previous reply. I will rebase r/7754 off of master.

  7. Show all issues

    Remove this line.

  8. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
     
     
     
     
     
     
    Show all issues

    This can all be done in initialize.

    1. I initially thought about that, but some modes (e.g. ImageDifferenceDiffView) involve multiple images, so getting the starting dimensions can get messy if it's all done in initialize. This is why it may be better to have this as a function in the base class which subclasses can override and then have initialize call the function.

      Right now, the render function calls getStartingSize(), but I can move that into initialize. Thoughts?

    2. render is a fine place to put it.

  9. Show all issues

    You should cache the image result, e.g.

    var img = $('.image-diff-attachment'),
        width = img.width(),
        height = img.height();
    

    Also, how does this.$el relate to $('.image-diff-attachment')?

    We should probably cache the img on the object with this.$('img')

  10. Show all issues

    JavaScript doesn't support multiple returns.

  11. Show all issues

    Remove this line.

  12. Show all issues

    Can we change this to changeScale and have it only take the scale? We should be able to get width and height from this.

    1. Would the function have to take this: changeSize: function(size, this) {...

      Or can I call this._width and this._height within the function without doing that?

  13. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    If we cache the image on the object as _$img in initialize, we can do:

    this._$img.css({
       width: this._width * scale,
       height: this._height * scale
    });
    

    which is much more succinct and extensible.

  14. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Remove this.

  15. Show all issues

    Lets call this data-image-scale.

  16. Show all issues

    ES5 doesn't support destructuring.

  17. Show all issues

    Needs a doc comment.

    Can we call it _onChangeScaleClicked ?

  18. Show all issues

    Please replace this with

    var scale = parseFloat($(e.target).data('iamge-scale'));
    

    This ensures we get the correct attribute (image-scale) vs the first attribute on the element, as well as parsing it into a float so we can use it in computations.

  19. 
      
CH
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    This needs a better name.

  3. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    This should be one var statement, e.g.

    var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'),
        captionItems = [],
        $header,
        $headerInner,
        $revisionLabel,
        $revisionSelector;
    
  4. Show all issues

    Can we not use self = this here?

    CAn you replace all functions that use self to use this and call _.bind(fn, this) with them?

  5. Show all issues

    How about:

    $headerInner = $('<div class="review-ui-header-inner" />')
        .html(this.imgResSel)
        .appendTo($header);
    
    1. The scaling menu disappeared when I tried this.

    2. Well, it ought to work, so something is broken.

  6. Show all issues

    Undo this.

  7. Show all issues

    Blank line between these.

  8. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Show all issues

    We don't want to append to $header until its ready to display, so we should do:

    $headerInner = $('...');
    
    if () {
       // ...
    }
    
    $headerInner
        .html(this.imgResSel)
        .appendTo($header);
    
  9. Show all issues

    Blank line between these.

  10. Show all issues

    Use /* ... */ for multi-line comments.

  11. Show all issues

    Use /* ... */ for multi-line comments.

  12. 
      
CH
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    This should probably return something

    1. This base class function is used via overrides to return an array of length 2 for ImageAttachmentView,
      and an array of length 4 for the diff modes, so I'm unsure about what it should return here.

      I saw that the onImagesLoaded function just before it was also left empty (since other classes override
      it as well), so I followed that.

  3. Show all issues

    Missing a space between * and Changes.

  4. Show all issues

    We combine all var assignments into a single statement. That said, it's probably better to do this:

    var $image = this.$('.image-diff-attachment');
    return [$image.width(), $image.height()];
    
  5. Show all issues

    We should require that people calling this pass in a float, rather than calling parseFloat here.

  6. Show all issues

    Undo this, please.

  7. Show all issues

    These should all be a single statement. Also, variable names in javascript code should be origWidth rather than orig_width.

  8. Show all issues

    Leftover debug output?

  9. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
    Show all issues

    Same here re: parseFloat usage.

  10. Show all issues

    Undo this, please.

  11. Show all issues

    Multi-line comments should be formatted as:

    /*
     * The default ...
     * original ...
     * carries ...
     */
    
  12. reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues

    I'd really prefer to not hard-code information about the views here. Since the dimensions of the source images should be constant, can't we just get it once and not have to re-fetch it here?

    1. Agreed. This is also a problem because when you return to 'two-up' mode AFTER changing the scale in another mode, the new dimensions become the new 100%, and we don't want that happening. I'm thinking of ways of getting and caching the dimensions once.

  13. Show all issues

    Use === instead of ==.

  14. Show all issues

    Oh, you're already calling parseFloat here, so you can just remove it from the changeScale implementations.

  15. Show all issues

    Same here re: multi-line comments. That said, this comment doesn't add a ton of info (maybe where you define this._dimensions explain what the potential type(s) are?).

  16. 
      
CH
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
david
Review request changed
Status:
Discarded
Change Summary:

Discarded in favor of /r/8216/