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

Add watch progress saving setting to have multiple modes #6754

Open
wants to merge 11 commits into
base: development
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Feb 5, 2025

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

N/A

Description

There are times I want to skip a video in the middle but want to watch later without resuming
And I rarely need to save progress (except a few long videos per week)
This PR makes progress saving setting to have a new mode, now as:

  • Auto (same as Save Watched Progress enabled before)
  • Never (same as Save Watched Progress disabled before)
  • Semi-Auto (copied from tooltip text: Auto = Save on every video page exit, when video ended and error encountered (e.g. ratelimited and watch session expired). Semi-auto = Like Auto except on video page exit and can save progress manually via a button located beneath the video player.

Also updated video page action buttons layout to deal with all buttons visible and window with small width (See screenshots

Screenshots

New button on video page
image
image
image

Settings
image
image

Dropdown only left aligned on width < 380px (2 rows of buttons) otherwise centered like before
image
image
image

Testing

A. "Old" Modes

  • Test Auto mode acts the same as before
  • Test Never mode acts the same as before

B. New Mode

  • Update new setting to Semi-auto
  • Play any video, confirm new button & save progress at timestamp A, exit at timestamp B and ensure progress saved on A not B
  • Play any video, confirm progress saved when video ended (not on video page exit)
  • No idea how to test on error progress saving, add code to make it happen?

C. Setting migration

  • Checkout dev
  • Update settings to have {"_id":"saveWatchedProgress","value":false}
  • Checkout PR
  • Run app and ensure new setting set to Never (testing for Auto is optional coz it's default)

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) February 5, 2025 01:19
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 5, 2025
Comment on lines 44 to 49
watchedProgressSavingModeValueNamePairs() {
return [
['auto', this.$t('Settings.Privacy Settings.Watched Progress Saving Mode.Modes.Auto')],
['semi-auto', this.$t('Settings.Privacy Settings.Watched Progress Saving Mode.Modes.Semi-auto')],
['never', this.$t('Settings.Privacy Settings.Watched Progress Saving Mode.Modes.Never')],
]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please split this up into two separate array like we do for the other dropdowns. If you want to group them together we can place the two arrays next to each other when we migrate the privacy settings to the composition API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Split

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Feb 20, 2025
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

* 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
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 20, 2025
Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
@efb4f5ff-1298-471a-8973-3d47447115dc

Hmm this PR could close issue #447 and #609 if i read the descriptions of the issues correctly. Not sure about #964 though but maybe solutions from comments in #609 could be directed to that issue?

Thoughts?

@efb4f5ff-1298-471a-8973-3d47447115dc

Settings
409787999-161aaa1b-7a4a-48af-8f35-f99e97eaa486

Idk why but i dont like the placement of the dropdown at all it looks so out of place compared to all other settings. Are there other ways to style this to look "better"

@PikachuEXE
Copy link
Collaborator Author

#447 seems to be wanting time length based auto saving switch, #609 for percentage based
#964 seems to be similar to 447 but for much shorter time

This PR helps but not sure if that's entirely what they want

PikachuEXE and others added 2 commits February 26, 2025 08:55
Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
@PikachuEXE
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc
How about the one below? If not please tell me what works for you I can't guess~
image
image

@efb4f5ff-1298-471a-8973-3d47447115dc

Looks better imo

@PikachuEXE PikachuEXE requested review from ChunkyProgrammer, kommunarr and absidue and removed request for ChunkyProgrammer and absidue March 3, 2025 05:19
@MarmadileManteater
Copy link
Contributor

Question: do we think the option to save manual progress should be hidden for live streams?


.videoOptions :deep(.iconDropdown) {
.videoOptionsMobileRow :deep(.iconDropdown) {
inset-inline: auto calc(50% - 20px);
Copy link
Contributor

Choose a reason for hiding this comment

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

This style causes overflow on small width displays. It should probably float right in that case.
2025-03-11-120924_hyprshot

IMHO, it would look better centered on large displays:

how it looks centered
2025-03-11-121200_hyprshot 2025-03-11-121258_hyprshot

Additionally, this is nitpicky, but this should probably be handled through the existing props for ft-icon-button instead of deep styles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should probably be handled through the existing props

Resize handler to change dropdownPositionX prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

You know what. I didn't think about the resize handler in the JS side, and it is easier to handle responsiveness in CSS.

* development: (97 commits)
  Translated using Weblate (German)
  Translated using Weblate (Czech)
  Translated using Weblate (Basque)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Chinese (Simplified Han script))
  Translated using Weblate (Japanese)
  Translated using Weblate (Estonian)
  Translated using Weblate (Portuguese (Brazil))
  Bump the eslint group with 4 updates (FreeTubeApp#6985)
  Add support for channel's `Courses` tab (FreeTubeApp#6641)
  Bump electron from 34.3.0 to 35.0.1 (FreeTubeApp#6986)
  Bump shaka-player from 4.13.6 to 4.13.8 (FreeTubeApp#6990)
  Bump css-minimizer-webpack-plugin from 7.0.0 to 7.0.2 (FreeTubeApp#6988)
  Bump swiper from 11.2.4 to 11.2.5 (FreeTubeApp#6989)
  Bump lefthook from 1.11.2 to 1.11.3 (FreeTubeApp#6987)
  Update Invidious DASH xtags handling for YouTube.js changes (FreeTubeApp#6944)
  Translated using Weblate (French)
  Allow legacy formats to be used even when dash and audio-only are unavailable (FreeTubeApp#6977)
  Translated using Weblate (Hungarian)
  Fix search not working on mobile because of undefined ref (FreeTubeApp#6706)
  ...
@PikachuEXE
Copy link
Collaborator Author

Now dropdown style updated to

  • Dropdown only left aligned on width < 380px (2 rows of buttons) otherwise centered like before
    image
    image
    image

@PikachuEXE
Copy link
Collaborator Author

Updated to hide progress saving button on live/upcoming to be consistent with checks in actual progress saving

i.e. if (this.isUpcoming || this.isLive)

Question: do we think the option to save manual progress should be hidden for live streams?

@PikachuEXE PikachuEXE requested review from absidue and removed request for absidue March 13, 2025 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants