Move the choose default folder logic to the main process #6811
Conversation
| @@ -59,12 +59,8 @@ class Settings { | |||
| }) | |||
| } | |||
|
|
|||
| static _findBounds() { | |||
| return db.settings.findOneAsync({ _id: 'bounds' }) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 () { ...
There was a problem hiding this comment.
I am against this change since it makes the
boundcode 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
* development: (58 commits) Convert FtSubscribeButton and watch-video-info SCSS to CSS (FreeTubeApp#6814) Use numbers instead of strings for the DBActions and SyncEvents constants (FreeTubeApp#6815) Translated using Weblate (English (United Kingdom)) Fix: search history text overflows if search term is long (FreeTubeApp#6728) Check if a keyboard composition session is active when pressing 'Enter' on ft-input (FreeTubeApp#6799) use hq img (FreeTubeApp#6826) Move the choose default folder logic to the main process (FreeTubeApp#6811) Translated using Weblate (French) Translated using Weblate (Turkish) Set process.platform at build time (FreeTubeApp#6784) Use logical spec for float (FreeTubeApp#6783) Migrate DataSettings to the composition API (FreeTubeApp#6785) Local API: Improve audio quality by sorting streams, highest bitrate first (FreeTubeApp#6807) Bump sass from 1.84.0 to 1.85.0 (FreeTubeApp#6825) Bump webpack from 5.97.1 to 5.98.0 (FreeTubeApp#6820) Bump postcss from 8.5.1 to 8.5.2 in the stylelint group (FreeTubeApp#6819) Bump the babel group with 2 updates (FreeTubeApp#6817) Bump globals from 15.14.0 to 15.15.0 (FreeTubeApp#6823) Bump sass-loader from 16.0.4 to 16.0.5 (FreeTubeApp#6822) Bump eslint from 9.20.0 to 9.20.1 in the eslint group (FreeTubeApp#6818) ... # Conflicts: # src/renderer/components/watch-video-info/watch-video-info.scss # src/renderer/components/watch-video-info/watch-video-info.vue
Move the choose default folder logic to the main process
Pull Request Type
Description
From a security perspective the current
SHOW_OPEN_DIALOGIPC call has two destinct issues, firstly it just wraps the Electron dialog API, giving the renderer processes full control over what options to pass as it doesn't validate any of the options and secondly while yes the app code prompts the user for the folder and then saves the path to the settings, there is nothing stopping you from setting it to any path you want through the devtools, without prompting the user and then using theWRITE_SCREENSHOTIPC call to write to that location.This pull request attempts to resolve both of those issues. It blocks modifications to the
downloadFolderPathandscreenshotFolderPathsettings from the renderer process and also provides a single IPC call to handle prompting the user and saving the path to the settings. The newCHOOSE_DEFAULT_FOLDERIPC call only takes a single parameter which tells it whether it is prompting the user for the screenshots or downloads folder. That means that theWRITE_SCREENSHOTIPC call will only be able to write to the folder selected by the user in the folder chooser dialog.Yes, I am aware that we still have Node integration turned on, so these changes don't make much of a difference just yet, but once we turn it off, the IPC calls will be the way to perform priviledged actions, so we want to lock them down as much as possible.
Testing
Use the buttons in the screenshot and download settings to check that the folder chooser shows up and works correctly (e.g. try choosing a folder, try cancelling and check that it doesn't overwrite the existing value, etc).
Desktop
Pika: Some minor code cleanup included too