Add import/export settings buttons#9123
Conversation
You won't see a warning, it will purposefully silently drop them because they are not meant to be writeable by the window and you won't see any issues in the same window because in the same window you are showing the stuff in-memory in that window but it won't be saved to disk, synced to other windows or actually get used. While I don't want to discourage new contributors, could I please ask that you put a bit more thought into your pull requests or at the very least put more effort into testing them, as this is the second one that you have submitted that doesn't actually work in practice. |
|
You're right, I'm sorry. I'm a bit too eager to contribute. I did test my changes, but I saw your warning about the folder fields a bit late, and I only tested the settings value after import rather than actually testing whether the folders were correct. |
Pull request was converted to draft
|
Maybe folder paths should not be allowed to be imported, does it really make sense? It is user and OS dependent, in most case the imported path would be invalid. |
|
What probably should not be transferred:
|
|
I checked all the settings, I think that settings impacted by env like Concerning passwords, they are not encrypted, so exporting them as plain text would not be an issue. As for whether this is relevant, I’d tend to say yes, but it could be changed if needed. There are also settings that need the app to be restarted. I've made the implementation similar to the list of non-exportable settings for the declaration, and when they are imported, the user is prompted to restart the app or to revert the settings that needed a restart. It works but I will see if I find something more "elegant" |
|
I'm now satisfied with the state of this MR! |
Head branch was pushed to by a user without write access
…dependent of process.env
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Pull request was converted to draft
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
| 'screenshotAskPath', | ||
| 'screenshotFolderPath', | ||
|
|
||
| /* Depends on process.env.SUPPORTS_LOCAL_API */ |
There was a problem hiding this comment.
I'll let others chime in but I think we could keep the SUPPORTS_LOCAL_API settings transferable.
There was a problem hiding this comment.
There are many references to these values and quite easy for the code to miss SUPPORTS_LOCAL_API check before using them
Better be safe first
Pull Request Type
Related issue
#1018 (comment)
#7345
Description
This pull request adds support for importing and exporting FreeTube settings.
Some settings are not exportable, they are represented by the
settingsNotTransferrableset insidesettings.js. They are OS/user-specific settings, experimental settings, and settings that depend onprocess.env.I'm not entirely satisfied with having to duplicate setting keys, but the other solution that I thought of was to replace the state object values with a more complex structure, having additional fields like
isExportable. However, in my opinion, it would have required too much refactoring.Screenshots
Testing
Basic test
Non-exportable setting test
UI test