feature(attachments): Inline attachments, download store does not use tasks

Summary:
This diff updates the FileDownloadStore to use it's own internal Download class instead of leveraging the TaskStore (since downloads shouldn't be resumable / serialized to disk anyway). This means things can be tighter and less dependent on Actions. Downloads are now promise-based, so actions like "open file after downloading" can be chained to the end of the Download promise.

The MessageStore now queues downloads for inline attachments when you view the thread, and the MessageItem.cjsx substitutes in local file paths for cid:<CID>.

Test Plan: I made some pretty big changes and did not break any tests. This is a bad sign... Tests forthcoming, but will probably ship this to users tomorrow first.

Reviewers: evan

Reviewed By: evan

Differential Revision: https://review.inboxapp.com/D1149

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

@@ -3,30 +3,24 @@ React = require 'react'
 {Actions} = require 'inbox-exports'
 
 # Passed in as props from MessageItem and FileDownloadStore
-# Generated in tasks/download-file.coffee
 # This is empty if the attachment isn't downloading.
-# @props.downloadData:
-#   state - One of "pending "started" "progress" "completed" "aborted" "failed"
-#   fileId - The id of the file
-#   shellAction - Action used to open the file after downloading
-#   downloadPath - The full path of the download location
-#   total - From request-progress: total number of bytes
-#   percent - From request-progress
-#   received - From request-progress: currently received bytes
-#
+# @props.download is a FileDownloadStore.Download object
 # @props.file is a File object
 
 module.exports =
 MessageAttachment = React.createClass
   displayName: 'MessageAttachment'
 
+  propTypes:
+    file: React.PropTypes.object.isRequired,
+    download: React.PropTypes.object
+
   getInitialState: ->
     progressPercent: 0
 
   render: ->
-    <div className={"attachment-file-wrap " + (@props.downloadData?.state ? "")}>
-
-      <span className="attachment-download-bar-wrap" style={@_showDownload()}>
+    <div className={"attachment-file-wrap " + (@props.download?.state() ? "")}>
+      <span className="attachment-download-bar-wrap">
         <span className="attachment-bar-bg"></span>
         <span className="attachment-download-progress" style={@_downloadProgressStyle()}></span>
       </span>
@@ -57,23 +51,19 @@ MessageAttachment = React.createClass
       </button>
 
   _downloadProgressStyle: ->
-    width: @props.downloadData?.percent ? 0
+    width: @props.download?.percent ? 0
 
   _onClickRemove: ->
     Actions.removeFile
       file: @props.file
       messageLocalId: @props.messageLocalId
 
-  _onClickView: -> Actions.viewFile(@props.file) if @_canClickToView()
+  _onClickView: -> Actions.fetchAndOpenFile(@props.file) if @_canClickToView()
 
-  _onClickDownload: -> Actions.saveFile @props.file
+  _onClickDownload: -> Actions.fetchAndSaveFile(@props.file)
 
-  _onClickAbort: -> Actions.abortDownload(@props.file, @props.downloadData)
+  _onClickAbort: -> Actions.abortDownload(@props.file, @props.download)
 
   _canClickToView: -> not @props.removable and not @_isDownloading()
 
-  _showDownload: ->
-    if @_isDownloading() then {display: "block"} else {display: "none"}
-
-  _isDownloading: ->
-    @props.downloadData?.state in ["pending", "started", "progress"]
+  _isDownloading: -> @props.download?.state() is "downloading"

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

@@ -1,24 +1,29 @@
 moment = require 'moment'
 React = require 'react'
+_ = require 'underscore-plus'
 EmailFrame = require './email-frame'
 MessageAttachment = require "./message-attachment.cjsx"
 MessageParticipants = require "./message-participants.cjsx"
-{FileDownloadStore, ComponentRegistry} = require 'inbox-exports'
+{ComponentRegistry, FileDownloadStore} = require 'inbox-exports'
 Autolinker = require 'autolinker'
 
 module.exports =
 MessageItem = React.createClass
   displayName: 'MessageItem'
+  propTypes:
+    message: React.PropTypes.object.isRequired,
+    collapsed: React.PropTypes.bool
+    thread_participants: React.PropTypes.arrayOf(React.PropTypes.object),
 
   getInitialState: ->
     # 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.downloadsForFiles(@_fileIds())
+    downloads: FileDownloadStore.downloadsForFileIds(@props.message.fileIds())
     showQuotedText: false
     collapsed: @props.collapsed
 
   componentDidMount: ->
-    @_storeUnlisten = FileDownloadStore.listen(@_onFileDownloadStoreChange)
+    @_storeUnlisten = FileDownloadStore.listen(@_onDownloadStoreChange)
 
   componentWillUnmount: ->
     @_storeUnlisten() if @_storeUnlisten
@@ -29,6 +34,9 @@ MessageItem = React.createClass
     quotedTextClass += " state-" + @state.showQuotedText
 
     messageIndicators = ComponentRegistry.findAllViewsByRole('MessageIndicator')
+    attachments = @_attachmentComponents()
+    if attachments.length > 0
+      attachments = <div className="attachments-area">{attachments}</div>
 
     header =
       <header className="message-header" onClick={@_onToggleCollapsed}>
@@ -48,11 +56,9 @@ MessageItem = React.createClass
     else
       <div className="message-item-wrap">
         {header}
-        <div className="attachments-area" style={display: @_hasAttachments()}>
-          {@_attachmentCompontents()}
-        </div>
+        {attachments}
         <EmailFrame showQuotedText={@state.showQuotedText}>
-          {@_formatBody(@props.message.body)}
+          {@_formatBody()}
         </EmailFrame>
         <a className={quotedTextClass} onClick={@_toggleQuotedText}></a>
       </div>
@@ -60,13 +66,22 @@ MessageItem = React.createClass
 
   # Eventually, _formatBody will run a series of registered body transformers.
   # For now, it just runs a few we've hardcoded here, which are all synchronous.
-  _formatBody: (body) ->
-    return "" unless body
-    formattedBody = Autolinker.link(body, {twitter: false})
-    formattedBody
+  _formatBody: ->
+    return "" unless @props.message
 
-  _fileIds: ->
-    (@props.message?.files ? []).map (file) -> file.id
+    body = @props.message.body
+
+    # Apply the autolinker pass to make emails and links clickable
+    body = Autolinker.link(body, {twitter: false})
+
+    # Find cid:// references and replace them with the paths to downloaded files
+    for file in @props.message.files
+      continue if _.find @state.downloads, (d) -> d.fileId is file.id
+      cidLink = "cid:#{file.contentId}"
+      fileLink = "#{FileDownloadStore.pathForFile(file)}"
+      body = body.replace(cidLink, fileLink)
+
+    body
 
   _containsQuotedText: ->
     # I know this is gross - one day we'll replace it with a nice system.
@@ -84,12 +99,10 @@ MessageItem = React.createClass
 
   _formatContacts: (contacts=[]) ->
 
-  _attachmentCompontents: ->
-    (@props.message.files ? []).map (file) =>
-      <MessageAttachment file={file} downloadData={@state.downloads?[file.id] ? {}}/>
-
-  _hasAttachments: ->
-    if @props.message.files?.length > 0 then "block" else "none"
+  _attachmentComponents: ->
+    attachments = _.filter @props.message.files, (f) -> not f.contentId?
+    attachments.map (file) =>
+      <MessageAttachment file={file} download={@state.downloads[file.id]}/>
 
   _messageTime: ->
     moment(@props.message.date).format(@_timeFormat())
@@ -112,9 +125,9 @@ MessageItem = React.createClass
   # Stubbable for testing. Returns a `moment`
   _today: -> moment()
 
-  _onFileDownloadStoreChange: ->
-    # downloads is a hash keyed by the file id
-    @setState downloads: FileDownloadStore.downloadsForFiles(@_fileIds())
+  _onDownloadStoreChange: ->
+    @setState
+      downloads: FileDownloadStore.downloadsForFileIds(@props.message.fileIds())
 
   _onToggleCollapsed: ->
     @setState

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

@@ -143,10 +143,6 @@ test_thread = (new Thread).fromJSON({
 })
 
 describe "MessageList", ->
-  keymap_path = "internal_packages/message-list/keymaps/message-list.cson"
-  base_path = "keymaps/base.cson"
-  keymap_mappings = CSON.readFileSync(keymap_path)
-
   _resetMessageStore = ->
     MessageStore._items = []
     MessageStore._threadId = null
@@ -156,10 +152,6 @@ describe "MessageList", ->
     @message_list = TestUtils.renderIntoDocument(<MessageList />)
     @message_list_node = @message_list.getDOMNode()
 
-    # IMPORTANT: You need to manually register the keymaps with the
-    # KeymapManager (aliased onto atom.keymaps).
-    atom.keymaps.add(keymap_path, keymap_mappings)
-
   it "renders into the document", ->
     expect(TestUtils.isCompositeComponentWithType(@message_list,
            MessageList)).toBe true

internal_packages/message-list/stylesheets/message-list.less

@@ -163,10 +163,16 @@
       }
     }
 
-    &.pending, &.started, &.progress {
-      // When downloading is in progress.
+    .attachment-download-bar-wrap {
+      display:none;
+    }
+    &.downloading {
       color: @text-color-subtle;
 
+      .attachment-download-bar-wrap {
+        display:inherit;
+      }
+      
       .attachment-file-and-name:hover {
         cursor: default;
         color: @text-color-subtle;

src/atom.coffee

@@ -329,6 +329,7 @@ class Atom extends Model
         console.warn(error.stack)
       else
         console.warn(error)
+        console.warn(error.stack)
 
       @emitter.emit('will-throw-error', eventObject)
       @emit('uncaught-error', error.message, null, null, null, error)
@@ -825,14 +826,11 @@ class Atom extends Model
   setRepresentedFilename: (filename) ->
     ipc.send('call-window-method', 'setRepresentedFilename', filename)
 
-  showSaveDialog: (callback) ->
-    callback(showSaveDialogSync())
-
-  showSaveDialogSync: (defaultPath) ->
+  showSaveDialog: (defaultPath, callback) ->
     defaultPath ?= @project?.getPath()
-    currentWindow = @getCurrentWindow()
+    parentWindow = if process.platform is 'darwin' then null else @getCurrentWindow()
     dialog = remote.require('dialog')
-    dialog.showSaveDialog currentWindow, {title: 'Save File', defaultPath}
+    dialog.showSaveDialog(parentWindow, {title: 'Save File', defaultPath}, callback)
 
   saveSync: ->
     stateString = JSON.stringify(@state)

src/browser/edgehill-application.coffee

@@ -243,9 +243,6 @@ class AtomApplication
     ipc.on 'command', (event, command) =>
       @emit(command)
 
-    ipc.on 'save-file', (event, path) =>
-      @saveFile(path)
-
     ipc.on 'window-command', (event, command, args...) ->
       win = BrowserWindow.fromWebContents(event.sender)
       win.emit(command, args...)
@@ -521,18 +518,3 @@ class AtomApplication
     dialog = require 'dialog'
     dialog.showOpenDialog parentWindow, openOptions, (pathsToOpen) =>
       @lastFocusedWindow?.browserWindow.webContents.send("paths-to-open", pathsToOpen)
-
-  saveFile: (path) ->
-    dialog = require 'dialog'
-
-    # Show the open dialog as child window on Windows and Linux, and as
-    # independent dialog on OS X. This matches most native apps.
-    parentWindow =
-      if process.platform is 'darwin' then null else BrowserWindow.getFocusedWindow()
-
-    options =
-      title: "Save"
-      defaultPath: path
-
-    dialog.showSaveDialog parentWindow, options, (savePath) =>
-      @lastFocusedWindow?.browserWindow.webContents.send("save-file-selected", savePath)

src/flux/actions.coffee

@@ -66,11 +66,10 @@ windowActions = [
   "fileUploaded",
   "fileAborted",
   "removeFile",
-  "viewFile",
-  "saveFile",
+  "fetchAndOpenFile",
+  "fetchAndSaveFile",
+  "fetchFile",
   "abortDownload",
-  "downloadStateChanged",
-  "fileDownloaded",
 
   # Notification actions
   "postNotification",

src/flux/models/file.coffee

@@ -15,13 +15,13 @@ class File extends Model
 
     'contentType': Attributes.String
       modelKey: 'contentType'
-      jsonKey: 'content-type'
+      jsonKey: 'content_type'
 
     'messageIds': Attributes.Collection
       modelKey: 'messageIds'
       jsonKey: 'message_ids'
       itemClass: String
 
-    'isEmbedded': Attributes.Boolean
-      modelKey: 'isEmbedded'
-      jsonKey: 'is_embedded'
+    'contentId': Attributes.String
+      modelKey: 'contentId'
+      jsonKey: 'content_id'

src/flux/models/message.coffee

@@ -77,7 +77,7 @@ class Message extends Model
 
   toJSON: ->
     json = super
-    json.file_ids = _.map @files, (file) -> file.id
+    json.file_ids = @fileIds()
     json.object = 'draft' if @draft
     json
 
@@ -103,6 +103,9 @@ class Message extends Model
         participants["#{(contact?.email ? "").toLowerCase().trim()} #{(contact?.name ? "").toLowerCase().trim()}"] = contact if contact?
     return _.values(participants)
 
+  fileIds: ->
+    _.map @files, (file) -> file.id
+
   uploadFiles: (paths = []) ->
     # TODO: DEPRECATE. MOVE TO STORE
     FileUploadTask = require '../tasks/file-upload-task'

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

@@ -4,77 +4,133 @@ ipc = require 'ipc'
 path = require 'path'
 shell = require 'shell'
 Reflux = require 'reflux'
+_ = require 'underscore-plus'
 Actions = require '../actions'
-DownloadFileTask = require '../tasks/download-file'
+progress = require 'request-progress'
+NamespaceStore = require '../stores/namespace-store'
+
+class Download
+  constructor: ({@fileId, @targetPath, @progressCallback}) ->
+    @percent = 0
+    @promise = null
+    @
+
+  state: ->
+    if not @promise
+      'unstarted'
+    if @promise.isFulfilled()
+      'finished'
+    if @promise.isRejected()
+      'failed'
+    else
+      'downloading'
+
+  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?
+
+      # Does the file already exist on disk? If so, just resolve immediately.
+      fs.exists @targetPath, (exists) =>
+        return resolve(@) if exists
+        @request = atom.inbox.makeRequest
+          path: "/n/#{namespace}/files/#{@fileId}/download"
+          success: => resolve(@)
+          error: => reject(@)
+
+        progress(@request, {throtte: 250})
+        .on("progress", (progress) =>
+          @percent = progress.percent
+          @progressCallback()
+        ).pipe(fs.createWriteStream(@targetPath))
+
+  abort: ->
+    @request?.abort()
+
 
 module.exports =
 FileDownloadStore = Reflux.createStore
   init: ->
-    # From Views
-    @listenTo Actions.viewFile, @_onViewFile
-    @listenTo Actions.saveFile, @_onSaveFile
-    @listenTo Actions.abortDownload, @_onAbortDownload
-
-    # From Tasks
-    @listenTo Actions.downloadStateChanged, @_onDownloadStateChanged
-    @listenTo Actions.fileDownloaded, @_onFileDownloaded
-
-    # Keyed by fileId
-    @_fileDownloads = {}
+    @listenTo Actions.fetchFile, @_fetch
+    @listenTo Actions.fetchAndOpenFile, @_fetchAndOpen
+    @listenTo Actions.fetchAndSaveFile, @_fetchAndSave
+    @listenTo Actions.abortDownload, @_cleanupDownload
 
+    @_downloads = []
+    @_downloadDirectory = "#{atom.getConfigDirPath()}/downloads"
+    fs.exists @_downloadDirectory, (exists) =>
+      fs.mkdir(@_downloadDirectory) unless exists
 
   ######### PUBLIC #######################################################
 
-  # Returns a hash of fileDownloads keyed by fileId
-  downloadsForFiles: (fileIds=[]) ->
-    downloads = {}
+  # Returns a hash of download objects keyed by fileId
+
+  pathForFile: (file) ->
+    return undefined unless file
+    path.join(@_downloadDirectory, "#{file.id}-#{file.filename}")
+
+  downloadForFileId: (fileId) ->
+    _.find @_downloads, (d) -> d.fileId is fileId
+
+  downloadsForFileIds: (fileIds=[]) ->
+    map = {}
     for fileId in fileIds
-      downloads[fileId] = @_fileDownloads[fileId] if @_fileDownloads[fileId]?
-    return downloads
+      download = @downloadForFileId(fileId)
+      map[fileId] = download if download
+    map
 
   ########### PRIVATE ####################################################
 
-  _onViewFile: (file) ->
-    Actions.queueTask new DownloadFileTask
+  # Returns a promise allowing other actions to be daisy-chained
+  # to the end of the download operation
+  _startDownload: (file, options = {}) ->
+    targetPath = @pathForFile(file)
+
+    # is there an existing download for this file? If so,
+    # return that promise so users can chain to the end of it.
+    download = _.find @_downloads, (d) -> d.fileId is file.id
+    return download.run() if download
+
+    # create a new download for this file and add it to our queue
+    download = new Download
       fileId: file.id
-      shellAction: "openItem" # Safe to serialize
-      downloadPath: path.join(os.tmpDir(), file.filename)
+      targetPath: targetPath
+      progressCallback: => @trigger()
+ 
+    cleanup = =>
+      @_cleanupDownload(download)
+      Promise.resolve(download)
 
-  _onSaveFile: (file) ->
-    # We setup the listener here because we don't want to catch someone
-    # else's open dialog
-    unlistenSave = Actions.savePathSelected.listen (pathToSave) =>
-      unlistenSave?()
-      if pathToSave?
-        @_actionAfterDownload = "showItemInFolder"
-        Actions.queueTask new DownloadFileTask
-          fileId: file.id
-          shellAction: "showItemInFolder"
-          downloadPath: pathToSave
-
-    # When the dialog closes, it triggers `Actions.pathToSave`
-    ipc.send('save-file', @_defaultSavePath(file))
-
-  _onAbortDownload: (file) ->
-    Actions.abortTask({object: 'DownloadFileTask', fileId: file.id})
-
-  # Generated in tasks/download-file.coffee
-  # downloadData:
-  #   state - One of "pending "started" "progress" "completed" "aborted" "failed"
-  #   fileId - The id of the file
-  #   shellAction - Action used to open the file after downloading
-  #   downloadPath - The full path of the download location
-  #   total - From request-progress: total number of bytes
-  #   percent - From request-progress
-  #   received - From request-progress: currently received bytes
-  _onDownloadStateChanged: (downloadData={}) ->
-    @_fileDownloads[downloadData.fileId] = downloadData
+    @_downloads.push(download)
+    promise = download.run().catch(cleanup).then(cleanup)
     @trigger()
+    promise
 
-  _onFileDownloaded: ({fileId, shellAction, downloadPath}) ->
-    delete @_fileDownloads[fileId]
+  _fetch: (file) ->
+    @_startDownload(file)
+
+  _fetchAndOpen: (file) ->
+    @_startDownload(file).then (download) ->
+      shell.openItem(download.targetPath)
+
+  _fetchAndSave: (file) ->
+    atom.showSaveDialog @_defaultSavePath(file), (savePath) =>
+      return unless savePath
+      @_startDownload(file).then (download) ->
+        stream = fs.createReadStream(download.targetPath)
+        stream.pipe(fs.createWriteStream(savePath))
+        stream.on 'end', ->
+          shell.showItemInFolder(savePath)
+
+  _cleanupDownload: (download) ->
+    download.abort()
+    @_downloads = _.without(@_downloads, download)
     @trigger()
-    shell[shellAction](downloadPath)
 
   _defaultSavePath: (file) ->
     if process.platform is 'win32'
@@ -84,6 +140,5 @@ FileDownloadStore = Reflux.createStore
     downloadDir = path.join(home, 'Downloads')
     if not fs.existsSync(downloadDir)
       downloadDir = os.tmpdir()
-    else
 
     path.join(downloadDir, file.filename)

src/flux/stores/message-store.coffee

@@ -70,6 +70,13 @@ MessageStore = Reflux.createStore
         return unless loadedThreadId == @_threadId
         @_items = items
         @_itemsLocalIds = localIds
+
+        # Start fetching inline image attachments. Note that the download store
+        # is smart enough that calling this multiple times is not bad!
+        for msg in items
+          for file in msg.files
+            Actions.fetchFile(file) if file.contentId
+
         @trigger(@)
 
   _fetchFromCacheDebounced: _.debounce ->