• 
      

    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/