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.

Sign up for GitHub

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

Jump to bottom

Apply MVVM Pattern to Settings UI #9207

Closed
carlos-zamora opened this issue Feb 18, 2021 · 2 comments
Closed

Apply MVVM Pattern to Settings UI #9207

carlos-zamora opened this issue Feb 18, 2021 · 2 comments
Labels
Area-SettingsUI Anything specific to the SUI Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone
Engineering Impro...

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Feb 18, 2021 •
edited
Loading

The Settings UI is currently directly bound to the settings model. Unfortunately, this forces us to run into a few issues every now and then:

  • polluting the settings model with more XAML dependencies
  • inability to check if a value truly changed

The Model-View-ViewModel Design Pattern was designed to keep the settings model and the settings UI pure simultaneously.

We've already started some of this work by introducing the ProfileViewModel. It serves as a middle-man between the UI controls and the settings model, and can be used to track extra information like "is this the base layer" and special values for settings (i.e. background image's use desktop wallpaper, etc.).

We need to apply this design pattern to...

  • ColorSchemes
  • Global settings
  • CascadiaSettings

This allows for the following scenarios:

  • detect unsaved changes and display an appropriate warning label
  • only load font lists once, and share them across all ProfileViewModels and AppearanceViewModels
  • load the color schemes and be able to display a mini-grid of colors to display throughout the code (i.e. profile page)
  • load the key bindings upon initial SUI load asynchronously, then just pass that into the actions page for a huge performance boost
@carlos-zamora carlos-zamora added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Area-SettingsUI Anything specific to the SUI labels Feb 18, 2021
@carlos-zamora carlos-zamora added this to the Terminal v2.0 milestone Feb 18, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 18, 2021
@carlos-zamora carlos-zamora mentioned this issue Feb 18, 2021
Merged
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 18, 2021
This was referenced Feb 18, 2021
Merged
Merged
@carlos-zamora carlos-zamora mentioned this issue Mar 4, 2021
Merged
@carlos-zamora carlos-zamora mentioned this issue May 14, 2021
Merged
@carlos-zamora carlos-zamora mentioned this issue Jun 22, 2021
Merged
6 tasks
@carlos-zamora carlos-zamora mentioned this issue Aug 2, 2021
Merged
This was referenced Dec 1, 2021
Closed
Merged
ghost pushed a commit that referenced this issue Dec 8, 2021
@carlos-zamora
Apply MVVM for profiles in SUI (#11877)
0939eec 
## 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
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Dec 15, 2021 •
edited
Loading

I should do this: #11877 (comment)

(or really, anybody should haha. Doesn't have to be me :P)

@carlos-zamora carlos-zamora mentioned this issue Feb 8, 2022
Open
@zadjii-msft
Copy link
Member

There was a long series of PRs for all the global settings pages, with which we can finally close this out. I think that landed broadly in 1.15 ish.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-SettingsUI Anything specific to the SUI Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Engineering Improvements
Development

No branches or pull requests
3 participants
@DHowett @carlos-zamora @zadjii-msft