Skip to content

Move the choose default folder logic to the main process #6811

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.

Sign up for GitHub

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

Merged
FreeTubeBot merged 1 commit into from
Feb 18, 2025
Merged

Move the choose default folder logic to the main process #6811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const IpcChannels = {
GET_SYSTEM_LOCALE: 'get-system-locale',
GET_PICTURES_PATH: 'get-pictures-path',
GET_NAVIGATION_HISTORY: 'get-navigation-history',
SHOW_OPEN_DIALOG: 'show-open-dialog',
SHOW_SAVE_DIALOG: 'show-save-dialog',
STOP_POWER_SAVE_BLOCKER: 'stop-power-save-blocker',
START_POWER_SAVE_BLOCKER: 'start-power-save-blocker',
Expand Down Expand Up @@ -49,6 +48,7 @@ const IpcChannels = {

GENERATE_PO_TOKEN: 'generate-po-token',

CHOOSE_DEFAULT_FOLDER: 'choose-default-folder',
WRITE_SCREENSHOT: 'write-screenshot',
}

Expand Down Expand Up @@ -126,6 +126,11 @@ const SyncEvents = {
},
}

const DefaultFolderKind = {
DOWNLOADS: 0,
SCREENSHOTS: 1
}

/*
DEV NOTE: Duplicate any and all changes made here to our [official documentation site here](https://github.com/FreeTubeApp/FreeTube-Docs/blob/master/usage/keyboard-shortcuts.md)
to have them reflect on the [keyboard shortcut reference webpage](https://docs.freetubeapp.io/usage/keyboard-shortcuts).
Expand Down Expand Up @@ -216,6 +221,7 @@ export {
IpcChannels,
DBActions,
SyncEvents,
DefaultFolderKind,
KeyboardShortcuts,
MAIN_PROFILE_ID,
MOBILE_WIDTH_THRESHOLD,
Expand Down
12 changes: 2 additions & 10 deletions src/datastores/handlers/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,8 @@ class Settings {
})
}

static _findBounds() {
return db.settings.findOneAsync({ _id: 'bounds' })
Copy link
Member

@PikachuEXE PikachuEXE Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against this change since it makes the 'bound code separated
Or just put them into constants somewhere

Also it's not related to screenshot one

Copy link
Member Author

@absidue absidue Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on what you are saying, as the first and last sentence don't make sense to me.

As for using constants, I would strongly advise against that, as the source of truth for the setting names are the object keys in the settings store, so making each key a constant, would make the rest of the app harder to maintain. As you would have to check the constants files every single time, because the variable names would be completely irrelevant as the getter, actions and mutation names are based on the string value, so would need to know the exact string value for the setting to be able to write the getter, action and mutation calls in the rest of the app as the majority of the files wouldn't be able to use the constants (this seems unnecessarily messy: computed: { ['get' + Settings.BACKEND_FALLBACK.charAt(0).toUpperCase() + Settings.BACKEND_FALLBACK.slice(1)]: function () { ...

Copy link
Member

@PikachuEXE PikachuEXE Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against this change since it makes the bound code separated

Currently all the 'bounds' are inside src/datastores/handlers/base.js
Now separated in different files
Not suggesting to have getters/setters like the rest of "settings"
Probably not even worth mentioning maybe I am just ranting

Also it's not related to screenshot one

PR: Move the choose default folder logic to the main process
Nothing mentioned about the changes & related testing and also unrelated to title (the former is bigger issue than the later

Copy link
Member Author

@absidue absidue Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it's not related to screenshot one
PR: Move the choose default folder logic to the main process
Nothing mentioned about the changes & related testing and also unrelated to title (the former is bigger issue than the later

It really didn't seem worth mentioning a minor code cleanup where the code does exactly the same thing as before, just with less duplication (literally just moved strings around), something you can easily check by looking at the code diff. If you really don't like it, I can change it only add the _findOne method and the new uses of it in this pull request and then switch the old code over to it in a separate pull request. But going back to the old approach of adding a separate method for every key, which has no advantage other than unnecessarily bloating the app, is not something I want to be doing.

Copy link
Member

@PikachuEXE PikachuEXE Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most important point here from me is to at least mention "there are some extra cleanup" in a short sentence.
No problem having minor cleanup with PRs, just mention it briefly to set an example for other new contributors.
I have encountered several times about mixing too much stuff in a PR (worst kind is changes totally not mentioned in PR description

}

static _findTheme() {
return db.settings.findOneAsync({ _id: 'baseTheme' })
static _findOne(_id) {
return db.settings.findOneAsync({ _id })
}

static _findSidenavSettings() {
Expand All @@ -77,10 +73,6 @@ class Settings {
}
}

static _findScreenshotFolderPath() {
return db.settings.findOneAsync({ _id: 'screenshotFolderPath' })
}

static _updateBounds(value) {
return db.settings.updateAsync({ _id: 'bounds' }, { _id: 'bounds', value }, { upsert: true })
}
Expand Down
67 changes: 56 additions & 11 deletions src/main/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
SyncEvents,
ABOUT_BITCOIN_ADDRESS,
KeyboardShortcuts,
DefaultFolderKind,
} from '../constants'
import * as baseHandlers from '../datastores/handlers/base'
import { extractExpiryTimestamp, ImageCache } from './ImageCache'
Expand Down Expand Up @@ -643,7 +644,7 @@ function runApp() {
searchQueryText = null
} = { }) {
// Syncing new window background to theme choice.
const windowBackground = await baseHandlers.settings._findTheme().then((setting) => {
const windowBackground = await baseHandlers.settings._findOne('baseTheme').then((setting) => {
if (!setting) {
return nativeTheme.shouldUseDarkColors ? '#212121' : '#f1f1f1'
}
Expand Down Expand Up @@ -745,7 +746,7 @@ function runApp() {
height: 800
})

const boundsDoc = await baseHandlers.settings._findBounds()
const boundsDoc = await baseHandlers.settings._findOne('bounds')
if (typeof boundsDoc?.value === 'object') {
const { maximized, fullScreen, ...bounds } = boundsDoc.value
const windowVisible = screen.getAllDisplays().some(display => {
Expand Down Expand Up @@ -989,14 +990,6 @@ function runApp() {
sender.executeJavaScript('document.querySelector("video.player").ui.getControls().togglePiP()', true)
})

ipcMain.handle(IpcChannels.SHOW_OPEN_DIALOG, async ({ sender }, options) => {
const senderWindow = findSenderWindow(sender)
if (senderWindow) {
return await dialog.showOpenDialog(senderWindow, options)
}
return await dialog.showOpenDialog(options)
})

ipcMain.handle(IpcChannels.SHOW_SAVE_DIALOG, async ({ sender }, options) => {
const senderWindow = findSenderWindow(sender)
if (senderWindow) {
Expand All @@ -1011,12 +1004,58 @@ function runApp() {
})
}

ipcMain.on(IpcChannels.CHOOSE_DEFAULT_FOLDER, async (event, kind) => {
if (!isFreeTubeUrl(event.senderFrame.url) || (kind !== DefaultFolderKind.DOWNLOADS && kind !== DefaultFolderKind.SCREENSHOTS)) {
return
}

const settingId = kind === DefaultFolderKind.DOWNLOADS ? 'downloadFolderPath' : 'screenshotFolderPath'

let currentPath = (await baseHandlers.settings._findOne(settingId))?.value

if (typeof currentPath !== 'string' || currentPath.length === 0) {
currentPath = app.getPath(kind === DefaultFolderKind.DOWNLOADS ? 'downloads' : 'pictures')
}

const dialogOptions = {
defaultPath: currentPath,
properties: ['openDirectory']
}

let result

const window = BrowserWindow.fromWebContents(event.sender)
if (window) {
result = await dialog.showOpenDialog(window, dialogOptions)
} else {
result = await dialog.showOpenDialog(dialogOptions)
}

if (result.canceled) {
return
}

await baseHandlers.settings.upsert(settingId, result.filePaths[0])

const syncPayload = {
event: SyncEvents.GENERAL.UPSERT,
data: {
_id: settingId,
value: result.filePaths[0]
}
}

BrowserWindow.getAllWindows().forEach((window) => {
window.webContents.send(IpcChannels.SYNC_SETTINGS, syncPayload)
})
})

ipcMain.handle(IpcChannels.WRITE_SCREENSHOT, async (event, filename, arrayBuffer) => {
if (!isFreeTubeUrl(event.senderFrame.url) || typeof filename !== 'string' || !(arrayBuffer instanceof ArrayBuffer)) {
return
}

const screenshotFolderPath = await baseHandlers.settings._findScreenshotFolderPath()
const screenshotFolderPath = await baseHandlers.settings._findOne('screenshotFolderPath')

let directory
if (screenshotFolderPath && screenshotFolderPath.value.length > 0) {
Expand Down Expand Up @@ -1172,6 +1211,12 @@ function runApp() {
return await baseHandlers.settings.find()

case DBActions.GENERAL.UPSERT:
// These two are only allowed to be changed by the CHOOSE_DEFAULT_FOLDER IPC action
// to avoid the "write to default folder" IPC calls being abused to write to arbitrary locations
if (data._id === 'downloadFolderPath' || data._id === 'screenshotFolderPath') {
return null
}

await baseHandlers.settings.upsert(data._id, data.value)
syncOtherWindows(
IpcChannels.SYNC_SETTINGS,
Expand Down
13 changes: 2 additions & 11 deletions src/renderer/components/download-settings/download-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import FtSelect from '../ft-select/ft-select.vue'
import FtButton from '../ft-button/ft-button.vue'
import FtInput from '../ft-input/ft-input.vue'
import { mapActions } from 'vuex'
import { IpcChannels } from '../../../constants'
import { DefaultFolderKind, IpcChannels } from '../../../constants'

export default defineComponent({
name: 'DownloadSettings',
Expand Down Expand Up @@ -50,20 +50,11 @@ export default defineComponent({
chooseDownloadingFolder: async function () {
if (process.env.IS_ELECTRON) {
const { ipcRenderer } = require('electron')

const folder = await ipcRenderer.invoke(
IpcChannels.SHOW_OPEN_DIALOG,
{ properties: ['openDirectory'] }
)

if (folder.canceled) return

this.updateDownloadFolderPath(folder.filePaths[0])
ipcRenderer.send(IpcChannels.CHOOSE_DEFAULT_FOLDER, DefaultFolderKind.DOWNLOADS)
}
},
...mapActions([
'updateDownloadAskPath',
'updateDownloadFolderPath',
'updateDownloadBehavior'
])
}
Expand Down
13 changes: 2 additions & 11 deletions src/renderer/components/player-settings/player-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import FtFlexBox from '../ft-flex-box/ft-flex-box.vue'
import FtButton from '../ft-button/ft-button.vue'
import FtInput from '../ft-input/ft-input.vue'
import FtTooltip from '../ft-tooltip/ft-tooltip.vue'
import { IpcChannels } from '../../../constants'
import { DefaultFolderKind, IpcChannels } from '../../../constants'
import path from 'path'
import { getPicturesPath } from '../../helpers/utils'

Expand Down Expand Up @@ -300,15 +300,7 @@ export default defineComponent({
// only use with electron
if (process.env.IS_ELECTRON) {
const { ipcRenderer } = require('electron')
const folder = await ipcRenderer.invoke(
IpcChannels.SHOW_OPEN_DIALOG,
{ properties: ['openDirectory'] }
)

if (!folder.canceled) {
await this.updateScreenshotFolderPath(folder.filePaths[0])
this.getScreenshotFolderPlaceholder()
}
ipcRenderer.send(IpcChannels.CHOOSE_DEFAULT_FOLDER, DefaultFolderKind.SCREENSHOTS)
}
},

Expand Down Expand Up @@ -364,7 +356,6 @@ export default defineComponent({
'updateScreenshotFormat',
'updateScreenshotQuality',
'updateScreenshotAskPath',
'updateScreenshotFolderPath',
'updateScreenshotFilenamePattern',
'parseScreenshotCustomFileName',
])
Expand Down