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

Player UI Modernization #695

Merged
merged 35 commits into from
May 30, 2024
Merged

Player UI Modernization #695

merged 35 commits into from
May 30, 2024

Conversation

insidegui
Copy link
Owner

@insidegui insidegui commented Jun 8, 2023

  • Use SF Symbols for most controls
  • Add AirPlay button
  • Make player controls more compact
  • Player controls positioned/sized according to video content instead of whole player area

Before

PUIBefore

After

CleanShot 2024-05-29 at 21 26 27

@allenhumphreys
Copy link
Collaborator

allenhumphreys commented Jun 8, 2023

Looks great, noticed a few things:

  1. The window width is now constrained to be unnecessarily wide
  2. The book marks indicators a bit funny, could be existing issuesScreenshot 2023-06-08 at 1 50 20 PM
  3. I managed to create a bunch of book marks all at the same spot, then while removing them I managed to get the app to crash with a constraint not being finite! You can also crash it by just changing the playback speed a few times.
  4. This isn't from your work, but I noticed full screen mode doesn't use the external player status which feels inconsistent. In full screen, you just see the video poster image. This is likely because the entire player view moves to the full screen window.
  5. For the PiP external status view, the status view takes the full space of the view which results in a bit of a jarring UI transition when moving in and our of PiP
  6. The volume slider thumb is vertically misplaced
  7. We should still the UX of the time remaining label from QuickTime. Clicking it changes between total time and time remaining. They also use a - to signal that it is the time remaining. Bonus points if we took the tvOS feature of showing what time of day the video will finish :-P
  8. The volume icon could change as the slider changes, e.g. speaker.wave.3.fill -> speaker.wave.2.fill -> speaker.wave.1.fill
  9. The goforward/backward buttons look squished.

Loving the more SwiftUI, this UI layout is insanely hard to follow.

@insidegui
Copy link
Owner Author

I'm not discarding the possibility of redoing the whole player in SwiftUI tbh 😅

@allenhumphreys
Copy link
Collaborator

Surely the controls view could pretty easily be a swiftui view, it's basically just buttons. Maybe a binding or 2. It'd be like 500 fewer lines of code 😂

@insidegui
Copy link
Owner Author

Something else we can remove (regardless of a SwiftUI migration) is the external playback stuff, since that's been disabled with a compile-time flag for a while.

@sugarmo
Copy link

sugarmo commented Feb 29, 2024

Can we add subtitles to downloaded videos? The current situation is that even if a video has subtitles when played online, the subtitles will be lost as long as it is downloaded and played again. In fact, transcript can be used as subtitles. Is it difficult to implement this feature?

@insidegui
Copy link
Owner Author

@sugarmo That's tracked in #657, feel free to continue this discussion there

@insidegui
Copy link
Owner Author

The window width is now constrained to be unnecessarily wide

Fixed in c9379db

For the PiP external status view, the status view takes the full space of the view which results in a bit of a jarring UI transition when moving in and our of PiP

Fixed as part of 3780e5a
Specifically:

externalStatusController.view.leadingAnchor.constraint(equalTo: videoLayoutGuide.leadingAnchor).isActive = true
externalStatusController.view.trailingAnchor.constraint(equalTo: videoLayoutGuide.trailingAnchor).isActive = true
externalStatusController.view.topAnchor.constraint(equalTo: videoLayoutGuide.topAnchor).isActive = true
externalStatusController.view.bottomAnchor.constraint(equalTo: videoLayoutGuide.bottomAnchor).isActive = true

The volume slider thumb is vertically misplaced

Probably for some weird legacy reason it was using custom drawing, just reverted to a regular NSSlider with .small control size: 8c9bb0c

We should still the UX of the time remaining label from QuickTime. Clicking it changes between total time and time remaining. They also use a - to signal that it is the time remaining. Bonus points if we took the tvOS feature of showing what time of day the video will finish :-P

Implemented in 0c0fb0a

I like the idea of having the time of day, will keep that in mind for later :)

The volume icon could change as the slider changes, e.g. speaker.wave.3.fill -> speaker.wave.2.fill -> speaker.wave.1.fill

Done in 7f3d5a5

The goforward/backward buttons look squished.

Not sure what happened (maybe different SDKs / macOS versions) but they look fine now, macOS 14.5 building with Xcode 15.3

@allenhumphreys
Copy link
Collaborator

We should still the UX of the time remaining label from QuickTime.

Steal* 🤪

Nothing like correcting the internet record 1 year later!

@insidegui
Copy link
Owner Author

The book marks indicators a bit funny, could be existing issues

Redesigned in 5d304f4

CleanShot 2024-05-29 at 18 24 29

I managed to create a bunch of book marks all at the same spot, then while removing them I managed to get the app to crash with a constraint not being finite! You can also crash it by just changing the playback speed a few times.

Not sure what the crash was about, but since we don't do clustering or some other solution for now, I've introduced a minimum distance of 30s between bookmarks, which should not be a problem for regular use (I usually have just a few bookmarks in any given session, and they're never really close together like that). Also done in 5d304f4

Extra stuff

  • The player view now handles hover tracking for the timeline, reducing the precision required to interact with the timeline for seeking / previewing; this is achieved by expanding the vertical area of the timeline and requiring the cursor to move out further away from the visible area before disengaging the hover interaction
  • Restored previous behavior where player controls can be interacted with while the app is in the background (except for the timeline)
  • New playback speed control (borrowed the implementation from another branch)
  • Cute animation for the play/pause button
  • I can't believe this wasn't already the case, but most player settings such as volume, the mode for the time left / duration label, and playback speed are now persisted (a4c4fb8, 28f7733)
  • When right-clicking the playback speed control, there's a "Custom" option that allows the user to enter any custom speed between 0.25 and 3.5 (0fd5260)
  • Improved the look of the floating time preview that shows up when hovering over the timeline or editing bookmarks
speed.mp4

@insidegui insidegui marked this pull request as ready for review May 30, 2024 00:25
@insidegui
Copy link
Owner Author

I managed to create a bunch of book marks all at the same spot

Mitigated in 9bd2834

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

3 participants