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.



  • 40
  • 0
  • 37
  • 0
  • 77
Description From Last Updated
Can you add screenshots showing this off? chipx86 chipx86
Too many blank lines. There should only be 2. chipx86 chipx86
Too many spaces here. chipx86 chipx86
This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should ... chipx86 chipx86
Blank line between these. chipx86 chipx86
Too many spaces here. chipx86 chipx86
This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion ... chipx86 chipx86
No blank lines here. chipx86 chipx86
This needs to describe what the function is doing. chipx86 chipx86
Indentation is all wrong, and the blank line should be removed. chipx86 chipx86
This console.log should go away. Also, blank line between statements and blocks. chipx86 chipx86
I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror. This needs ... chipx86 chipx86
Too many spaces. chipx86 chipx86
Should have parens around the whole e => ... to help with readability, given the assignment. chipx86 chipx86
Missing a comment doc block. chipx86 chipx86
Indentation is all wrong. chipx86 chipx86
Missing a doc comment block. chipx86 chipx86
This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types ... chipx86 chipx86
This is too long for the line. chipx86 chipx86
Indentation is broken all throughout. chipx86 chipx86
This should probably be a <th>, not a <td> with a <label>. chipx86 chipx86
The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding ... chipx86 chipx86
The <tr> is empty, and the <img> isn't in a valid place. I'm having a hard time imagining how the ... chipx86 chipx86
This needs to describe what this class is all about. chipx86 chipx86
Blank line between these. chipx86 chipx86
Blank line between these. chipx86 chipx86
The options need to be described in an Option Args section. Grep around for examples. chipx86 chipx86
No blank line. chipx86 chipx86
No space around the =. Should be options={}. chipx86 chipx86
This needs to describe what the function does. chipx86 chipx86
No space before (). chipx86 chipx86
Space after :. chipx86 chipx86
There should only be one blank line at this level. chipx86 chipx86
No need to capitalize "dialog." chipx86 chipx86
Comment isn't formatted right. chipx86 chipx86
This isn't the class name. chipx86 chipx86
This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined ... chipx86 chipx86
Blank line between block and statement. chipx86 chipx86
Missing , at the end of this. chipx86 chipx86
This should go up in the models section. chipx86 chipx86
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...