Skip to content
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

Make Caption Settings Persistent #1269

Merged

Conversation

SebTota
Copy link
Contributor

@SebTota SebTota commented May 11, 2021


Made caption settings persistent.

Pull Request Type

  • Feature Implementation

Related issue
#989

Description
Make the caption settings persistent across new videos and closing/re-opening the application by:

  • Saving caption settings to db when the caption settings modal is closed
  • Retrieving the saved caption settings each time a video is opened

Testing

  1. Open a video with captions
  2. Modify caption settings
  3. Open a new video with captions and turn captions on, notice caption settings persist
  4. Close the application and reopen a video, notice caption settings persist

Desktop (please complete the following information):

  • OS: Mac OS
  • OS Version: 10.15.7
  • FreeTube version: Latest Development Branch

Additional context
At the time of creating this PR, issue #1268 may make testing difficult. Please check the comment I left on that issue if caption settings do not appear in video player.

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Also will saved caption settings not working/applicable for different videos?

src/renderer/components/ft-video-player/ft-video-player.js Outdated Show resolved Hide resolved
@SebTota
Copy link
Contributor Author

SebTota commented May 11, 2021

I realized settings were only persistent to a single session. This is fixed in the new commit.

@SebTota
Copy link
Contributor Author

SebTota commented May 11, 2021

Also will saved caption settings not working/applicable for different videos?

If I understand your question correctly, yes, caption settings will be saved when going to a different video. They will be saved from video to video and even when the application is closed and reopened.

To be clear, this does not save a default caption language. It only saves the caption settings that pertain to how captions look like when they are on. It does not turn captions on by default to a certain language. I was going to add that feature but I figured it should be a different issue created for it since the caption settings pertain more to the videojs video player and the settings for a default language pertain more to FreeTube itself.

@GilgusMaximus
Copy link
Contributor

Sorry to say that, but this is not working. I can set the settings and then go to the next video and have them reset again.
I also built the app to test this, but it does not work in dev and in built version.

I do not get a setting in the settings.db file and it seems like there is also a problem within the same session now.

Additionally there have been 2 merge conflicts introduced by other commits, which are easy to fix however, as you just have to remove the merge conflict info and can leave all changes in if I am seeing this correctly.

@SebTota
Copy link
Contributor Author

SebTota commented Jun 5, 2021

Do you mind taking a look at this video (hosted on Flickr because it's too large for GitHub). Do you not have the same results?

@GilgusMaximus
Copy link
Contributor

This is how it looks for me in dev mode as well as in production mode. I also pulled the fresh branch just to make sure. From history we know that electron on Windows isn't always great with updating settings and cookies in production mode, but it usually works in dev mode.

Maybe someone else can chip in and check this out? Just to make sure this is not a problem with my OS. @Svallinn @PikachuEXE ?

Github.mp4

@SebTota
Copy link
Contributor Author

SebTota commented Jun 8, 2021

@GilgusMaximus I took another look at the code and you are correct, there was an issue. I'm not sure how this happened and why I didn't notice it earlier. Please take another look if you can. I apologize for not noticing this earlier.

@Svallinn
Copy link
Member

LGTM. Thank you for your contribution.

@Svallinn Svallinn merged commit 2297bf3 into FreeTubeApp:development Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants