Delete temporary files when done with them

(safeUnlink isn't really tested)

5 files changed, 78 insertions, 12 deletions

diff --git a/src/downloaders.js b/src/downloaders.js
index b9cc33d..2df6655 100644
--- a/src/downloaders.js
+++ b/src/downloaders.js
@@ -14,6 +14,13 @@ const { promisify } = require('util')
 const writeFile = promisify(fs.writeFile)
 const copyFile = fse.copy
 
+// Pseudo-tempy!!
+/*
+const tempy = {
+  directory: () => './tempy-fake'
+}
+*/
+
 function makeHTTPDownloader() {
   return function(arg) {
     const dir = tempy.directory()
diff --git a/src/loop-play.js b/src/loop-play.js
index 9d87423..dd477a1 100644
--- a/src/loop-play.js
+++ b/src/loop-play.js
@@ -11,12 +11,14 @@
 const { spawn } = require('child_process')
 const FIFO = require('fifo-js')
 const EventEmitter = require('events')
+const promisifyProcess = require('./promisify-process')
+const { getItemPathString } = require('./playlist-utils')
+
+const { safeUnlink } = require('./playlist-utils')
+
 const {
-  getDownloaderFor, makeConverterDownloader,
-  byName: downloadersByName
+  getDownloaderFor, byName: downloadersByName
 } = require('./downloaders')
-const { getItemPathString } = require('./playlist-utils')
-const promisifyProcess = require('./promisify-process')
 
 class DownloadController extends EventEmitter {
   waitForDownload() {
@@ -47,6 +49,7 @@ class DownloadController extends EventEmitter {
     // (The reasoning is that it's possible for a download to
     // be canceled and replaced with a new download (see cancel)
     // which would void the result of the old download.)
+    // The resulting file is a WAV.
 
     this.cleanupListeners()
 
@@ -68,8 +71,13 @@ class DownloadController extends EventEmitter {
       return
     }
 
-    if (!canceled) {
+    // If this current download has been canceled, we should get rid of the
+    // download file (and shouldn't emit a download success).
+    if (canceled) {
+      this.emit('deleteFile', file)
+    } else {
       this.emit('downloaded', file)
+      this.cleanupListeners()
     }
   }
 
@@ -113,10 +121,21 @@ class PlayController extends EventEmitter {
     while (this.nextTrack) {
       this.currentTrack = this.nextTrack
 
+      const next = this.nextFile
+      this.nextFile = undefined
+
       this.startNextDownload()
 
-      if (this.nextFile) {
-        await this.playFile(this.nextFile)
+      if (next) {
+        await this.playFile(next)
+
+        // Now that we're done playing the file, we should delete it.. unless
+        // it's the file that's coming up! (This would only happen in the case
+        // that all temporary files are stored in the same folder, together;
+        // indeed an unusual case, but technically possible.)
+        if (next !== this.nextFile) {
+          this.emit('deleteFile', next)
+        }
       }
 
       await this.waitForDownload()
@@ -159,7 +178,6 @@ class PlayController extends EventEmitter {
       downloader = getDownloaderFor(picked.downloaderArg)
     }
 
-    downloader = makeConverterDownloader(downloader, 'wav')
     this.downloadController.download(downloader, picked.downloaderArg)
 
     this.downloadController.waitForDownload()
@@ -266,6 +284,15 @@ class PlayController extends EventEmitter {
     this.kill()
   }
 
+  async skipUpNext() {
+    if (this.nextFile) {
+      this.emit('deleteFile', this.nextFile)
+    }
+
+    this.downloadController.cancel()
+    this.startNextDownload()
+  }
+
   seekAhead(secs) {
     this.sendCommand(`seek +${parseFloat(secs)}`)
   }
@@ -329,7 +356,7 @@ class PlayController extends EventEmitter {
 }
 
 module.exports = function loopPlay(
-  picker, playerCommand = 'mpv', playOpts = []
+  playlist, picker, playerCommand = 'mpv', playOpts = []
 ) {
   // Looping play function. Takes one argument, the "picker" function,
   // which returns a track to play. Stops when the result of the picker
@@ -340,6 +367,9 @@ module.exports = function loopPlay(
 
   const playController = new PlayController(picker, downloadController)
 
+  downloadController.on('deleteFile', f => safeUnlink(f, playlist))
+  playController.on('deleteFile', f => safeUnlink(f, playlist))
+
   Object.assign(playController, {playerCommand, playOpts})
 
   const promise = playController.loopPlay()
diff --git a/src/play.js b/src/play.js
index 7807c93..43576ff 100755
--- a/src/play.js
+++ b/src/play.js
@@ -343,7 +343,7 @@ async function main(args) {
       promise: playPromise,
       playController: play,
       downloadController
-    } = loopPlay(picker, playerCommand, playOpts)
+    } = loopPlay(activePlaylist, picker, playerCommand, playOpts)
 
     // We're looking to gather standard input one keystroke at a time.
     // But that isn't *always* possible, e.g. when piping into the http-music
@@ -413,7 +413,7 @@ async function main(args) {
         // TODO: It would be nice to have this as a method of
         // PlayController.
         // Double TODO: This doesn't actually work!!
-        play.startNextDownload()
+        play.skipUpNext()
       }
 
       if (
diff --git a/src/playlist-utils.js b/src/playlist-utils.js
index aed4964..765a317 100644
--- a/src/playlist-utils.js
+++ b/src/playlist-utils.js
@@ -1,5 +1,11 @@
 'use strict'
 
+const path = require('path')
+const fs = require('fs')
+
+const { promisify } = require('util')
+const unlink = promisify(fs.unlink)
+
 const parentSymbol = Symbol('parent')
 
 function updatePlaylistFormat(playlist) {
@@ -287,6 +293,27 @@ function isTrack(obj) {
   // return typeof array[1] === 'string'
 }
 
+function safeUnlink(file, playlist) {
+  if (!playlist) {
+    throw new Error('No playlist given to safe-unlink.')
+  }
+
+  // TODO: Is it good to call this every time? - But flattening a list probably
+  // isn't THAT big of a task.
+  const flat = flattenGrouplike(playlist)
+
+  if (
+    flat.items.some(t => path.resolve(t.downloaderArg) === path.resolve(file))
+  ) {
+    throw new Error(
+      'Attempted to delete a file path found in the playlist.json file - ' +
+      'this is almost definitely a bug!'
+    )
+  }
+
+  return unlink(file)
+}
+
 module.exports = {
   parentSymbol,
   updatePlaylistFormat, updateTrackFormat,
@@ -296,5 +323,6 @@ module.exports = {
   getPlaylistTreeString,
   getItemPath, getItemPathString,
   parsePathString,
-  isGroup, isTrack
+  isGroup, isTrack,
+  safeUnlink
 }
diff --git a/todo.txt b/todo.txt
index a6e4f48..417e506 100644
--- a/todo.txt
+++ b/todo.txt
@@ -267,3 +267,4 @@ TODO: Handle avconv failing (probably handle downloader rejections from within
 
 TODO: Delete temporary files when done with them - seriously! http-music alone
       filled up a good 9GB of disk space, solely on temporary music files.
+      (Done!)