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

Removed MVVM Light #68

Merged
merged 7 commits into from
May 25, 2022
Merged

Removed MVVM Light #68

merged 7 commits into from
May 25, 2022

Conversation

TimGels
Copy link
Member

@TimGels TimGels commented May 22, 2022

Removed the MVVMLight package as it is deprecated and added the Microsoft.Toolkit.Mvvm package as the successor. Some implementations had to be redone as MVVM Toolkit didn't provide a drop in replacement. An example for such case is the clearing/ unregistering of a viewmodel.

I have added a method in the code behind of the main page that checks if it will navigate to the login page. If that is the case, it clears the navigation cache resulting in all cached pages being cleared (except for the pages using the 'required' navigation cache mode). The reason I added the cache mode enabled to the main page is so that users don't have to navigate through their directory, after navigating back to the main page after every navigation.

Reference: #66

@TimGels TimGels requested a review from JohannesKauffmann May 22, 2022 20:24
@TimGels TimGels force-pushed the replace-mvvmlight branch 4 times, most recently from 0d143c3 to 19dc6f0 Compare May 22, 2022 20:41
Removed the MVVMLight package as it is deprecated and added the
Microsoft.Toolkit.Mvvm package as the successor. Some implementations
had to be redone as MVVM Toolkit didn't provide a drop in replacement.
An example for such case is the clearing/ unregistering of a viewmodel.
I have added a method in the code behind of the main page that checks if
it will navigate to the login page. If that is the case, it clears the
navigation cache resulting in all cached pages being cleared (except for
the pages using the 'required' navigation cache mode). The reason I
added the cache mode enabled to the main page is so that users don't
have to navigato through their directories after navigation back to the
main page after every navigation.
@TimGels TimGels force-pushed the replace-mvvmlight branch from 19dc6f0 to bc5c4e3 Compare May 22, 2022 20:43
@JohannesKauffmann
Copy link
Collaborator

  1. start app
  2. play video, set volume op 100
  3. go back
  4. play video again

On master, at step 4, the volume slider is at 100, while this branch has a regression where the slider goes to 0 even though there is audio playing.

@TimGels
Copy link
Member Author

TimGels commented May 22, 2022

  1. start app

    1. play video, set volume op 100

    2. go back

    3. play video again

On master, at step 4, the volume slider is at 100, while this branch has a regression where the slider goes to 0 even though there is audio playing.

I can reproduce this. Yes this seems to be a regression in the way how the VM is kept or not kept in memory or something.

OneDrive-Cloud-Player/App.xaml Outdated Show resolved Hide resolved
OneDrive-Cloud-Player/App.xaml.cs Outdated Show resolved Hide resolved
OneDrive-Cloud-Player/App.xaml.cs Show resolved Hide resolved
OneDrive-Cloud-Player/Views/LoginPage.xaml Show resolved Hide resolved
OneDrive-Cloud-Player/Views/MainPage.xaml.cs Show resolved Hide resolved
With MVVMLight, the static volumeUpdated was initialized to false on
every navigation action to the page. With the new navigation, the
static volumeUpdated actually held it's value which resulted in the
volume not being updated after navigating to the page for a second
time. This has been fixed by making the variable not static, so it
is initialized every time correctly.

Also added an assertion to self-document this, and added clarification
comment.
@TimGels TimGels linked an issue May 24, 2022 that may be closed by this pull request
@TimGels TimGels force-pushed the replace-mvvmlight branch from 77158d9 to fce3d53 Compare May 25, 2022 20:40
@TimGels TimGels requested a review from JohannesKauffmann May 25, 2022 21:02
@TimGels TimGels merged commit c719283 into master May 25, 2022
@TimGels TimGels deleted the replace-mvvmlight branch May 25, 2022 21:15
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.

Deprecated MVVM Light package
2 participants