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

[kommunarr]

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 Theater Mode / 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
• Remove Enable Theatre Mode by Default Setting
• Minor settings changes:
  • en-us:
    • Turn on Captions by Default -> Enable Captions by Default (for visual consistency)
  • 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

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.

[efb4f5ff-1298-471a-8973-3d47447115dc]

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

VirtualBoxVM_M1Tz4qFemr.mp4

• Q: Not sure but wouldnt this PR make Integrate countdown inside the player instead of static message ouside of the player when autoplay is enabled #1214 more urgent to implement? If so should we do it here or in a followup?

[kommunarr]

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)

[absidue]

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.

[kommunarr]

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

[kommunarr]

Update: I cleaned this up in bb3be09

[absidue]

@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?

[kommunarr]

@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]

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]

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]

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]

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:

[github-actions]

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

[github-actions]

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

[absidue]

Copying my comment from above as I had left it on an already closed thread so it might have been missed:

Sorry to revive an already closed thread. I've changed my mind on this please use the shaka-player methods, that way we know that it will always behave the same. e.g. shaka-player's PiP methods support both normal PiP and document PiP (customisable PiP) but your code only supports the former, so when Electron adds support for document PIP, we'll either have to duplicate all the code for it from shaka-player or call shaka-player's method, which is what I am already asking you to do.

[absidue]

I agree with @efb4f5ff-1298-471a-8973-3d47447115dc while yes technically full screen by default and theatre mode by default are not mutually exclusive, it seems very unlikey that someone would want to combine them, as you would have to explicitly leave full screen to see theatre mode. So I'm on the side of moving theatre mode by default into the dropdown as a normal option, so making it mutually exclusive (definitely not one of those drop down inside a drop down things, that I thought we had agreed on a while back had bad UX) and for adding a migration into the datastore for migrating over existing users of the theatre mode by default setting.

[kommunarr]

I agree with @efb4f5ff-1298-471a-8973-3d47447115dc while yes technically full screen by default and theatre mode by default are not mutually exclusive, it seems very unlikey that someone would want to combine them, as you would have to explicitly leave full screen to see theatre mode. So I'm on the side of moving theatre mode by default into the dropdown as a normal option, so making it mutually exclusive (definitely not one of those drop down inside a drop down things, that I thought we had agreed on a while back had bad UX) and for adding a migration into the datastore for migrating over existing users of the theatre mode by default setting.

Will make these changes now. EDIT: done.

Sorry to revive an already closed thread. I've changed my mind on this please use the shaka-player methods, that way we know that it will always behave the same. e.g. shaka-player's PiP methods support both normal PiP and document PiP (customisable PiP) but your code only supports the former, so when Electron adds support for document PIP, we'll either have to duplicate all the code for it from shaka-player or call shaka-player's method, which is what I am already asking you to do.

@absidue I don't understand where I'm supposed to do this. Is there a way I can call the Shaka built-in functions from executeJavascript, or is there another section of the code that is being referred to?

[efb4f5ff-1298-471a-8973-3d47447115dc]

Sorry to revive an already closed thread. I've changed my mind on this please use the shaka-player methods, that way we know that it will always behave the same. e.g. shaka-player's PiP methods support both normal PiP and document PiP (customisable PiP) but your code only supports the former, so when Electron adds support for document PIP, we'll either have to duplicate all the code for it from shaka-player or call shaka-player's method, which is what I am already asking you to do.

@absidue I don't understand where I'm supposed to do this. Is there a way I can call the Shaka built-in functions from executeJavascript, or is there another section of the code that is being referred to?

Will approve after this change.

[absidue]

@absidue I don't understand where I'm supposed to do this. Is there a way I can call the Shaka built-in functions from executeJavascript, or is there another section of the code that is being referred to?

As mentioned in my original thread, you can access the ui property on the video element and then use it the same way as the ui variable in the ft-shaka-video-player component.

ipcMain.on(IpcChannels.REQUEST_FULLSCREEN, ({ sender }) => {
  sender.executeJavaScript('document.querySelector("video.player").ui.getControls().toggleFullScreen()', true)
})

ipcMain.on(IpcChannels.REQUEST_PIP, ({ sender }) => {
  sender.executeJavaScript('document.querySelector("video.player").ui.getControls().togglePiP()', true)
})

[kommunarr]

🤦🤦 I sincerely apologize, I completely missed that part when reading the diff and just saw the use of video.player! Just added and revalidated now.

[PikachuEXE]

I am waiting for @absidue to review IPC related stuff first

[github-actions]

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

[github-actions]

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