Add a bunch of code commenting, and a skip up-next method

3 files changed, 90 insertions, 27 deletions

diff --git a/src/http-music.js b/src/http-music.js
index 3d7e217..f6c1a52 100755
--- a/src/http-music.js
+++ b/src/http-music.js
@@ -246,7 +246,11 @@ setupDefaultPlaylist('./playlist.json')
 
       process.stdin.on('data', data => {
         if (Buffer.from('s').equals(data)) {
-          play.skip()
+          play.skipCurrent()
+        }
+
+        if (Buffer.from([0x7f]).equals(data)) { // Delete
+          play.skipUpNext()
         }
 
         if (
diff --git a/src/loop-play.js b/src/loop-play.js
index 100c14b..b49d9e4 100644
--- a/src/loop-play.js
+++ b/src/loop-play.js
@@ -8,43 +8,107 @@ const tempy = require('tempy')
 class DownloadController {
   constructor(picker, downloader) {
     this.process = null
+    this.requestingSkipUpNext = false
+    this.isDownloading = false
 
     this.picker = picker
     this.downloader = downloader
   }
 
   async downloadNext() {
+    this.requestingSkipUpNext = false
+    this.isDownloading = true
+
+    // We need to actually pick something to download; we'll use the picker
+    // (given in the DownloadController constructor) for that.
+    // (See pickers.js.)
     const picked = this.picker()
 
+    // If the picker returns null, nothing was picked; that means that we
+    // should stop now. No point in trying to play nothing!
     if (picked == null) {
+      this.wavFile = null
       return false
     }
 
+    // The picked result is an array containing the title of the track (only
+    // really used to display to the user) and an argument to be passed to the
+    // downloader. The downloader argument doesn't have to be anything in
+    // particular; but typically it's a string containing a URL or file path.
+    // It's up to the downloader to decide what to do with it.
     const [ title, downloaderArg ] = picked
+
     console.log(`Downloading ${title}..\nDownloader arg: ${downloaderArg}`)
 
+    // The "from" file is downloaded by the downloader (given in the
+    // DownloadController constructor) using the downloader argument we just
+    // got.
     const from = await this.downloader(downloaderArg)
 
+    // The "to" file is simply a WAV file. We give this WAV file a specific
+    // name - the title of the track we got earlier, sanitized to be file-safe
+    // - so that when `play` outputs the name of the song, it's obvious to the
+    // user what's being played.
     const tempDir = tempy.directory()
     const to = tempDir + `/.${sanitize(title)}.wav`
 
-    // We pass false to promisifyProcess to show we want hte output of avconv
-    // to be silenced.
-    const convertProcess = spawn('avconv', ['-y', '-i', from, to])
-    const convertPromise = promisifyProcess(convertProcess, false)
-
+    // Now that we've got the `to` and `from` file names, we can actually do
+    // the convertion. We don't want any output from `avconv` at all, since the
+    // output of `play` will usually be displayed while `avconv` is running,
+    // so we pass `-loglevel quiet` into `avconv`.
+    const convertProcess = spawn('avconv', [
+      '-loglevel', 'quiet', '-i', from, to
+    ])
+
+    // It's handy to store the output WAV file (the "to" file) and the `avconv`
+    // process; the WAV file is used later to be played, and the convert
+    // process is stored so it can be killed before it finishes.
     this.wavFile = to
     this.process = convertProcess
 
+    // Now it's only a matter of time before the process is finished.
+    // Literally; we need to await the promisified version of our convertion
+    // child process.
     try {
-      await convertPromise
+      await promisifyProcess(convertProcess)
     } catch(err) {
       console.warn("Failed to convert " + title)
       console.warn("Selecting a new track\n")
 
+      // There's a chance we'll fail, though. That could happen if the passed
+      // "from" file doesn't actually contain audio data. In that case, we
+      // have to attempt this whole process over again, so that we get a
+      // different file. (Technically, the picker might always pick the same
+      // file; if that's the case, and the convert process is failing on it,
+      // we could end up in an infinite loop. That would be bad, since there
+      // isn't any guarding against a situation like that here.)
+      return await this.downloadNext()
+    }
+
+    // If we were requested to skip the up-next track that's currently being
+    // downloaded (probably by the user), we'll have to do that now.
+    if (this.requestingSkipUpNext) return await this.downloadNext()
+
+    // We successfully downloaded something, and so the downloadNext function
+    // is done. We mark that here, so that skipUpNext will know that it'll need
+    // to start a whole new downloadNext to have any effect.
+    this.isDownloading = false
+  }
+
+  skipUpNext() {
+    // If we're already in the process of downloading the up-next track, we'll
+    // set the requestingSkipUpNext flag to true. downloadNext will use this to
+    // cancel its current download and begin new.
+    if (this.isDownloading) {
+      this.requestingSkipUpNext = true
       this.killProcess()
+    }
 
-      return await this.downloadNext()
+    // If we aren't currently downloading a track, downloadNext won't
+    // automatically be called from the start again, so we need to do that
+    // here.
+    if (!this.isDownloading) {
+      this.downloadNext()
     }
   }
 
@@ -64,6 +128,9 @@ class PlayController {
   }
 
   async loopPlay() {
+    // Playing music in a loop isn't particularly complicated; essentially, we
+    // just want to keep downloading and playing tracks until none is picked.
+
     await this.downloadController.downloadNext()
 
     while (this.downloadController.wavFile) {
@@ -109,10 +176,14 @@ module.exports = function loopPlay(picker, downloader, playArgs = []) {
   return {
     promise,
 
-    skip: function() {
+    skipCurrent: function() {
       playController.killProcess()
     },
 
+    skipUpNext: function() {
+      downloadController.skipUpNext()
+    },
+
     kill: function() {
       playController.killProcess()
       downloadController.killProcess()
diff --git a/src/promisify-process.js b/src/promisify-process.js
index 4d06f8c..d1c09d7 100644
--- a/src/promisify-process.js
+++ b/src/promisify-process.js
@@ -2,26 +2,14 @@
 
 const { Writable } = require('stream')
 
-module.exports = function promisifyProcess(proc, showLogging = true) {
-  // Takes a process (from child_process) and returns a promise that resolves
-  // when the process exits (or rejects with a warning, if the exit code is
-  // non-zero).
+module.exports = function promisifyProcess(proc) {
+  // Takes a process (from the child_process module) and returns a promise that
+  // resolves when the process exits (or rejects with a warning, if the exit
+  // code is non-zero).
 
   return new Promise((resolve, reject) => {
-    if (showLogging) {
-      proc.stdout.pipe(process.stdout)
-      proc.stderr.pipe(process.stderr)
-    } else {
-      // For some mysterious reason, youtube-dl doesn't seem to work unless
-      // we pipe the output of it SOMEWHERE..
-
-      const emptyStream = () => Object.assign(new Writable(), {
-        write: () => {}
-      })
-
-      proc.stdout.pipe(emptyStream())
-      proc.stderr.pipe(emptyStream())
-    }
+    proc.stdout.pipe(process.stdout)
+    proc.stderr.pipe(process.stderr)
 
     proc.on('exit', code => {
       if (code === 0) {