Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing showOpenDialog's web callback #2608

Conversation

MarmadileManteater
Copy link
Contributor


Implementing showOpenDialog's web callback

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

  • Bugfix
  • 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.

@PrestonN PrestonN enabled auto-merge (squash) September 22, 2022 20:02
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 22, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 23, 2022
@github-actions
Copy link
Contributor

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

auto-merge was automatically disabled September 23, 2022 02:48

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 23, 2022 02:48
@github-actions
Copy link
Contributor

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

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Sep 23, 2022

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

@PrestonN PrestonN merged commit 1512178 into FreeTubeApp:development Sep 23, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 23, 2022
@MarmadileManteater MarmadileManteater deleted the implementing-dialog-boxes-in-web branch October 6, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants