Allow pasting of images to upload

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

mandeep
Review Board
release-4.0.x
3359
ddb087d...
reviewboard
brennie

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

    Undo this.

  4. 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. 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)
     
     
     
     
     
     
     

    We can combine this into:

    if ($(pasteEvent.target).is('textarea, input') ||
        pasteEvent.clipboarData.files.length === 0) {
        return;
    }
    
  7. This should use ===. See mdn for details.

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

  9. This can be a single line, e.g.

    reader.onload = e => this._onPasteFinished(e, file);
    
  10. blank line between these.

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

  12. 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.
     */
    
  13. 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().

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

  15. Remove this blank line.

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

  17. Lets add primary: true to this.

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

        onClick: this._onUploadClicked,
    
  19. 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.

  20. Blank line between these.

  21. How about:

    this.$el.append($(imageTemplate({
        fileContent: this.model.get('fileContent'),
        fileName: file.name,
    })));
    
  22. Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS.

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

  24. One too many blank lines.

  25. 
      
mandeep
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mandeep
Review request changed
chipx86
  1. 
      
  2. Can you add screenshots showing this off?

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

    Too many blank lines. There should only be 2.

  4. Too many spaces here.

  5. 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. Blank line between these.

  7. Too many spaces here.

  8. 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. No blank lines here.

  10. This needs to describe what the function is doing.

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

  12. This console.log should go away.

    Also, blank line between statements and blocks.

  13. 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. Should have parens around the whole e => ... to help with readability, given the assignment.

  15. Missing a comment doc block.

  16. reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     

    Indentation is all wrong.

  17. Missing a doc comment block.

  18. 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."

  19. This is too long for the line.

  20. reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Indentation is broken all throughout.

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

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

  23. 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?

  24. This needs to describe what this class is all about.

  25. Blank line between these.

  26. Blank line between these.

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

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

  29. This needs to describe what the function does.

  30. There should only be one blank line at this level.

  31. No need to capitalize "dialog."

  32. Comment isn't formatted right.

  33. This isn't the class name.

  34. 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);
    
  35. Blank line between block and statement.

  36. Missing , at the end of this.

  37. reviewboard/staticbundles.py (Diff revision 3)
     
     

    This should go up in the models section.

  38. 
      
Loading...