Allow pasting of images to upload

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

mandeep
Review Board
release-4.0.x
cec4f08...
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.



  • 9
  • 0
  • 24
  • 0
  • 33
Description From Last Updated
So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as ... brennie brennie
This will need a doc comment of the form /** * Handle a paste event. * * Args: * e ... brennie brennie
Lets not handle this case specifically. If we drop this case, the readAsArrayBuffer case should handle it properly. brennie brennie
As I mentioned previously, the current approach requires us to click on the page to focus an element inside this ... brennie brennie
This needs a doc comment. Likewise all methods in here. brennie brennie
Instead of this, we can call _.bindAll(this, '_onUploadClicked'); before the RB.DialogView.prototype.initialite.call(...) and change this to: onClick: this._onUploadClicked, brennie brennie
How about: this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, }))); brennie brennie
Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS. brennie brennie
We might want to render a generic "we could not generate a thumbnail" caption or something. brennie brennie
mandeep
Review request changed

People:

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