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

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. 
      
Loading...