fix(downloads): Improve inline attachment handling

- Show downloading state for inline attachments
- Ensure that the UI updates /after/ the download has completed
- Don't delete finished downloads (previously we were forgetting that a file was downloaded and checking again and again)

Fixes #462

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

@@ -58,12 +58,21 @@ class MessageItemBody extends React.Component
       showQuotedText: !@state.showQuotedText
 
   _onBodyChanged: (body) =>
+    downloadingSpinnerPath = Utils.imageNamed('inline-loading-spinner.gif')
+
     # Replace cid:// references with the paths to downloaded files
     for file in @props.message.files
-      continue if @props.downloads[file.id]
-      cidLink = "cid:#{file.contentId}"
-      fileLink = "#{FileDownloadStore.pathForFile(file)}"
-      body = body.replace(cidLink, fileLink)
+      download = @props.downloads[file.id]
+      cidRegexp = new RegExp("cid:#{file.contentId}(['\"]+)", 'gi')
+
+      if download and download.state isnt 'finished'
+        # Render a spinner and inject a `style` tag that injects object-position / object-fit
+        body = body.replace cidRegexp, (text, quoteCharacter) ->
+          "#{downloadingSpinnerPath}#{quoteCharacter} style=#{quoteCharacter} object-position: 50% 50%; object-fit: none; "
+      else
+        # Render the completed download
+        body = body.replace cidRegexp, (text, quoteCharacter) ->
+          "#{FileDownloadStore.pathForFile(file)}#{quoteCharacter}"
 
     # Replace remaining cid:// references - we will not display them since they'll
     # throw "unknown ERR_UNKNOWN_URL_SCHEME". Show a transparent pixel so that there's

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

@@ -116,6 +116,7 @@ describe "MessageItem", ->
       @component = ReactTestUtils.renderIntoDocument(
         <MessageItemBody message={@message} downloads={@downloads} />
       )
+      advanceClock()
 
   describe "when the message contains attachments", ->
     beforeEach ->
@@ -156,6 +157,7 @@ describe "MessageItem", ->
       it "should give images a fixed height when height and width are set as html attributes", ->
         @message.body = """
           <img src=\"cid:#{file_inline.contentId}\"/>
+          <img src='cid:#{file_inline.contentId}'/>
           <img src=\"cid:#{file_inline.contentId}\" width="50"/>
           <img src=\"cid:#{file_inline.contentId}\" width="50" height="40"/>
           <img src=\"cid:#{file_inline.contentId}\" width="1000" height="800"/>
@@ -163,9 +165,10 @@ describe "MessageItem", ->
         @createComponent()
         body = @component.state.processedBody
         expect(body).toEqual """<img src="/fake/path-inline.png"/>
-<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR4nGNikAQAACIAHF/uBd8AAAAASUVORK5CYII=" width="50"/>
-<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR4nGNikAQAACIAHF/uBd8AAAAASUVORK5CYII=" width="50" height="40" style="height:40px;" />
-<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR4nGNikAQAACIAHF/uBd8AAAAASUVORK5CYII=" width="1000" height="800" style="height:592px;" />
+<img src='/fake/path-inline.png'/>
+<img src="/fake/path-inline.png" width="50"/>
+<img src="/fake/path-inline.png" width="50" height="40" style="height:40px;" />
+<img src="/fake/path-inline.png" width="1000" height="800" style="height:592px;" />
 """
 
   describe "showQuotedText", ->

spec/stores/file-download-store-spec.coffee

@@ -251,7 +251,7 @@ describe "FileDownloadStore", ->
   describe "_abortFetchFile", ->
     beforeEach ->
       @download =
-        abort: jasmine.createSpy('abort')
+        ensureClosed: jasmine.createSpy('abort')
         fileId: @testfile.id
       FileDownloadStore._downloads[@testfile.id] = @download
 
@@ -260,11 +260,11 @@ describe "FileDownloadStore", ->
       spyOn(fs, 'unlink')
       FileDownloadStore._abortFetchFile(@testfile)
       expect(fs.unlink).toHaveBeenCalled()
-      expect(@download.abort).toHaveBeenCalled()
+      expect(@download.ensureClosed).toHaveBeenCalled()
 
     it "should not try to delete the file if doesn't exist", ->
       spyOn(fs, 'exists').andCallFake (path, callback) -> callback(false)
       spyOn(fs, 'unlink')
       FileDownloadStore._abortFetchFile(@testfile)
       expect(fs.unlink).not.toHaveBeenCalled()
-      expect(@download.abort).toHaveBeenCalled()
+      expect(@download.ensureClosed).toHaveBeenCalled()

src/components/retina-img.cjsx

@@ -17,12 +17,6 @@ StylesImpactedByZoom = [
   'marginRight'
 ]
 
-# We don't want to call `getLoadSettings` for each and every RetinaImg
-# instance because it's a fairly expensive operation. Since the
-# resourcePath can't change once the app has booted, it's safe to set the
-# constant at require-time
-DEFAULT_RESOURCE_PATH = NylasEnv.getLoadSettings().resourcePath
-
 Mode =
   ContentPreserve: 'original'
   ContentLight: 'light'
@@ -138,8 +132,7 @@ class RetinaImg extends React.Component
       name = "#{basename}-active.#{ext}"
     if @props.selected is true
       name = "#{basename}-selected.#{ext}"
-    resourcePath = @props.resourcePath ? DEFAULT_RESOURCE_PATH
-    Utils.imageNamed(resourcePath, name)
 
+    Utils.imageNamed(name, @props.resourcePath)
 
 module.exports = RetinaImg

src/flux/models/utils.coffee

@@ -5,6 +5,7 @@ tz = Intl.DateTimeFormat().resolvedOptions().timeZone
 
 TaskRegistry = null
 DatabaseObjectRegistry = null
+DefaultResourcePath = null
 
 module.exports =
 Utils =
@@ -140,9 +141,12 @@ Utils =
   tableNameForJoin: (primaryKlass, secondaryKlass) ->
     "#{primaryKlass.name}-#{secondaryKlass.name}"
 
-  imageNamed: (resourcePath, fullname) ->
+  imageNamed: (fullname, resourcePath) ->
     [name, ext] = fullname.split('.')
 
+    DefaultResourcePath ?= NylasEnv.getLoadSettings().resourcePath
+    resourcePath ?= DefaultResourcePath
+
     Utils.images ?= {}
     if not Utils.images[resourcePath]?
       imagesPath = path.join(resourcePath, 'static', 'images')

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

@@ -18,7 +18,15 @@ mkdirpAsync = (folder) ->
     mkdirp folder, (err) ->
       if err then reject(err) else resolve(folder)
 
+State =
+  Unstarted: 'unstarted'
+  Downloading: 'downloading'
+  Finished: 'finished'
+  Failed: 'failed'
+
 class Download
+  @State: State
+
   constructor: ({@fileId, @targetPath, @filename, @filesize, @progressCallback}) ->
     if not @filename or @filename.length is 0
       throw new Error("Download.constructor: You must provide a non-empty filename.")
@@ -29,23 +37,14 @@ class Download
 
     @percent = 0
     @promise = null
+    @state = State.Unstarted
     @
 
-  state: ->
-    if not @promise
-      'unstarted'
-    else if @promise.isFulfilled()
-      'finished'
-    else if @promise.isRejected()
-      'failed'
-    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()
+    state: @state
     fileId: @fileId
     percent: @percent
     filename: @filename
@@ -60,23 +59,20 @@ class Download
     @promise = new Promise (resolve, reject) =>
       accountId = AccountStore.current()?.id
       stream = fs.createWriteStream(@targetPath)
-      finished = false
-      finishedAction = null
 
       # We need to watch the request for `success` or `error`, but not fire
-      # a callback until the stream has ended. These helper functions ensure
-      # that resolve or reject is only fired once regardless of the order
-      # these two events (stream end and `success`) happen in.
-      streamEnded = =>
-        finished = true
-        if finishedAction
-          finishedAction(@)
+      # a callback until the stream has ended. This helper functions ensure
+      # that resolve or reject is only fired once the stream has been closed.
+      checkIfFinished = (action) =>
+        # Wait for the stream to finish writing to disk and clear the request
+        return if @request
 
-      onStreamEnded = (action) =>
-        if finished
-          action(@)
-        else
-          finishedAction = action
+        if @state is State.Finished
+          resolve()
+        else if @state is State.Failed
+          reject()
+
+      @state = State.Downloading
 
       NylasAPI.makeRequest
         json: false
@@ -90,19 +86,22 @@ class Download
             @percent = progress.percent
             @progressCallback()
           .on "end", =>
-            # Wait for the file stream to finish writing before we resolve or reject
-            stream.end(streamEnded)
+            @request = null
+            checkIfFinished()
           .pipe(stream)
 
         success: =>
           # 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.
-          onStreamEnded(resolve)
+          @state = State.Finished
+          @percent = 100
+          checkIfFinished()
 
         error: =>
-          onStreamEnded(reject)
+          @state = State.Failed
+          checkIfFinished()
 
-  abort: ->
+  ensureClosed: ->
     @request?.abort()
 
 
@@ -129,16 +128,15 @@ FileDownloadStore = Reflux.createStore
     return undefined unless file
     path.join(@_downloadDirectory, file.id, file.displayName())
 
-  downloadDataForFile: (fileId) -> @_downloads[fileId]?.data()
+  downloadDataForFile: (fileId) ->
+    @_downloads[fileId]?.data()
 
   # Returns a hash of download objects keyed by fileId
   #
   downloadDataForFiles: (fileIds=[]) ->
     downloadData = {}
     fileIds.forEach (fileId) =>
-      data = @downloadDataForFile(fileId)
-      return unless data
-      downloadData[fileId] = data
+      downloadData[fileId] = @downloadDataForFile(fileId)
     return downloadData
 
   ########### PRIVATE ####################################################
@@ -152,29 +150,30 @@ FileDownloadStore = Reflux.createStore
   # Returns a promise with a Download object, allowing other actions to be
   # daisy-chained to the end of the download operation.
   _runDownload: (file) ->
+    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 = @_downloads[file.id]
+    return download.run() if download
+
+    # create a new download for this file
+    download = new Download
+      fileId: file.id
+      filesize: file.size
+      filename: file.displayName()
+      targetPath: targetPath
+      progressCallback: => @trigger()
+
+    # Do we actually need to queue and run the download? Queuing a download
+    # for an already-downloaded file has side-effects, like making the UI
+    # flicker briefly.
     @_prepareFolder(file).then =>
-      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 = @_downloads[file.id]
-      return download.run() if download
-
-      # create a new download for this file
-      download = new Download
-        fileId: file.id
-        filesize: file.size
-        filename: file.displayName()
-        targetPath: targetPath
-        progressCallback: => @trigger()
-
-      # Do we actually need to queue and run the download? Queuing a download
-      # for an already-downloaded file has side-effects, like making the UI
-      # flicker briefly.
       @_checkForDownloadedFile(file).then (alreadyHaveFile) =>
         if alreadyHaveFile
           # If we have the file, just resolve with a resolved download representing the file.
           download.promise = Promise.resolve()
+          download.state = State.Finished
           return Promise.resolve(download)
         else
           @_downloads[file.id] = download
@@ -230,9 +229,8 @@ FileDownloadStore = Reflux.createStore
       fs.unlink(downloadPath) if exists
 
   _cleanupDownload: (download) ->
-    download.abort()
+    download.ensureClosed()
     @trigger()
-    delete @_downloads[download.fileId]
 
   _defaultSavePath: (file) ->
     if process.platform is 'win32'

src/flux/stores/message-body-processor.coffee

@@ -31,7 +31,7 @@ class MessageBodyProcessor
     @_version
 
   processAndSubscribe: (message, callback) =>
-    callback(@process(message))
+    _.defer => callback(@process(message))
     sub = {message, callback}
     @_subscriptions.push(sub)
     return =>