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

Stay in fullscreen/fullwindow/PiP + default viewing mode setting #5903

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

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 21, 2024

Stay in fullscreen/fullwindow/PiP + default viewing mode setting

Pull Request Type

  • Feature Implementation

Related issue

closes #5427
#2295

Description

  • Stay in fullscreen, fullwindow, or PiP for the next video in an viewing session if one or more of these viewing modes is active upon the route changing
  • Add Default Viewing Mode dropdown setting for whether to enable fullscreen / fullwindow / PiP / external player by default (i.e., the same way that Enable Theater Mode by Default works)
    • Area for future improvement: the External Player option corresponds specifically to when an ft-list-video is interacted with, not any YT links found in a description or entered into the search bar. This was not implemented because we would have to implement logic for creating the external player video config from a URL, which is somewhat marginal and not sufficiently germane to this PR.
    • Note: the External Player option does not appear as an option when no external player is set
      • If the setting was changed to External Player prior to this, it is de facto set to Default until a new external player is set
  • Minor settings changes:
    • en-us:
      • Enable Theatre Mode by Default -> Enable Theater Mode by Default
      • Turn on Captions by Default -> Enable Captions by Default (for visual consistency / grouping with its compatriot)
    • View Playback Rate Interval input moved from the same ft-flex-box with the sliders -> colocated with the other ft-inputs (it appeared odd visually and was not a sufficiently logical grouping)
    • Video Player Settings flex box of ft-inputs moved to above the flex box of ft-sliders (better logical/related grouping)

Screenshots

Before After
Before Settings Settings After

Testing

  1. With A) playlist / B) recommended videos autoplay enabled, set the Default Viewing Mode to a) fullscreen / b) fullwindow / c) PiP. I also recommend lowering the autoplay countdown to 0s or 1s.
    1. Play a video and ensure that the viewing mode is properly set to enabled by default.
    2. While still in the viewing mode, skip to near the end of the video and let it autoplay. Upon the loading of the next video, the previously active viewing mode should still be active.
    3. Leave the viewing mode and X) let the video autoplay or Y) click on another video. The Default viewing mode should persist, NOT FS/FW/PiP.
    4. (Optional) Test 1iii with autoplay disabled.
  2. Enable PiP, then click any other recommended or future video. PiP should persist.
  3. Enable fullscreen / fullwindow on a video, skip to another video, then press Ctrl+Left Arrow. Verify that the previous video is opened in fullscreen / fullwindow.
  4. Ensure that with Default Viewing Mode of External Player, clicking on any ft-list-video will attempt to play it in the currently active external player. Any ft-list-video should also not have a redundant External Player icon.
  5. Then set the chosen External Player to None. The viewing mode should now function as default, and clicking on any ft-list-video will play it in the Watch page normally.
  6. (REG, optional) Test that Enable Theater Mode by Default enabled/disabled still works as intended.

Desktop

  • OS: MacOS
  • OS Version: Ventura

Additional context

I combined "Stay in fullscreen/fullwindow/PiP" with "default viewing mode setting" under one PR because the latter one is actually very little code, and testing them both together is more time efficient for you all. I can separate this out if desired, although I'd imagine it would not be.

Current limitations: does not work for the search bar, randomly encountered YT video links (e.g., in descriptions), or the video thumbnail link in the playlist list view.
…l player is set

This will prevent issues with users who accidentally change this setting and report that clicking on videos results in errors.
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 21, 2024 00:55
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 21, 2024
Theatre mode is not mutually exclusive with the viewing mode and thus should not be included here. This also saves us the work of having to update the default viewing mode to theatre mode on first load for 1-2 releases that we would have otherwise needed.
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 23, 2024

  • I still would like to see Theater Mode in there as it is a viewing mode
  • Scroll on Fullwindow shouldnt happen
VirtualBoxVM_M1Tz4qFemr.mp4

@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 23, 2024

I still would like to see Theater Mode in there as it is a viewing mode

See my thoughts on that here:

I still think we should keep the Enable Theater Mode By Default setting, as it is not mutually exclusive with the other viewing modes.

Also, good catch on that bug! Will look into that. Funnily enough, I think we actually have a feature request for something like this, but not as buggy looking I'd imagine, and not implemented by accident lol

Q: Not sure but wouldnt this PR make #1214 more urgent to implement? If so should we do it here or in a followup?

Not directly, since this PR only affects the behavior immediately following the countdown, but it would be good to have that UX issue resolved (separate PR though for sure)

@kommunarr kommunarr added PR: WIP and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 23, 2024
src/renderer/components/player-settings/player-settings.js Outdated Show resolved Hide resolved
Comment on lines 14 to 20
<component
:is="watchPageLinkType"
class="thumbnailLink"
tabindex="-1"
:to="watchPageLinkTo"
href="javascript:void(0)"
@click="handleWatchPageLinkClick"
Copy link
Member

Choose a reason for hiding this comment

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

As this code is quite confusing to read, it would probably be better to use a v-if and v-else here and for the second instance of the same thing below.

Copy link
Collaborator Author

@kommunarr kommunarr Oct 23, 2024

Choose a reason for hiding this comment

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

Got it, is there a way I can do this without duplicating the children for the v-else case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update: I cleaned this up in bb3be09

@absidue
Copy link
Member

absidue commented Oct 23, 2024

@kommunarr That bug is caused by not entering full window correctly, you just set the property to true but don't scroll to the top or add the CSS class that disables scrolling. Please make sure the features in this pull request behave exactly the same as the buttons and keyboard shortcuts do (that's what I meant by please don't reinvent the wheel in the private messages): https://github.com/FreeTubeApp/FreeTube/blob/development/src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js#L1649-L1661

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

I still would like to see Theater Mode in there as it is a viewing mode

See my thoughts on that #5427 (comment):

Hmm presenting my arguments for why i think it should be included in the dropdown.

  1. Consistency in Settings: Including Theater mode in the dropdown ensures consistency in how the user selects their preferred viewing mode. This uniformity makes the settings more intuitive and easier to use, as all modes are presented in the same way.

  2. User Preference: While Theater mode might not be mutually exclusive with Fullscreen or Picture-in-Picture, some users may prefer to set it explicitly as their default mode. Including it in the dropdown respects user preferences and provides a straightforward way for them to select their desired viewing experience.

  3. Clarity in Configuration: Having Theater mode in a separate toggle and the other modes in a dropdown can be confusing for users. Merging all viewing mode options into a single dropdown simplifies the settings interface and makes it clear that the user can choose between all available modes, including Theater mode.

To me as an user it isnt clear to me at all what will be my default viewing mode, is it fullscreen or theater?

image

@absidue absidue added the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 23, 2024
@kommunarr
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc I don't have much to say other than that your points are valid and it's a tradeoff either way. 937935f is the commit to drop if we want to put Theater Mode in there. I'm still leaning towards keeping them separate, but I am open to hearing more perspectives as well.

@PikachuEXE
Copy link
Collaborator

Just about Enable Theater Mode by Default
I think the issue is that in this PR the new option makes this option / label sound weird
It's actually default when Default Viewing Mode is Default, it's "secondary viewing mode" when others are selected
No idea how to represent this well

"Default Viewing Mode" now has

  • "default mode" (not theater mode)
  • theater mode
  • Full screen + "default mode" fallback
  • Full screen + theater mode fallback
  • Full window + "default mode" fallback
  • Full window + theater mode fallback
  • PiP + "default mode" fallback
  • PiP + theater mode fallback

@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 24, 2024

I was thinking about the same thing you were Pika, but I ultimately think we're overthinking it because we are assuming that people will inherently be asking themselves "but how does theater mode play into it" and get confused, when I don't think there is any inherent reason to do that or imply it if our settings structure doesn't.

If we have it as separate settings, that's an intuitive indication that viewing mode is orthogonal to whether theater mode is enabled, which is in fact the case in reality as well. For Fullwindow or Fullscreen, you have to actually exit that viewing mode to interact with things like comments and seeing the playlist / recommended videos, and you'll have to see a player that is in theater mode or isn't. If you're in PiP, the size of the grayed out video player is still in play if you want to toggle it off, and that video player is in theater mode or isn't.

I think we might be assuming that it is going to be a comprehension problem when we theorize it out in terms of number of total distinct states, but I believe that the UX of them being separate settings altogether makes the relationship much less complex and far more intuitive than we're making it out to be.

@PikachuEXE
Copy link
Collaborator

I simply find having 2 "default" a bit confusing
A short tooltip on theater mode option like "This also decides the fallback mode for some viewing modes" or a tooltip on the new option (no idea about the text) would be clearer

Also can you add some new lines for long paragraphs :S

@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 24, 2024

I'm open to any potential smaller suggestions on what can be done in terms of updating the labels or icons. One that you mention might be renaming the Default option to something else, like Normal. The big one may be that we have the faDisplay icon for the Default Viewing Mode dropdown, which is pretty similar to the tv icon for Theater Mode and creating an unnecessary connection. I'm trying to find something in the fontawesome library that's more like YT's miniplayer icon. I will say that even making it seem like they're connected (which again, practically speaking, they are not, other than in the narrow sense that "You can't see how big the player is while you are currently in FS/FW") through a clarifying tooltip could more confusing than just not mentioning it at all.

Edit: Updated now to use faExpand, which I think might be it:

Screenshot 2024-10-23 at 8 36 46 PM

@ChunkyProgrammer ChunkyProgrammer removed the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 27, 2024
@kommunarr kommunarr removed the PR: WIP label Oct 27, 2024
@kommunarr kommunarr added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 27, 2024
@kommunarr
Copy link
Collaborator Author

One case I haven't implemented: staying in PiP when you click another video without letting it autoplay. It's a bit specific, but I am thinking about it.

@absidue
Copy link
Member

absidue commented Oct 28, 2024

As PiP and full screen by default are only supported in Electron, please make sure that those options are hidden in the UI outside of Electron (the screenshot settings are like that so you can take inspiration for how it is handled there) and that if the user does have those values set (e.g. copying the settings.db from FreeTube desktop to FreeTubeAndroid) that it acts as if the settings are not set, so that it doesn't break anything.

This allows staying in PiP when clicking on other videos, staying in fullscreen/fullwindow when using Ctrl+Left Arrow / Ctrl+Right Arrow, and staying in PiP when using the watch-video-playlist Play Prev / Play Next buttons.
@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 28, 2024

Updated logic to have the reincarnate-with player state variables saved on player destroy instead of video ended, thus allowing staying in PiP when clicking on an arbitrary video, staying in fullscreen/fullwindow when using Ctrl+Left Arrow / Ctrl+Right Arrow, and staying in PiP when using the watch-video-playlist Play Prev / Play Next buttons. Also updated the settings dropdown to check IS_ELECTRON for the fullscreen and PiP settings.

@@ -928,6 +928,14 @@ function runApp() {
return app.getPath('pictures')
})

ipcMain.on(IpcChannels.REQUEST_FULLSCREEN, ({ sender }) => {
sender.executeJavaScript('document.getElementById("videoContainer").requestFullscreen({navigationUI: "hide"})', true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: why is this required? (ipcRenderer.send(IpcChannels.REQUEST_FULLSCREEN) then this

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.

[Feature Request]: Enable Preferred Viewing Mode by Default
5 participants