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

Added file browser control #167

Closed d2dyno1 closed 1 year ago

YourOrdinaryCat commented 2 years ago

Hello there, I've been taking a look at the code, sorry for the delay on the comment. I think it would help if you spread the code a little across the different projects - stuff like ViewModels could be moved to the Rise.Data project for example. I think messages, services, and file explorer views would fit in the Rise.Storage project you created as well.

This is mostly done to split up the project more nicely, and compile times in UWP C# projects go way up if a single project has a lot of stuff, specially when it comes to XAML (bug in VS that never got fixed shrugs). Some extra comments:

Other than that, stuff looks nice. Thanks for your contribution! Tell me if you need help with anything or have any questions.

d2dyno1 commented 2 years ago

Hi @YourOrdinaryCat! Sorry for late reply, but I got tied up in school 🤷 Moving ViewModels to Rise.Data would be out of scope for this PR as it'd require some additional changes however, I made that easy by not referencing any UI namespaces.

Rise.Storage was created to provide abstractions to be consumed by both frontend and backend projects (RiseMP doesn't currently support backend and frontend projects separation though). In regards about the IStorageService -- it's my personal preference to put the service in the backend instead of Rise.Storage since it shouldn't need to use it.

I'm not sure if BreadcrumbItemViewModel should be a Model, its properties are bound to the UI so my reasoning was to make it a View-Model.

Some pages are empty but will be implemented later.

YourOrdinaryCat commented 2 years ago

I see, that's fair. I too have been tied up by studies lately.

Everything seems good, thanks for the explanation. Do tag me whenever you think the PR can be merged.

itsWindows11 commented 2 years ago

Any news regarding the file browser?

d2dyno1 commented 2 years ago

Hi! Currently I'm waiting for @josephbeattie to do the xaml part 👀

@josephbeattie How's progress on that?

itsWindows11 commented 2 years ago

@d2dyno1 please resolve the conflicts.

@josephbeattie told me he won't work on the UI.