Rise-Software / Rise-Media-Player

One media player for everything you own or stream; whether it's music or videos, online or offline Rise Media Player does it all. And it's beautiful and native with the latest version of WinUI.
GNU General Public License v3.0
1.07k stars 76 forks source link

Now Playing code improvements #164

Closed YourOrdinaryCat closed 2 years ago

YourOrdinaryCat commented 2 years ago

Resolved / Related Issues

169

Details of Changes There are multiple Now Playing controls, some duplicates of each other in some areas. This PR attempts to fix that issue. Current plan goes as follows:

Validation Not needed yet, will update as soon as the previous bars are replaced.

Screenshots / Videos (optional) Media Players as of May 29:

Small Main Player

Medium Main Player

Large Main Player

Small Now Playing

Medium Now Playing

Large Now Playing

YourOrdinaryCat commented 2 years ago

The new media bar was added to allow for easier customization - it will be DataTemplate driven and will make it easier to customize the design.

The MediaPlaybackViewModel will be a reimplementation of PlaybackViewModel with better documentation, improved methods, and new events. This is to make everything easier to use and understand.

This PR will also define a simple interface with all needed properties to display media items.

YourOrdinaryCat commented 2 years ago

I'll be working on subclassing the default MediaTransportControls directly, which would essentially give us the system behavior for free while still being able to work on custom addons. The new control will still be there, but subclassing the MTC directly will help a lot.

YourOrdinaryCat commented 2 years ago

Still a work in progress, but I'm planning to add a few things that will make working with the transport controls much easier. We also have a compact bar now, if that's needed. For now, screenshots:

image

image

image

josephbeattie commented 2 years ago

https://www.figma.com/file/uOqnndf1l14u4IFNtk7BM4/RiseMP-Design-Toolkit?node-id=0%3A1

This figma link is a lil messy, but just go to the now playing bar section and you’ll see some rough designs, including a compact mode etc. Also, see if you can add an option to just show video thumbnail/album art in the bottom right corner. This looks 100x better! Will it make live scrubbing work correctly?

itsWindows11 commented 2 years ago

Oop I accidentally deleted this thinking I pressed on reply, sorry @josephbeattie.

itsWindows11 commented 2 years ago

Still a work in progress, but I'm planning to add a few things that will make working with the transport controls much easier. We also have a compact bar now, if that's needed. For now, screenshots:

image

image

image

There's too much spacing between the slider and the volume icon, unlike the now playing bar that's used currently.

esibruti commented 2 years ago

Still a work in progress, but I'm planning to add a few things that will make working with the transport controls much easier. We also have a compact bar now, if that's needed. For now, screenshots:

image

image

image

So, this way, with these changes, the sound problem in the application will be solved? (The sound problem is that when you change the sound in the now playing bar, it doesn't stay the same throughout the program)

And another problem, will the glyphs for windows 10 also look like this or will they continue to not appear in some places in the playing bar?

YourOrdinaryCat commented 2 years ago

I tested on Windows 10, took the screenshots on an 11 VM. Both show the icons correctly.

YourOrdinaryCat commented 2 years ago

The control template and derived classes were added in the 512ea6334d714618f183ae87230ed1f85776251d commit.

YourOrdinaryCat commented 2 years ago

@itsWindows11 instead of changing button visibility directly in the template, please do it with the default props:

https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.mediatransportcontrols?view=winrt-22000#properties (specifically the Is[Blank]ButtonVisible and Is[Blank]Enabled props)

This is to make what actually gets enabled clearer and to be able to hide/disable controls through visual states, you could set the values in the default template for RiseMediaTransportControls.

YourOrdinaryCat commented 2 years ago

Latest commit only has the definitions + docs. The methods don't do anything yet.

YourOrdinaryCat commented 2 years ago

Last commit just added an instance of it* typo sorry

YourOrdinaryCat commented 2 years ago

From the next commit onwards, I'll be working on making stuff use the MPViewModel. UI improvements will follow soon.

YourOrdinaryCat commented 2 years ago

Yes.

YourOrdinaryCat commented 2 years ago

I may redo the Now Playing pages, there's a lot of trickery with the MainPage that would take a while to get working and it's exception prone with the new playback handling.

YourOrdinaryCat commented 2 years ago

The last two commits were made to fix a Now Playing bug, where it would crash the app if the thumbnail was null. The thumbnail was previously not fetched when activating the app from a file, which caused a crash that is now fixed. It also makes some of the code logic when working with albums much easier to follow.

YourOrdinaryCat commented 2 years ago

For anyone interested in playback related methods such as PlaySingleItemAsync, I made it so that you don't have to pass a cancellation token or wrap it inside a try catch, you can call them as much as you wish and they'll work fine. Of course, you'll still get exceptions if something unrelated to cancellation goes wrong.

YourOrdinaryCat commented 2 years ago

I'm currently trying to lower the reference count of the original PlaybackViewModel - this is why you see a bunch of commits just deleting files or removing dependencies. I'll update the comment at the top with the current plan, to make things like this more clear.

YourOrdinaryCat commented 2 years ago

Last commit starts with some work on the UI.

cc @josephbeattie, may interest you.

itsWindows11 commented 2 years ago

uhm there's an issue image

YourOrdinaryCat commented 2 years ago

o h, I think the last few commits fixed that

itsWindows11 commented 2 years ago

What about compact overlay mode?

itsWindows11 commented 2 years ago

o h, I think the last few commits fixed that

nope, still crashing when clicking on the fullscreen button

itsWindows11 commented 2 years ago

Seems like the state manager wasn't able to access the elements inside the transport controls, so moved the code over.

YourOrdinaryCat commented 2 years ago

Alright, thanks

YourOrdinaryCat commented 2 years ago

Seems like the controls don't change between states anymore. Do we need the fullscreen button anyways? We're using the restore button and artwork for that aren't we?

itsWindows11 commented 2 years ago

Seems like the controls don't change between states anymore. Do we need the fullscreen button anyways? We're using the restore button and artwork for that aren't we?

I think the fullscreen button would be useful on the now playing and video playback page, but not with the default implementation (make the page fullscreen instead of the control).

For states we could just change them in the code, if visual states don't work.

YourOrdinaryCat commented 2 years ago

So maybe a custom button for those, and we disable the current one on the MainPage?

itsWindows11 commented 2 years ago

yup, btw can we continue in email to not flood this PR with comments?

YourOrdinaryCat commented 2 years ago

Sure thing, also just improved the VisualStates a little, gonna add some animation now.

YourOrdinaryCat commented 2 years ago

Updated pictures of the Now Playing bars are now available in the first comment.