feat(attachment): improved downloading and draggable images

Summary:
Fixes T1975
Fixes T1900
Fixes T1899
Fixes T1979

Attachments downloading update progress

downloads will restart if the file on disk isn't complete

can drag images onto drive

Test Plan: edgehill --test

Reviewers: bengotow

Reviewed By: bengotow

Maniphest Tasks: T1900, T1899, T1975, T1979

Differential Revision: https://phab.nylas.com/D1638

exports/nylas-component-kit.coffee

@@ -13,6 +13,7 @@ module.exports =
   RetinaImg: require '../src/components/retina-img'
   EmptyState: require '../src/components/empty-state'
   ListTabular: require '../src/components/list-tabular'
+  DraggableImg: require '../src/components/draggable-img'
   MultiselectList: require '../src/components/multiselect-list'
   MultiselectActionBar: require '../src/components/multiselect-action-bar'
   ResizableRegion: require '../src/components/resizable-region'

internal_packages/attachments/lib/attachment-component.cjsx

@@ -15,7 +15,7 @@ class AttachmentComponent extends React.Component
   @propTypes:
     file: React.PropTypes.object.isRequired,
     download: React.PropTypes.object
-    removable: React.PropTypes.boolean
+    removable: React.PropTypes.bool
     targetPath: React.PropTypes.string
     messageLocalId: React.PropTypes.string
 
@@ -23,7 +23,7 @@ class AttachmentComponent extends React.Component
     @state = progressPercent: 0
 
   render: =>
-    <div className={"attachment-inner-wrap #{@props.download?.state() ? ""}"}>
+    <div className={"attachment-inner-wrap #{@props.download?.state ? ""}"}>
       <span className="attachment-download-bar-wrap">
         <span className="attachment-bar-bg"></span>
         <span className="attachment-download-progress" style={@_downloadProgressStyle()}></span>
@@ -49,8 +49,8 @@ class AttachmentComponent extends React.Component
       <div className="attachment-icon" onClick={@_onClickRemove}>
         <RetinaImg className="remove-icon" name="remove-attachment.png"/>
       </div>
-    else if @_isDownloading()
+    else if @_isDownloading() and @_canAbortDownload()
-      <div className="attachment-icon" onClick={@_onClickRemove}>
+      <div className="attachment-icon" onClick={@_onClickAbort}>
         <RetinaImg className="remove-icon" name="remove-attachment.png"/>
       </div>
     else
@@ -59,13 +59,15 @@ class AttachmentComponent extends React.Component
       </div>
 
   _downloadProgressStyle: =>
-    width: @props.download?.percent ? 0
+    width: "#{@props.download?.percent ? 0}%"
 
   _onClickRemove: =>
     Actions.removeFile
       file: @props.file
       messageLocalId: @props.messageLocalId
 
+  _canAbortDownload: -> true
+
   _onClickView: => Actions.fetchAndOpenFile(@props.file) if @_canClickToView()
 
   _onClickDownload: => Actions.fetchAndSaveFile(@props.file)
@@ -74,7 +76,7 @@ class AttachmentComponent extends React.Component
 
   _canClickToView: => not @props.removable and not @_isDownloading()
 
-  _isDownloading: => @props.download?.state() is "downloading"
+  _isDownloading: => @props.download?.state is "downloading"
 
   _extension: -> @props.file.filename.split('.').pop()
 

internal_packages/attachments/lib/image-attachment-component.cjsx

@@ -1,12 +1,13 @@
 path = require 'path'
 React = require 'react'
 AttachmentComponent = require './attachment-component'
+{Spinner, DraggableImg} = require 'nylas-component-kit'
 
 class ImageAttachmentComponent extends AttachmentComponent
   @displayName: 'ImageAttachmentComponent'
 
   render: =>
-    <div className={"attachment-inner-wrap " + @props.download?.state() ? ""}>
+    <div className={"attachment-inner-wrap " + @props.download?.state ? ""}>
       <span className="attachment-download-bar-wrap">
         <span className="attachment-bar-bg"></span>
         <span className="attachment-download-progress" style={@_downloadProgressStyle()}></span>
@@ -17,9 +18,22 @@ class ImageAttachmentComponent extends AttachmentComponent
       </span>
 
       <div className="attachment-preview" onClick={@_onClickView}>
-        <img src={@props.targetPath} />
+        {@_imgOrLoader()}
       </div>
 
     </div>
 
+  _canAbortDownload: -> false
+
+  _imgOrLoader: ->
+    if @props.download
+      if @props.download.percent <= 5
+        <div style={width: "100%", height: "100px"}>
+          <Spinner visible={true} />
+        </div>
+      else
+        <DraggableImg src={"#{@props.targetPath}?percent=#{@props.download.percent}"} />
+    else
+      <DraggableImg src={@props.targetPath} />
+
 module.exports = ImageAttachmentComponent

internal_packages/attachments/stylesheets/attachments.less

@@ -155,6 +155,7 @@
     position: relative;
     z-index: 1;
     max-width: 100%;
+    background: @background-secondary;
   }
 }
 

internal_packages/composer/lib/image-file-upload.cjsx

@@ -1,7 +1,7 @@
 path = require 'path'
 React = require 'react'
 FileUpload = require './file-upload'
-{RetinaImg} = require 'nylas-component-kit'
+{RetinaImg, DraggableImg} = require 'nylas-component-kit'
 
 class ImageFileUpload extends FileUpload
   @displayName: 'ImageFileUpload'
@@ -18,7 +18,7 @@ class ImageFileUpload extends FileUpload
       </span>
 
       <div className="attachment-preview" >
-        <img src={@props.uploadData.filePath} />
+        <DraggableImg src={@props.uploadData.filePath} />
       </div>
 
       <span className="attachment-upload-progress" style={@_uploadProgressStyle()}></span>

internal_packages/file-list/lib/file-frame-store.coffee

@@ -33,7 +33,7 @@ FileFrameStore = Reflux.createStore
   _update: ->
 
   _onFileDownloadChange: ->
-    @_download = FileDownloadStore.downloadForFileId(@_file.id) if @_file
+    @_download = FileDownloadStore.downloadDataForFile(@_file.id) if @_file
     if @_file and @_ready is false and not @_download
       @_ready = true
       @trigger()
@@ -46,7 +46,7 @@ FileFrameStore = Reflux.createStore
       filepath = FileDownloadStore.pathForFile(@_file)
       fs.exists filepath, (exists) =>
         Actions.fetchFile(@_file) if not exists
-        @_download = FileDownloadStore.downloadForFileId(@_file.id)
+        @_download = FileDownloadStore.downloadDataForFile(@_file.id)
         @_ready = not @_download
         @trigger()
     else

internal_packages/message-list/lib/message-item.cjsx

@@ -32,7 +32,7 @@ class MessageItem extends React.Component
     @state =
       # Holds the downloadData (if any) for all of our files. It's a hash
       # keyed by a fileId. The value is the downloadData.
-      downloads: FileDownloadStore.downloadsForFileIds(@props.message.fileIds())
+      downloads: FileDownloadStore.downloadDataForFiles(@props.message.fileIds())
       showQuotedText: @_isForwardedMessage()
       detailedHeaders: false
 
@@ -74,8 +74,8 @@ class MessageItem extends React.Component
         <EmailFrame showQuotedText={@state.showQuotedText}>
           {@_formatBody()}
         </EmailFrame>
-        {@_renderAttachments()}
         <a className={@_quotedTextClasses()} onClick={@_toggleQuotedText}></a>
+        {@_renderAttachments()}
       </div>
     </div>
 
@@ -260,7 +260,7 @@ class MessageItem extends React.Component
 
     # Replace cid:// references with the paths to downloaded files
     for file in @props.message.files
-      continue if _.find @state.downloads, (d) -> d.fileId is file.id
+      continue if @state.downloads[file.id]
       cidLink = "cid:#{file.contentId}"
       fileLink = "#{FileDownloadStore.pathForFile(file)}"
       body = body.replace(cidLink, fileLink)
@@ -325,6 +325,6 @@ class MessageItem extends React.Component
 
   _onDownloadStoreChange: =>
     @setState
-      downloads: FileDownloadStore.downloadsForFileIds(@props.message.fileIds())
+      downloads: FileDownloadStore.downloadDataForFiles(@props.message.fileIds())
 
 module.exports = MessageItem

internal_packages/message-list/spec/message-item-spec.cjsx

@@ -94,7 +94,7 @@ describe "MessageItem", ->
       return '/fake/path-inline.png' if f.id is file_inline.id
       return '/fake/path-downloading.png' if f.id is file_inline_downloading.id
       return null
-    spyOn(FileDownloadStore, 'downloadsForFileIds').andCallFake (ids) ->
+    spyOn(FileDownloadStore, 'downloadDataForFiles').andCallFake (ids) ->
       return {'file_1_id': download, 'file_inline_downloading_id': download_inline}
 
     @message = new Message

src/components/draggable-img.cjsx

@@ -0,0 +1,23 @@
+React = require 'react'
+
+###
+# Images are supposed to by default show a ghost image when dragging and
+# dropping. Unfortunatley this does not work in Electron. Since we're a
+# desktop app we don't want all images draggable, but we do want some (like attachments) to be able to be dragged away with a preview image.
+###
+class DraggableImg extends React.Component
+  @displayName: 'DraggableImg'
+
+  constructor: (@props) ->
+
+  render: =>
+    <img ref="img" onDragStart={@_onDragStart} {...@props} />
+
+  _onDragStart: (event) =>
+    img = React.findDOMNode(@refs.img)
+    rect = img.getBoundingClientRect()
+    y = event.clientY - rect.top
+    x = event.clientX - rect.left
+    event.dataTransfer.setDragImage(img, x, y)
+
+module.exports = DraggableImg

src/flux/stores/file-download-store.coffee

@@ -13,7 +13,7 @@ NamespaceStore = require '../stores/namespace-store'
 NylasAPI = require '../nylas-api'
 
 class Download
-  constructor: ({@fileId, @targetPath, @progressCallback}) ->
+  constructor: ({@fileId, @targetPath, @filename, @filesize, @progressCallback}) ->
     @percent = 0
     @promise = null
     @
@@ -28,59 +28,78 @@ class Download
     else
       'downloading'
 
+  # We need to pass a plain object so we can have fresh references for the
+  # React views while maintaining the single object with the running
+  # request.
+  data: -> Object.freeze _.clone
+    state: @state()
+    fileId: @fileId
+    percent: @percent
+    filename: @filename
+    filesize: @filesize
+    targetPath: @targetPath
+
   run: ->
     # If run has already been called, return the existing promise. Never
     # initiate multiple downloads for the same file
     return @promise if @promise
 
-    namespace = NamespaceStore.current()?.id
     @promise = new Promise (resolve, reject) =>
       return reject(new Error("Must pass a fileID to download")) unless @fileId?
       return reject(new Error("Must have a target path to download")) unless @targetPath?
 
       fs.exists @targetPath, (exists) =>
         # Does the file already exist on disk? If so, just resolve immediately.
-        return resolve(@) if exists
+        if exists
+          fs.stat @targetPath, (err, stats) =>
+            if not err and stats.size >= @filesize
+              return resolve(@)
+            else
+              @_doDownload(resolve, reject)
+        else
+          @_doDownload(resolve, reject)
 
-        stream = fs.createWriteStream(@targetPath)
+  _doDownload: (resolve, reject) =>
-        finished = false
+    namespace = NamespaceStore.current()?.id
-        finishedAction = null
+    stream = fs.createWriteStream(@targetPath)
+    finished = false
+    finishedAction = null
 
-        # We need to watch the request for `success` or `error`, but not fire
+    # We need to watch the request for `success` or `error`, but not fire
-        # a callback until the stream has ended. These helper functions ensure
+    # a callback until the stream has ended. These helper functions ensure
-        # that resolve or reject is only fired once regardless of the order
+    # that resolve or reject is only fired once regardless of the order
-        # these two events (stream end and `success`) happen in.
+    # these two events (stream end and `success`) happen in.
-        streamEnded = ->
+    streamEnded = ->
-          finished = true
+      finished = true
-          if finishedAction
+      if finishedAction
-            finishedAction(@)
+        finishedAction(@)
 
-        onStreamEnded = (action) ->
+    onStreamEnded = (action) ->
-          if finished
+      if finished
-            action(@)
+        action(@)
-          else
+      else
-            finishedAction = action
+        finishedAction = action
 
-        @request = NylasAPI.makeRequest
+    @request = NylasAPI.makeRequest
-          json: false
+      json: false
-          path: "/n/#{namespace}/files/#{@fileId}/download"
+      path: "/n/#{namespace}/files/#{@fileId}/download"
-          success: =>
+      success: =>
-            # At this point, the file stream has not finished writing to disk.
+        # At this point, the file stream has not finished writing to disk.
-            # Don't resolve yet, or the browser will load only part of the image.
+        # Don't resolve yet, or the browser will load only part of the image.
-            onStreamEnded(resolve)
+        onStreamEnded(resolve)
-          error: =>
+      error: =>
-            onStreamEnded(reject)
+        onStreamEnded(reject)
 
-        progress(@request, {throtte: 250})
+    progress(@request, {throtte: 250})
-        .on("progress", (progress) =>
+    .on("progress", (progress) =>
-          @percent = progress.percent
+      @percent = progress.percent
-          @progressCallback()
+      @progressCallback()
-        )
+    )
-        .on("end", =>
+    .on("end", =>
-          # Wait for the file stream to finish writing before we resolve or reject
+      # Wait for the file stream to finish writing before we resolve or reject
-          stream.end(streamEnded)
+      stream.end(streamEnded)
-        )
+    )
-        .pipe(stream)
+    .pipe(stream)
 
   abort: ->
     @request?.abort()
@@ -92,9 +111,9 @@ FileDownloadStore = Reflux.createStore
     @listenTo Actions.fetchFile, @_fetch
     @listenTo Actions.fetchAndOpenFile, @_fetchAndOpen
     @listenTo Actions.fetchAndSaveFile, @_fetchAndSave
-    @listenTo Actions.abortDownload, @_cleanupDownload
+    @listenTo Actions.abortDownload, @_abortDownload
 
-    @_downloads = []
+    @_downloads = {}
     @_downloadDirectory = "#{atom.getConfigDirPath()}/downloads"
     mkdirp(@_downloadDirectory)
 
@@ -104,46 +123,56 @@ FileDownloadStore = Reflux.createStore
 
   pathForFile: (file) ->
     return undefined unless file
-    path.join(@_downloadDirectory, "#{file.id}-#{file.filename}")
+    path.join(@_downloadDirectory, file.id, "#{file.filename}")
 
-  downloadForFileId: (fileId) ->
+  downloadDataForFile: (fileId) -> @_downloads[fileId]?.data()
-    return Utils.deepClone(_.find @_downloads, (d) -> d.fileId is fileId)
-
-  downloadsForFileIds: (fileIds=[]) ->
-    map = {}
-    for fileId in fileIds
-      download = @downloadForFileId(fileId)
-      if download
-        map[fileId] = download
-    return Utils.deepClone(map)
 
+  downloadDataForFiles: (fileIds=[]) ->
+    downloadData = {}
+    fileIds.forEach (fileId) =>
+      data = @downloadDataForFile(fileId)
+      return unless data
+      downloadData[fileId] = data
+    return downloadData
 
   ########### PRIVATE ####################################################
 
   # Returns a promise allowing other actions to be daisy-chained
   # to the end of the download operation
   _startDownload: (file, options = {}) ->
-    targetPath = @pathForFile(file)
+    @_prepareFolder(file).then =>
+      targetPath = @pathForFile(file)
 
-    # is there an existing download for this file? If so,
+      # is there an existing download for this file? If so,
-    # return that promise so users can chain to the end of it.
+      # return that promise so users can chain to the end of it.
-    download = _.find @_downloads, (d) -> d.fileId is file.id
+      download = @_downloads[file.id]
-    return download.run() if download
+      return download.run() if download
 
-    # create a new download for this file and add it to our queue
+      # create a new download for this file and add it to our queue
-    download = new Download
+      download = new Download
-      fileId: file.id
+        fileId: file.id
-      targetPath: targetPath
+        filesize: file.size
-      progressCallback: => @trigger()
+        filename: file.filename
+        targetPath: targetPath
+        progressCallback: => @trigger()
 
-    cleanup = =>
+      cleanup = =>
-      @_cleanupDownload(download)
+        @_cleanupDownload(download)
-      Promise.resolve(download)
+        Promise.resolve(download)
 
-    @_downloads.push(download)
+      @_downloads[file.id] = download
-    promise = download.run().catch(cleanup).then(cleanup)
+      promise = download.run().catch(cleanup).then(cleanup)
-    @trigger()
+      @trigger()
-    promise
+      return promise
+
+  _prepareFolder: (file) ->
+    new Promise (resolve, reject) =>
+      folder = path.join(@_downloadDirectory, file.id)
+      fs.exists folder, (exists) =>
+        if exists then resolve(folder)
+        else
+          mkdirp folder, (err) =>
+            if err then reject(err) else resolve(folder)
 
   _fetch: (file) ->
     @_startDownload(file)
@@ -161,9 +190,18 @@ FileDownloadStore = Reflux.createStore
         stream.on 'end', ->
           shell.showItemInFolder(savePath)
 
+  _abortDownload: (downloadData) ->
+    download = @_downloads[downloadData.fileId]
+    return unless download
+    @_cleanupDownload(download)
+    p = @pathForFile
+      id: downloadData.fileId
+      filename: downloadData.filename
+    fs.unlinkSync(p)
+
   _cleanupDownload: (download) ->
     download.abort()
-    @_downloads = _.without(@_downloads, download)
+    delete @_downloads[download.fileId]
     @trigger()
 
   _defaultSavePath: (file) ->

src/flux/tasks/send-draft.coffee

@@ -6,6 +6,7 @@ Message = require '../models/message'
 Task = require './task'
 TaskQueue = require '../stores/task-queue'
 SyncbackDraftTask = require './syncback-draft'
+FileUploadTask = require './file-upload-task'
 NylasAPI = require '../nylas-api'
 
 module.exports =
@@ -21,7 +22,8 @@ class SendDraftTask extends Task
     other instanceof SendDraftTask and other.draftLocalId is @draftLocalId
 
   shouldWaitForTask: (other) ->
-    other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId
+    (other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId) or
+    (other instanceof FileUploadTask and other.messageLocalId is @draftLocalId)
 
   performLocal: ->
     # When we send drafts, we don't update anything in the app until