• 
      

    Allow pasting of images to upload

    Review Request #10078 — Created July 10, 2018 and updated

    Information

    Review Board
    release-4.0.x
    ddb087d...

    Reviewers

    The user can now attach an image to a review request by pasting. This
    action opens a dialog box showing where the user is shown the image
    being uploaded and is able to add a caption for the image, or cancel the
    upload.

    
     
    Description From Last Updated

    So I stepped thorugh this and was able to figure out what the cause. THe way that browsers load images …

    brenniebrennie

    Can you add screenshots showing this off?

    chipx86chipx86

    Undo this.

    brenniebrennie

    So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as …

    brenniebrennie

    This will need a doc comment of the form /** * Handle a paste event. * * Args: * e …

    brenniebrennie

    We can combine this into: if ($(pasteEvent.target).is('textarea, input') || pasteEvent.clipboarData.files.length === 0) { return; }

    brenniebrennie

    This should use ===. See mdn for details.

    brenniebrennie

    Not a super helpful comment. Since we're checking file.type I think it is self explanatory.

    brenniebrennie

    This can be a single line, e.g. reader.onload = e => this._onPasteFinished(e, file);

    brenniebrennie

    blank line between these.

    brenniebrennie

    Lets not handle this case specifically. If we drop this case, the readAsArrayBuffer case should handle it properly.

    brenniebrennie

    no blank line here.

    brenniebrennie

    Combine these.

    brenniebrennie

    Combine these.

    brenniebrennie

    Remove this line.

    brenniebrennie

    How about we call this _onFilePasted? Likewise this will need a comment of the form: /** * Handle a file …

    brenniebrennie

    As I mentioned previously, the current approach requires us to click on the page to focus an element inside this …

    brenniebrennie

    Missing a blank line.

    brenniebrennie

    This should use const. We never want to use var in .es6.js files.

    brenniebrennie

    Here, too.

    brenniebrennie

    Remove this blank line.

    brenniebrennie

    This needs a doc comment. Likewise all methods in here.

    brenniebrennie

    Lets add primary: true to this.

    brenniebrennie

    Instead of this, we can call _.bindAll(this, '_onUploadClicked'); before the RB.DialogView.prototype.initialite.call(...) and change this to: onClick: this._onUploadClicked,

    brenniebrennie

    ] on next line.

    brenniebrennie

    Format as: attachment.save({ success: () => document.location.reload(), error: (model, xhr) => console.log($.parseJSON(xhr.responseText)), }); Also, we should probably replace consoel.log with …

    brenniebrennie

    Blank line between these.

    brenniebrennie

    How about: this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, })));

    brenniebrennie

    Combine these.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS.

    brenniebrennie

    We might want to render a generic "we could not generate a thumbnail" caption or something.

    brenniebrennie

    One too many blank lines.

    brenniebrennie

    Col: 9 Forgotten 'debugger' statement?

    reviewbotreviewbot

    Col: 37 Missing semicolon.

    reviewbotreviewbot

    Col: 29 Missing '()' invoking a constructor.

    reviewbotreviewbot

    Col: 80 Missing semicolon.

    reviewbotreviewbot

    Too many blank lines. There should only be 2.

    chipx86chipx86

    Too many spaces here.

    chipx86chipx86

    This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should …

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Too many spaces here.

    chipx86chipx86

    This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion …

    chipx86chipx86

    No blank lines here.

    chipx86chipx86

    This needs to describe what the function is doing.

    chipx86chipx86

    Indentation is all wrong, and the blank line should be removed.

    chipx86chipx86

    This console.log should go away. Also, blank line between statements and blocks.

    chipx86chipx86

    I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror. This needs …

    chipx86chipx86

    Too many spaces.

    chipx86chipx86

    Should have parens around the whole e => ... to help with readability, given the assignment.

    chipx86chipx86

    Missing a comment doc block.

    chipx86chipx86

    Indentation is all wrong.

    chipx86chipx86

    Missing a doc comment block.

    chipx86chipx86

    This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types …

    chipx86chipx86

    This is too long for the line.

    chipx86chipx86

    Indentation is broken all throughout.

    chipx86chipx86

    This should probably be a <th>, not a <td> with a <label>.

    chipx86chipx86

    The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding …

    chipx86chipx86

    The <tr> is empty, and the <img> isn't in a valid place. I'm having a hard time imagining how the …

    chipx86chipx86

    This needs to describe what this class is all about.

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    The options need to be described in an Option Args section. Grep around for examples.

    chipx86chipx86

    No blank line.

    chipx86chipx86

    No space around the =. Should be options={}.

    chipx86chipx86

    This needs to describe what the function does.

    chipx86chipx86

    No space before ().

    chipx86chipx86

    Space after :.

    chipx86chipx86

    There should only be one blank line at this level.

    chipx86chipx86

    No need to capitalize "dialog."

    chipx86chipx86

    Comment isn't formatted right.

    chipx86chipx86

    This isn't the class name.

    chipx86chipx86

    This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined …

    chipx86chipx86

    Blank line between block and statement.

    chipx86chipx86

    Missing , at the end of this.

    chipx86chipx86

    This should go up in the models section.

    chipx86chipx86
    mandeep
    brennie
    1. 
        
    2. Show all issues

      So I stepped thorugh this and was able to figure out what the cause. THe way that browsers load images is asynchronous. Just because we have the data: URI doesn't mean that Firefox/Chrome etc. know the dimensions of the image. It only knows those dimensions once we create the <img> and set the src attribtue. Then it starts parsing and loading the image.

      However, by then, we've already finished render() and the parent class will have resized itself with the original size of the image (which is 0 px wide and 0 px high). Once we finish rendering, the main thread is unblocked and the browser will start loading the image. Once the image has finished loading, it renders, but we've already resized so it looks wonky

      The following is a solution that preloads the image source in render:

      render() {
          const file = this.model.get('file');
      
          if (file.type.startsWith('image/')) {
              const uri = this.model.get('fileContent');
              const image = new Image();
      
              image.onload = () => this.$el.append($(imageTemplate({
                  fileContent: uri,
                  fileName: file.name,
              })));
      
              image.src = uri;
          } else if (...) {
              // ...
          }
      }
      
    3. reviewboard/cmdline/rbext.py (Diff revision 1)
       
       
      Show all issues

      Undo this.

    4. Show all issues

      So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as you have to click in the page to paste a file).

      Instead of doing this, check out my comment in render for an alternate suggestion:

    5. Show all issues

      This will need a doc comment of the form

      /**
       * Handle a paste event.
       *
       * Args:
       *  e (jQuery.Event):
       *    The event that triggered this handler.
       *
       */
      
    6. reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1)
       
       
       
       
       
       
       
      Show all issues

      We can combine this into:

      if ($(pasteEvent.target).is('textarea, input') ||
          pasteEvent.clipboarData.files.length === 0) {
          return;
      }
      
    7. Show all issues

      This should use ===. See mdn for details.

    8. Show all issues

      Not a super helpful comment. Since we're checking file.type I think it is self explanatory.

    9. Show all issues

      This can be a single line, e.g.

      reader.onload = e => this._onPasteFinished(e, file);
      
    10. Show all issues

      blank line between these.

    11. Show all issues

      Lets not handle this case specifically. If we drop this case, the readAsArrayBuffer case should handle it properly.

    12. Show all issues

      no blank line here.

    13. Show all issues

      Combine these.

    14. Show all issues

      Combine these.

    15. Show all issues

      Remove this line.

    16. Show all issues

      How about we call this _onFilePasted?
      Likewise this will need a comment of the form:

      /**
       * Handle a file being pasted.
       *
       * Args:
       *     readEvent (Event):
       *         The event from the `FileReader`.
       *
       *     file (File):
       *         The file that was pasted.
       */
      
    17. Show all issues

      As I mentioned previously, the current approach requires us to click on the page to focus an element inside this view before we can paste. However, we can do the following in render / remove

      $(window).on('paste', this._onPaste):
      

      We'll also have to add a remove() function:

      remove() {
          Backbone.View.prototype.remove.call(this);
          $(window).off('paste', this._onPaste);
      }
      

      To get around this-binding, we'll want to add '_onPaste' to the list of functions in _.bindAll() that is called in initialize().

    18. Show all issues

      Missing a blank line.

    19. Show all issues

      This should use const. We never want to use var in .es6.js files.

    20. Show all issues

      Here, too.

    21. Show all issues

      Remove this blank line.

    22. Show all issues

      This needs a doc comment. Likewise all methods in here.

    23. Show all issues

      Lets add primary: true to this.

    24. Show all issues

      Instead of this, we can call _.bindAll(this, '_onUploadClicked'); before the RB.DialogView.prototype.initialite.call(...) and change this to:

          onClick: this._onUploadClicked,
      
    25. Show all issues

      ] on next line.

    26. Show all issues

      Format as:

      attachment.save({
          success: () => document.location.reload(),
          error: (model, xhr) => console.log($.parseJSON(xhr.responseText)),
      });
      

      Also, we should probably replace consoel.log with alert.

    27. Show all issues

      Blank line between these.

    28. Show all issues

      How about:

      this.$el.append($(imageTemplate({
          fileContent: this.model.get('fileContent'),
          fileName: file.name,
      })));
      
    29. Show all issues

      Combine these.

    30. Show all issues

      Unnecessary.

    31. Show all issues

      Unnecessary.

    32. Show all issues

      Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS.

    33. Show all issues

      We might want to render a generic "we could not generate a thumbnail" caption or something.

      1. Like an alert?

      2. No, just some text in the dialog.

    34. Show all issues

      One too many blank lines.

    35. 
        
    mandeep
    Review request changed
    Bugs:
    Commit:
    cec4f08d6bd7f9caf5179fa4ab3c519c62e9f34a
    8141b49a08d17f9b9c3c7f66482bafb05f1fc9cb

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mandeep
    Review request changed
    chipx86
    1. 
        
    2. Show all issues

      Can you add screenshots showing this off?

    3. reviewboard/static/rb/css/pages/review-request.less (Diff revision 3)
       
       
       
       
       
       
      Show all issues

      Too many blank lines. There should only be 2.

    4. Show all issues

      Too many spaces here.

    5. Show all issues

      This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should be prefixe with "TODO:". However, if this is something that should be done, it should be done now.

      Comments, btw, should be in sentence casing, ending with a period, and should use /* .. */ style. So:

      /* TOD: Use classes for these. */
      
    6. Show all issues

      Blank line between these.

    7. Show all issues

      Too many spaces here.

    8. Show all issues

      This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion has a guide on this, and you can grep around for newer classes following the style.

    9. Show all issues

      No blank lines here.

    10. Show all issues

      This needs to describe what the function is doing.

    11. Show all issues

      Indentation is all wrong, and the blank line should be removed.

    12. Show all issues

      This console.log should go away.

      Also, blank line between statements and blocks.

    13. Show all issues

      I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror.

      This needs extremely thorough testing, and it needs a comment block explaining why we're testing this and the implications.

    14. Show all issues

      Too many spaces.

    15. Show all issues

      Should have parens around the whole e => ... to help with readability, given the assignment.

    16. Show all issues

      Missing a comment doc block.

    17. reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3)
       
       
       
       
       
       
       
       
      Show all issues

      Indentation is all wrong.

    18. Show all issues

      Missing a doc comment block.

    19. Show all issues

      This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types of files in the browser (text files, PDFs, etc.). So we should be more clear:

      "Thumbnail previews can only be generated for image files. The thumbnail will be generated after upload instead."

    20. Show all issues

      This is too long for the line.

    21. reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Indentation is broken all throughout.

    22. Show all issues

      This should probably be a <th>, not a <td> with a <label>.

    23. Show all issues

      The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding in variables as above. Doing it the way you have it now will break the template if any of the localized text returned from gettext isn't HTML-safe.

      This should be <%- captionLabel %> and that variable should be passed in when invoking below.

    24. Show all issues

      The <tr> is empty, and the <img> isn't in a valid place. I'm having a hard time imagining how the browser is rendering this table.

      Maybe it shouldn't even be a table?

    25. Show all issues

      This needs to describe what this class is all about.

    26. Show all issues

      Blank line between these.

    27. Show all issues

      Blank line between these.

    28. Show all issues

      The options need to be described in an Option Args section. Grep around for examples.

    29. Show all issues

      No blank line.

    30. Show all issues

      No space around the =. Should be options={}.

    31. Show all issues

      This needs to describe what the function does.

    32. Show all issues

      No space before ().

    33. Show all issues

      Space after :.

    34. Show all issues

      There should only be one blank line at this level.

    35. Show all issues

      No need to capitalize "dialog."

    36. Show all issues

      Comment isn't formatted right.

    37. Show all issues

      This isn't the class name.

    38. Show all issues

      This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined string for a rare case like this. Instead, do:

      $('<p>')
          .text(gettext('.....'))
          .appendTo(this.$el);
      
    39. Show all issues

      Blank line between block and statement.

    40. Show all issues

      Missing , at the end of this.

    41. reviewboard/staticbundles.py (Diff revision 3)
       
       
      Show all issues

      This should go up in the models section.

    42.