-
Notifications
You must be signed in to change notification settings - Fork 857
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
Replace certain labels with clearer values #4342
base: development
Are you sure you want to change the base?
Replace certain labels with clearer values #4342
Conversation
Also makes Playlist Autoplay disabled as an option if Autoplay is enabled. This makes the relationship between the two far clearer.
e81d316
to
475cddb
Compare
const newSetting = outdatedSettings[outdatedSetting] | ||
const oldValue = outdatedSettingInDB[1] | ||
loadSetting(newSetting, oldValue) | ||
await DBSettingHandlers.delete(outdatedSetting) |
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.
@absidue Your suggestion has been implemented. Only last question is if we want to comment this line out for one release to allow new release toe-dippers from being disrupted.
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 like this more but it's still not quite what I had in mind.
My idea was that the fixing of the settings would happen inside the datastore base handler settings class (src/datastores/handlers/base.js
) in the relevant query functions e.g. find()
, _findAppReadyRelatedSettings()
, etc (you'll have to check which ones are actually affected). That way it would be the only place that would have to have any code for renamed settings, the rest of FreeTube like the store, would not have to care that settings were ever renamed, as it's handled in one place at the lowest possible level.
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.
Want to make sure I understand what you mean - have application logic in src/datastores/handlers/base.js
that queries for a specific set of settings before find()
, uses each one's value to supplant their new equivalent's, and then deletes them? We don't have any other app logic there, so I worry that might be a bit out of place if I'm reading you right.
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.
@absidue Apologies, just boosting this q in case you didn't see.
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.
@absidue Apologies to boost this once again, but still awaiting your response on this last part. We don't actually have any application logic in that file, and I don't think it's confusing in the code currently which code corresponds to the renaming logic (just one for-block and object). It also lets us reuse a similar paradigm back-to-back (see the code itself). I am willing to refactor this if there is a strong enough pro to moving it (+ the time commitment), but I'm having trouble seeing that at the moment.
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.
Personally I see these changes as database migrations, which I would prefer to keep as close to the actual databases as possible. It also keeps the code a lot simpler, as only place that has to care about the settings having another name the Settings.find()
method in the src/datastores/handlers/base.js
file, instead of your current approach that spreads it across many files and also requires IPC calls to make the changes, instead of being able to directly modify the database.
You wouldn't need any extra find calls. just do the normal find all call on the database, modify it if necessary, make the database modifications if necessary and then return the response, that can all happen inside the Settings.find()
method in the src/datastores/handlers/base.js
file.
@jasonhenriquez could u maybe make changes to incorporate #1682 to your PR? |
We discussed this over the Matrix channel, but just so others can see, this will not be included in this PR due to its effect on other ongoing efforts. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…at/improve-label-clarity
Conflicts have been resolved. A maintainer will review the pull request shortly. |
I'm not capable of solving this one in a reasonable amount of time. Someone else please take this up. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Replace certain labels with clearer values
Pull Request Type
Related issue
N/A; although, general reports of confusion with these labels in comments, issues, and the Matrix channel
Description
en-US
label changes:Next Video Interval
->Autoplay Countdown Timer
Enter Fullscreen on Display Rotate
->Enter Fullscreen On Rotating the Device
Enable Screenshot
->Enable Video Screenshot
Hide Unsubscribe Button
->Hide Subscribe Button
*Play Next Video
->Autoplay (Recommended Channels and Playlists)
*Autoplay Playlists
->Autoplay (Playlists Only)
*Autoplay Videos
->Start Videos Automatically
*Enable Theatre Mode by Default
->Enable Theater Mode by Default
*Behavioral change:
Player Settings
is now disabled when General Autoplay is enabled (to more clearly represent that it is a subset of its behavior)New dev utility:
aliasToOriginal
object insettings.js
to allow for easy, safe, & reverse-compatible modification of older existing settings variable namesTesting
development
from your local and confirm that those selections are still preserved.Autoplay (Playlists Only)
is disabled as a choice whenAutoplay (Recommended Channels and Playlists)
is enabled.Desktop
Additional context
I kept the original keys of
en-US.yaml
intact to prevent any disruption to non-English language users.