-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Set default externalPlayerPath #1702
Conversation
Hi, thanks for looking into this. I wonder how it can be partially fixed, maybe it does not work for the other paths as well? I think we should allow externalPlayerPath to still be null, and handle the cases where it is not allowed. |
The other paths are not set to |
I see, so maybe setting it to undefined fixes it as well? |
Okay, I have opened #1704 to handle the null's in the However, we should still consider changing the default externalPlayerPath to a string type. |
src/renderer/lib/state.js
Outdated
@@ -207,6 +207,8 @@ function load (cb) { | |||
onSavedState(err) | |||
return | |||
} | |||
} else if (saved.prefs.externalPlayerPath == null) { | |||
saved.prefs.externalPlayerPath = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct way to handle old configuration values is by adding an entry to the migrations.js
file to "upgrade" the user's config when they update to a new version of the app. This way, we can handle the migration in one place. Then, throughout the rest of the codebase we can just assume that the configuration file has the new format. So, in this case, we could just assume that saved.prefs.externalPlayerPath
will always be a string going forward :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the migration, assuming the next version will be 0.21.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is the purpose of this pull request?
What changes did you make?
PR #1498 fixed issue #1652 for
downloadPath
andtorrentsFolderPath
, but not forexternalPlayerPath
. I changed the default value to""
.Is there anything you'd like reviewers to focus on?
I handled existing config files that have
externalPlayerPath = null
by changing this to""
when loading the config file. It's not a beautiful solution, but otherwise they have to edit it by hand.