Rise-Media-Player icon indicating copy to clipboard operation
Rise-Media-Player copied to clipboard

Added file browser control

Open d2dyno1 opened this issue 2 years ago • 6 comments

d2dyno1 avatar Apr 24 '22 13:04 d2dyno1

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:

  • In App.xaml.cs, you have FileBrowserPageViewModel with a public setter. Please make the setter private.
  • BreadcrumbItemViewModel would fit more nicely in the Models project, as a model (that is, just name it Breadcrumb or something). ViewModels are mostly there to wrap models with property change notifications and stuff the view may need, and because you don't seem to need those, naming it ViewModel is a little misleading. This also applies to stuff like FileBrowserHomePageViewModel.
  • There's FileBrowserHomePage which, correct me if I'm wrong, but it seems to be empty? If it isn't being used, remove it.

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

YourOrdinaryCat avatar May 11 '22 02:05 YourOrdinaryCat

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.

d2dyno1 avatar May 27 '22 19:05 d2dyno1

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.

YourOrdinaryCat avatar May 28 '22 01:05 YourOrdinaryCat

Any news regarding the file browser?

itsWindows11 avatar Aug 11 '22 08:08 itsWindows11

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

@josephbeattie How's progress on that?

d2dyno1 avatar Aug 12 '22 20:08 d2dyno1

@d2dyno1 please resolve the conflicts.

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

itsWindows11 avatar Sep 19 '22 21:09 itsWindows11