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

MVVM #160

Open
esibruti opened this issue Mar 28, 2022 · 6 comments
Open

MVVM #160

esibruti opened this issue Mar 28, 2022 · 6 comments
Labels
enhancement New feature or request triage-approved Issues that have been approved by the Rise Media Player team and awaiting a fix

Comments

@esibruti
Copy link
Contributor

What's the Problem?

We have to do a lot of work on the code part. UWP is meant to be programmed with the MVVM pattern. RiseMP, from what I have seen, is very poor on this.
For maintenance it becomes much better and much easier. Many UWP applications use MVVM.

It has to be done during Alpha Preview / Alpha.
A step in this direction is to from now on program / add new features using the MVVM pattern.

Solution/Idea

Ex: We have to pass the code in NowPlayingBar.xaml.cs to a ViewModel.

Alternatives

N/A

Rise Media Player Version

No response

Windows Version

No response

Comments

No response

@itsWindows11
Copy link
Collaborator

itsWindows11 commented Mar 28, 2022

This is an important issue to work on so I'll pin this.

We have to pass the code in NowPlayingBar.xaml.cs to a ViewModel.

That's not the only thing we should do tbh, there's a LOT more than this.

We have to do a lot of work on the code part. UWP is meant to be programmed with the MVVM pattern. RiseMP, from what I have seen, is very poor on this.

Not being rude, but @josephbeattie and @rounk-ctrl messed up on that part a bit, assuming they're both beginners.

Also btw, not everything needs to be in the MVVM pattern.

@itsWindows11 itsWindows11 pinned this issue Mar 28, 2022
@esibruti
Copy link
Contributor Author

esibruti commented Mar 28, 2022

Also btw, not everything needs to be in the MVVM pattern.

I agree with you, not everything will need to move to a ViewModel but we have to start prioritizing that because to do maintenance and add functionality, right now, it gets confusing and complicated since it is "all spread out".

This is one reason why there are so few contributors to the Rise Media Player.

@esibruti esibruti mentioned this issue Mar 31, 2022
1 task
@YourOrdinaryCat
Copy link
Collaborator

I also think we should debloat some of the ViewModels and outright remove a few, some do stuff that they shouldn't be responsible for and others just wrap a model without property change notifications.

There's also a lot of singletons in App.xaml.cs, some of which I think would work better as static classes.

@esibruti
Copy link
Contributor Author

esibruti commented Apr 1, 2022

Wouldn't it be better to start working on these changes for the next version?

@YourOrdinaryCat
Copy link
Collaborator

I was working on improving that internally, but there's lots of code and I don't have much time. Certainly will be improved by the next release though.

@esibruti
Copy link
Contributor Author

esibruti commented Apr 1, 2022

I was working on improving that internally, but there's lots of code and I don't have much time. Certainly will be improved by the next release though.

The best way to start is when we mess with parts of the code that is not in MVVM, is to do like this or even, new features and things like that is to do it right the first time

@itsWindows11 itsWindows11 added enhancement New feature or request triage-approved Issues that have been approved by the Rise Media Player team and awaiting a fix labels Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage-approved Issues that have been approved by the Rise Media Player team and awaiting a fix
Projects
None yet
Development

No branches or pull requests

3 participants