Skip to content

Apply MVVM for profiles in SUI#11877

Merged
1 commit merged intomainfrom
dev/cazamor/eim/mvvm-profiles
Dec 8, 2021
Merged

Apply MVVM for profiles in SUI#11877
1 commit merged intomainfrom
dev/cazamor/eim/mvvm-profiles

Conversation

@carlos-zamora
Copy link
Member

Summary of the Pull Request

Cleans up ProfileViewModel, Profiles, and ProfilePageNavigationState to move all of the view model responsibilities over to ProfileViewModel. We don't actually store the ProfilePageNavigationState anymore. We only use it as a way to transfer information to the new page.

References

#9207 - Apply MVVM

Detailed Description of the Pull Request / Additional comments

  • I pulled out ProfileViewModel into its own file to keep things cleaner. It was getting pretty big.
  • The font lists are now stored in a static location in ProfileViewModel, which means that we can reuse the same list between pages.
  • the profile pivot was also moved to the ProfileViewModel and stored as a static value.

Validation Steps Performed

✅ pivot behavior is the same
✅ font list is still populated

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still cache the font list when you navigate away and then back to a profile / between profiles?

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wasn't as big a deal as I thought it was going to be when you initially proposed it. Nice.

@carlos-zamora

This comment has been minimized.

@carlos-zamora
Copy link
Member Author

Does this still cache the font list when you navigate away and then back to a profile / between profiles?

This should cache the list, which is a nice bonus.

@carlos-zamora

This comment has been minimized.

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 8, 2021
@ghost
Copy link

ghost commented Dec 8, 2021

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 0939eec into main Dec 8, 2021
@ghost ghost deleted the dev/cazamor/eim/mvvm-profiles branch December 8, 2021 00:25
{
_LastActivePivot = lastState.LastActivePivot();
}
auto profile{ winrt::get_self<ProfileViewModel>(viewModel) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW doing this when a navigation state is constructed is unusual -- this should happen to the VM after the navigation has completed.

Nav state should only be used to communicate incoming data to the view/viewmodel, not to actually enact change on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. Adding this as a comment under the main MVVM issue.

@zadjii-msft zadjii-msft mentioned this pull request Jan 12, 2022
6 tasks
ghost pushed a commit that referenced this pull request Jan 28, 2022
## Summary of the Pull Request
This fixes a bug where several settings would not show the reset button. The root cause of this issue is two fold:
1. Hooking up `CurrentXXX`
   - `GETSET_BINDABLE_ENUM_SETTING` was hooked up to the **settings** model profile object instead of the **view** model profile object. Since the settings model has no `PropertyChanged` system, any changes were directly being applied to the setting, but not notifying the view model (and thus, the view, by extension) to update themselves.
   - This fix required me to slightly modify the macro. Rather than using two parameters (object and function name), I used one parameter (path to getter/setter). 
2. Responding to the `PropertyChanged` notifications
   - Now that we're actually dispatching the `PropertyChanged` notifications, we need to actually respond to them. This behavior was defined in `Profiles::OnNavigatedTo()` in the `PropertyChanged()` handler. Funny enough, that code was still there, it just didn't do anything because it was trying to notify that `Profiles::CurrentXXX` changed. This is invalid because `CurrentXXX` got moved to `ProfileViewModel`.
   - The fix here was pretty easy. Just move the property changed handler to `ProfileViewModel`'s `PropertyChanged` handler that is defined in the ctor.

## References
Bug introduced in #11877

## Validation Steps Performed
✅ Profile termination behavior
✅ Bell notification style
✅ Text antialiasing
✅ Scrollbar visibility
zadjii-msft pushed a commit that referenced this pull request Mar 3, 2022
Cleans up `ProfileViewModel`, `Profiles`, and `ProfilePageNavigationState` to move all of the view model responsibilities over to `ProfileViewModel`. We don't actually store the `ProfilePageNavigationState` anymore. We only use it as a way to transfer information to the new page.

- I pulled out `ProfileViewModel` into its own file to keep things cleaner. It was getting pretty big.
- The font lists are now stored in a static location in `ProfileViewModel`, which means that we can reuse the same list between pages.
- the profile pivot was also moved to the `ProfileViewModel` and stored as a static value.

✅ pivot behavior is the same
✅ font list is still populated
zadjii-msft pushed a commit that referenced this pull request Mar 3, 2022
This fixes a bug where several settings would not show the reset button. The root cause of this issue is two fold:
1. Hooking up `CurrentXXX`
   - `GETSET_BINDABLE_ENUM_SETTING` was hooked up to the **settings** model profile object instead of the **view** model profile object. Since the settings model has no `PropertyChanged` system, any changes were directly being applied to the setting, but not notifying the view model (and thus, the view, by extension) to update themselves.
   - This fix required me to slightly modify the macro. Rather than using two parameters (object and function name), I used one parameter (path to getter/setter).
2. Responding to the `PropertyChanged` notifications
   - Now that we're actually dispatching the `PropertyChanged` notifications, we need to actually respond to them. This behavior was defined in `Profiles::OnNavigatedTo()` in the `PropertyChanged()` handler. Funny enough, that code was still there, it just didn't do anything because it was trying to notify that `Profiles::CurrentXXX` changed. This is invalid because `CurrentXXX` got moved to `ProfileViewModel`.
   - The fix here was pretty easy. Just move the property changed handler to `ProfileViewModel`'s `PropertyChanged` handler that is defined in the ctor.

Bug introduced in #11877

✅ Profile termination behavior
✅ Bell notification style
✅ Text antialiasing
✅ Scrollbar visibility
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants