Conversation
lhecker
left a comment
There was a problem hiding this comment.
Does this still cache the font list when you navigate away and then back to a profile / between profiles?
miniksa
left a comment
There was a problem hiding this comment.
That wasn't as big a deal as I thought it was going to be when you initially proposed it. Nice.
This comment has been minimized.
This comment has been minimized.
This should cache the list, which is a nice bonus. |
This comment has been minimized.
This comment has been minimized.
|
Hello @carlos-zamora! Because this pull request has the 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 (
|
| { | ||
| _LastActivePivot = lastState.LastActivePivot(); | ||
| } | ||
| auto profile{ winrt::get_self<ProfileViewModel>(viewModel) }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's fair. Adding this as a comment under the main MVVM issue.
## 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
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
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
Summary of the Pull Request
Cleans up
ProfileViewModel,Profiles, andProfilePageNavigationStateto move all of the view model responsibilities over toProfileViewModel. We don't actually store theProfilePageNavigationStateanymore. 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
ProfileViewModelinto its own file to keep things cleaner. It was getting pretty big.ProfileViewModel, which means that we can reuse the same list between pages.ProfileViewModeland stored as a static value.Validation Steps Performed
✅ pivot behavior is the same
✅ font list is still populated