FreeTube icon indicating copy to clipboard operation
FreeTube copied to clipboard

Implementing showOpenDialog's web callback

Open MarmadileManteater opened this issue 2 years ago • 3 comments


Implementing showOpenDialog's web callback

Pull Request Type Please select what type of pull request this is:

  • [ ] Bugfix
  • [X] Feature Implementation
  • [ ] Documentation
  • [ ] Other

Description This PR implements showOpenDialog's web callback, and in order to do that meaningfully, it also refactors the import functions in data-settings (importHistory, importPlaylists, importSubscriptions, etc) to use a new utility function called readFileFromDialog instead of fs.readFile.

/**
 * @param {Object} response the response from `showOpenDialog`
 * @param {Number} index which file to read (defaults to the first in the response)
 * @returns the text contents of the selected file
 */
async readFileFromDialog(context, { response, index = 0 }) {
  return await new Promise((resolve, reject) => {
    if (process.env.IS_ELECTRON) {
      // if this is Electron, use fs
      fs.readFile(response.filePaths[index], (err, data) => {
        if (err) {
          reject(err)
          return
        }
        resolve(new TextDecoder('utf-8').decode(data))
      })
    } else {
      // if this is web, use FileReader
      try {
        const reader = new FileReader()
        reader.onload = function (file) {
          resolve(file.currentTarget.result)
        }
        reader.readAsText(response.files[index])
      } catch (exception) {
        reject(exception)
      }
    }
  })
}

readFileFromDialog normalizes the reading of files from a file picker between Electron and web builds. In Electron, the underlying behavior should be exactly the same, but in web builds, this change makes the import buttons in data-settings work.

It is worth noting that this change also hides the Check for legacy subscriptions button when IS_ELECTRON is false. I figured this made sense because there is no way to check for that in a web browser without something like the native file API which has limited browser support at this time.

Screenshots (if appropriate) These screenshots are from a version of my web build with this PR applied to it. Before:

After:

Testing (for code that is not small enough to be easily understandable) This change effects all of the import buttons under data-settings. I have tested this with Electron and web builds on every type of subscription import, the playlist import, and the history import.

The easiest way to test the subscriptions is to populate them ( either through using the app or from an existing data export ) and export them to all of the different types (db,csv,json,opml). Then, you can remove all of your subscriptions in the privacy settings and check each of the subscription export types with the exact same subscriptions.

Desktop (please complete the following information):

  • OS: Windows 10
  • OS Version: Pro Version 21H2 Installed on ‎4/‎3/‎2022 OS build 19044.1889 Experience Windows Feature Experience Pack 120.2212.4180.0
  • FreeTube version: 0.17.1

Additional context In my opinion, the diff for data-settings is really hard to read, but I didn't remove much of any existing code. I moved all of the code inside the fs.readFile callbacks to after the call for readFileFromDialog.

I took code that looked like this:

const response = await this.showOpenDialog(options)
if (response.canceled || response.filePaths.length === 0) {
  return
}

const filePath = response.filePaths[0]

fs.readFile(filePath, async (err, data) => {
  if (err) {
    const message = this.$t('Settings.Data Settings.Unable to read file')
    this.showToast({
      message: `${message}: ${err}`
    })
    return
  }
  // more specific function code
})

And, I made it look like this:

const response = await this.showOpenDialog(options)
if (response.canceled || response.filePaths?.length === 0) {
  return
}
// this variable is also sometimes called `data`
let textDecode
try {
  textDecode = await this.readFileFromDialog({ response })
} catch (err) {
  const message = this.$t('Settings.Data Settings.Unable to read file')
  this.showToast({
    message: `${message}: ${err}`
  })
  return
}
// more specific function code

I would be willing to refactor everything back to using a callback with .then if that would reduce the amount of additions/deletions in this PR. The reason I used await over .then was because I noticed that this.showOpenDialog was being awaited, so I figured it made sense to also use await for this.readFileFromDialog.

This change would take one step closer towards making an in-memory filesystem unnecessary for running FreeTube in web environments.

MarmadileManteater avatar Sep 22 '22 20:09 MarmadileManteater

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Sep 23 '22 01:09 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Sep 23 '22 02:09 github-actions[bot]

Got this error when importing history, might or might not be related to this PR

image

Edit 1: Got the same error in development, probably unrelated

PikachuEXE avatar Sep 23 '22 06:09 PikachuEXE