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 "Picture in picture" plugin #674

Merged
merged 13 commits into from
Apr 9, 2022
Merged

Add "Picture in picture" plugin #674

merged 13 commits into from
Apr 9, 2022

Conversation

th-ch
Copy link
Owner

@th-ch th-ch commented Apr 7, 2022

This PR implements a raw version of PiP to resolve #656 - first version re-uses the main window as having a new one requires way more work.

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 8, 2022

  • The expand menu background is black and looks weird (should maybe be transparent when in "PiP" mode)
    image
    EDIT:

    ytmusic-player-bar.pip ytmusic-player-expanding-menu {
      background-color: transparent;
    }

    works but without the background the expand menu items are directly on top of the main controls and it looks even worse,
    so maybe something like

    ytmusic-player-bar.pip ytmusic-player-expanding-menu {
      border-radius: 30px;
      background-color: rgba(0, 0, 0, 0.3);
      backdrop-filter: blur(5px) brightness(20%);
    }
  • There are also 2 errors because of macOS specific code:

    • TypeError: win.setWindowButtonVisibility is not a function at togglePiP (C:\Git\youtube-music\plugins\picture-in-picture\back.js:51:6)
    • TypeError: Cannot read properties of undefined (reading 'hide') at togglePiP (C:\Git\youtube-music\plugins\picture-in-picture\back.js:29:12)
  • ⚠️ trying to log in after having used "PiP" causes the app to freeze ⚠️, not sure why (only happens after having toggle PiP atleast once)
    EDIT: @th-ch I figured it out, its because of the css

    body {
     -webkit-app-region: drag;
    }

    It doesn't even seems to work in the main app or pip, but it does work in the login screen which makes everything unselectable

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 8, 2022

sorry ignore what I said about the size and name and all that, I had unresolved bugs which made it behave weird

  • It needs to behave correctly when entering/exiting fullscreen mode (esc / f keys) because right now thats buggy

  • there should be a plugin option to set a hotkey for PiP

  • I don't think that alwaysontop should be enabled by default since you can just enable it in the options
    (maybe a plugin option to automatically enable alwaysontop but it shouldn't force this behavior)

  • EITHER: PiP mode should be saved and restored on app restart. OR: Original window size+position should be restored on quit
    (best would be a plugin option like "Remember PiP on exit" and defaults to restoring the og window)

  • SongMenu should maybe be tweaked to keep only essential options when PiP is active
    (In a followup PR maybe also minify the main menu if in-app-menu is active since it gets cut)

  • PiP doesn't work (black screen) if started while song is minimized (Miniplayer mode)
    can be fixed by unminimizing the player page before going fullscreen, something like:

    // Go fullscreen
    if (!document.querySelector('ytmusic-player-page').playerPageOpen_) {
      			document.querySelector('.toggle-player-page-button').click();
    }
    document.querySelector(".fullscreen-button").click();
  • style shouldn't be injected on pip activated but rather on

     module.exports = (win) => {
      injectCSS(win.webContents, path.join(__dirname, "style.css"));

    (can use css rules if it needs to be dynamic)

  • Maybe add some shadow to the icons/text in the player-bar so that they can be seen on a white background

    ytmusic-player-bar.pip svg, ytmusic-player-bar.pip yt-formatted-string {
      filter: drop-shadow(2px 4px 6px black);
      color: white;
    }
    // Go fullscreen
    document.querySelector(".fullscreen-button").click();
    document.querySelector("ytmusic-player-bar").classList.add("pip");
    // Exit fullscreen
    document.querySelector(".exit-fullscreen-button").click()
    document.querySelector("ytmusic-player-bar").classList.remove("pip");

Good work btw 😄

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 9, 2022

some changes related to other plugins that should maybe come in follow up PR's:

  • precise-volume hud position needs to be tweaked if both pip AND in-app-menu are active
    (the volume hud is positioned inside the in-app-titlebar somehow)

  • since in-app-menu is contained within the window, it shows only the topmost options in this mode, thus the most important menu options should maybe be moved up (View-exit/restart) (Options-AlwaysOnTop)

  • video-toggle should either be disabled or needs its own pip mode where it fades away unless hovered upon, also change its position to be a child of$('ytmusic-app-layout') (looks like it might be a better position even out of pip anyways)

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 9, 2022

  • right now its pretty annoying to exit the pip mode. there are 2 good solutions that I think could be both implemented:

    1. in togglePiP when PiP is activated ,register click event on the playerbar's exit fullscreen button
      $('.exit-fullscreen-button').addEvent -> onClick = togglePiP() / togglePictureInPicture()
      and when PiP is deactivated remove the listener
    2. like I wrote above, a custom keybind to toggle PiP
  • on PiP start: if browserWindow is maximized, unmaximize it to avoid weird interaction (/+ fullscreen?)

  • some more css changes for more visibility in the playerbar:

    ytmusic-player-bar.pip svg,
    ytmusic-player-bar.pip .time-info,
    ytmusic-player-bar.pip yt-formatted-string,
    ytmusic-player-bar.pip .yt-formatted-string {
        filter: drop-shadow(2px 4px 6px black);
        color: white !important;
        fill: white !important;
    }
    • fix playerbar's artist/album string which instead of being yt-formatted-string element they are links that have that as class.
    • fix .timeInfo which is a span that shows the current time (only when window is bigger than default PiP but still)
    • fix add !important to color to fix it not applying to artist name
    • fix fill: white !important mainly to fix #button.dropdown-trigger that didn't get filled by using color

the way I structured my comments with checkmarks might make it looks categorical, I did it do easily see stuff that was done since you did do most of them - but its just opinions and up for debate of course 💬
btw it feels kinda weird to write so much stuff on your pr since you are the project owner, I hope its okay 😅

@th-ch
Copy link
Owner Author

th-ch commented Apr 9, 2022

Interesting feedback, thanks for the involvement! The PR is getting big and it gives a first version of the plugin (not perfect but good enough for a first iteration) so I'll merge it as is, and we can follow up with the remaining improvements (which make sense)! 🚀

@th-ch th-ch merged commit 57ec0a4 into master Apr 9, 2022
@th-ch th-ch deleted the picture-in-picture-plugin branch April 9, 2022 19:53
@Araxeus
Copy link
Collaborator

Araxeus commented Apr 9, 2022

I don't think that alwaysontop should be enabled by default since you can just enable it in the options
(maybe a plugin option to automatically enable alwaysontop but it shouldn't force this behavior)

^ do you agree with that?

should I open PR's about stuff I wrote since you closed this?

@th-ch
Copy link
Owner Author

th-ch commented Apr 9, 2022

definitely, making it a user choice is better than forcing it!

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 9, 2022

also im thinking maybe its better if the PiP button is inserted on the video overlay (like the "quality changer" button )
image
which would be way easier to access

(and then to exit that you could press the exit-fullscreen button i mentioned above / use the custom keybind)

@Araxeus Araxeus mentioned this pull request Apr 10, 2022
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.

[feature request] picture-in-picture support
2 participants