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

Native video player support #69

Merged
merged 9 commits into from Dec 23, 2022
Merged

Native video player support #69

merged 9 commits into from Dec 23, 2022

Conversation

AdelaideSky
Copy link
Contributor

@AdelaideSky AdelaideSky commented Dec 23, 2022

Heyoooo ! Another pull request loool

This time, i focused on adding a native player to the app, to allow not using the YouTube player that can sometimes be weird without needing IINA !

Here is what I did in detail:

  • Added the YoutubeKit module to extract stream urls from YT urls.
  • Removed SDWebImageSwiftUI and use SwiftUI's AsyncImage elements ( Maybe should change this to use SDWebImageSwiftUI again as i think there was a good reason for it lol )
  • Changed OnHover behaviour of video row to not hide the thumbnail but make it smaller
  • Changed Menubar icon to match more YT logo
  • Added videoClickBehaviour setting, to let the user customise the action performed on double clicking on a video row & made the corresponding GeneralSettingsView picker.
  • Added support for native player. The app fetch the stream url of the video, and displays a native player for this stream.
  • Added useNativePlayer setting for the user to choose between youtube player and native one.

ToDo / ways to improve :

  • Add support for DASH streams in the native player, as YT doesn't provide any good quality non-DASH streams.
  • Do the same thing for audio only playback (put controls in the bottom of the menubar view when playing).
  • Make player window staying in all the desktops (like Quick Look's window) ( ✅, eventually add customisable behaviour for if app should appear also on fullscreen apps etc)

If you want to wait before merging, i'm going to work on the things listed above :3

@Aayush9029
Copy link
Owner

Aayush9029 commented Dec 23, 2022

@Adelenade Thanks a lot for the PR!

Feel free to take a look at my review 😊

NativeYoutube.xcodeproj/project.pbxproj Show resolved Hide resolved
NativeYoutube/Models/Constants.swift Outdated Show resolved Hide resolved
NativeYoutube/Views/SharedViews/VideoRowView.swift Outdated Show resolved Hide resolved
NativeYoutube/Views/SharedViews/VideoRowView.swift Outdated Show resolved Hide resolved
NativeYoutube.xcodeproj/project.pbxproj Show resolved Hide resolved
@Aayush9029
Copy link
Owner

Aayush9029 commented Dec 23, 2022

Do you think there's a way to preserve aspect ratio when re-sizing?

CleanShot 2022-12-23 at 12 19 01@2x

Weird behaviour.

When pressing Command + W on video player the text still remains.

CleanShot 2022-12-23 at 12 19 14@2x

But closing using the X button removes it.

@Aayush9029
Copy link
Owner

You can also remove the StatusBarIcon file from assets, since we'll be using the better looking menu icon!

@AdelaideSky
Copy link
Contributor Author

For the aspect ratio:

I commented in View+Extension.swift that window.aspectRatio = NSMakeSize(16.0, 9.0) should work according to documentation (https://developer.apple.com/documentation/appkit/nswindow/1419507-aspectratio) but it doesn't seem to work. I'm going to try again, maybe that using the native player fixes this

And for the stop playing, it's simply because i didn't modified the command+w code lol, only the button's one :'D

and yep i'm removing it

.frame(width: hovered ? 64 : 128, height: hovered ? 36 : 72)
.cornerRadius(5)
.shadow(radius: 6, x: 2)
// ...and move it a bit on the left to avoid looking weird.
Copy link
Owner

Choose a reason for hiding this comment

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

I'm curious why the thumbnail was resized instead of "hiding" it.

Wouldn't the ability to see more title be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infact, i found that having the thumbnail disappearing when you hover was really weird, so i tried to make it smaller instead. I havent seen any video title that wasn't displayed entirely on hover so i kept it like that

Also, as thumbnails in Youtube are really important, i feel like you should be able to click on the thumbnail to play the video, and not having it disappear right when you wanna click on it :0

I'm sorry if i'm unclear but yeah.. i really feel like it's frustrating to not being able to catch the thumbnail with the mouse LOL (but ofc it's a design choice so i understand if you prefer hiding it :3)

Copy link
Owner

Choose a reason for hiding this comment

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

😅 definitely a design choice!

Since we can see the thumbnails before we hover on the video I think it's safe to hide them!

Ideally we'd animate the thumbnail YouTube style so it shows the preview of the video but I'll need to take a look at YouTube api for that!

Let's slide them out for now.

I'll consider adding a themes tab on settings so users can toggle between them and maybe even customize the app a little bit!

@AdelaideSky
Copy link
Contributor Author

Good news: i found a way to keep aspect ratio, it's simply a .aspectRatio modifier on the view :'D
Bad news: it acts weird (if you resize one time the window, you can only resize it via the height axis (it still keeps the aspect ratio) and it's hard to find the pixel where you can do it) <_>

@Aayush9029
Copy link
Owner

Good news: i found a way to keep aspect ratio, it's simply a .aspectRatio modifier on the view :'D
Bad news: it acts weird (if you resize one time the window, you can only resize it via the height axis (it still keeps the aspect ratio) and it's hard to find the pixel where you can do it) <_>

you didn't push this work around right....?

@Aayush9029
Copy link
Owner

Aayush9029 commented Dec 23, 2022

Loving the new video player! It's already started to grow on me!

I'm re-writing the floating tab extension so it'll be easier to work with in future.

@AdelaideSky
Copy link
Contributor Author

AdelaideSky commented Dec 23, 2022

Glad you like it !

Oh you mean re-writing the picture-in-picture-like window stuff ? :0

And by the way, have you ever tried to add a log-in with google account to the app ? Or is it too complicated ? (i don't really know the YouTube Api but idk why it would be impossible :0)

@AdelaideSky
Copy link
Contributor Author

AdelaideSky commented Dec 23, 2022

Quickly corrected a bug where the archived .app was crashing when playing a video (just needed to add AVKit.framework)

@Aayush9029
Copy link
Owner

Aayush9029 commented Dec 23, 2022

And by the way, have you ever tried to add a log-in with google account to the app?

I've experimented with authentication it's quite simple to implement (using googled SDKs)

However, irrc There's a quota limit for requests.

Since we won't be showing subscription feed or user specific feed, having an api key should be fine!

I think at most, I'll make a new how to get YouTube api key video. Even that seems unnecessary

@Aayush9029
Copy link
Owner

Aayush9029 commented Dec 23, 2022

@Adelenade There's a weird flicker when using non-native video player.

CleanShot.2022-12-23.at.16.18.22.mp4

Update: I'm getting rid of the player altogether!

@Aayush9029
Copy link
Owner

@Aayush9029 Aayush9029 merged commit ee5bdd9 into Aayush9029:main Dec 23, 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.

None yet

2 participants