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

Implement MVVM for Color Schemes #13179

Merged
40 commits merged into from
Jul 29, 2022
Merged

Implement MVVM for Color Schemes #13179

40 commits merged into from
Jul 29, 2022

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented May 25, 2022 •
edited by carlos-zamora
Loading

Summary of the Pull Request

Implements the MVVM style for the Color Schemes editor

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • I work here

Detailed Description of the Pull Request / Additional comments

Introduces:

  • ColorSchemesPageViewModel: The view model responsible for the entire color schemes page. Handles what the current scheme is, adding/deleting/renaming schemes
  • ColorSchemeViewModel: A view model class for individual color schemes

Validation Steps Performed

Manually tested:

  • Edit a color scheme
  • Add new color scheme
  • Rename a color scheme
  • Delete a color scheme

@PankajBhojwani PankajBhojwani marked this pull request as ready for review May 27, 2022 18:25
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented May 27, 2022 •
edited
Loading

Known issue:

On the Color Schemes page in the SUI, reloading the settings (e.g. by hitting 'Save') always resets the currently selected color scheme in the combo box to Campbell. Did some investigation into this and the findings were really weird: we definitely set the correct scheme on the combo box in OnNavigatedTo, but sometime later we end up in ColorSchemeSelectionChanged with Campbell as the new scheme (I think this has something to do with xaml initializing the combo box). In main, the same thing happens, but we end up in ColorSchemeSelectionChanged twice, the first time with Campbell and the second time with the correct scheme and so it seems to work fine there.

EDIT: Fixed now!

carlos-zamora
carlos-zamora reviewed May 27, 2022
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Ok. I think this is a good split for MVVM. I'm trying my best to figure out what is reasonably in scope for this PR and what isn't. Here's what I've kinda decided on...

  • in scope:
    • move IsRenaming
    • fix up & polish notification system
    • fix some of the names on these classes/members. Curious what other reviewers suggest for names because some of these are hard haha.
  • follow-up:
    • changing CanDelete and CanRename functionality to operate without having to check a list of InBoxSchemes names. This should just be a property off of the ColorSchemeViewModel.

src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.idl Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.idl Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemes.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemeViewModel.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.cpp Show resolved Hide resolved
@ghost ghost merged commit df67137 into main Jul 29, 2022
@ghost ghost deleted the dev/pabhoj/sui_follow_ups branch July 29, 2022 17:11
@PankajBhojwani PankajBhojwani mentioned this pull request Aug 1, 2022
Merged
2 tasks
ghost pushed a commit that referenced this pull request Aug 2, 2022
@PankajBhojwani
Fix a couple of issues with color scheme MVVM (#13646)
7976e48 
## Summary of the Pull Request
- When 'discard changes' is hit, we re-initialize our list of color scheme view models but forgot to tell xaml about it, this commit fixes that. 
- Make sure to exit rename mode when 'update settings' gets called

## References
color schemes mvvm added in #13179 

## PR Checklist
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
Hitting discard changes doesn't cause an inconsistency with the currently selected scheme anymore
DHowett added a commit that referenced this pull request Aug 9, 2022
@DHowett
Propagate the color scheme resource changes from df67137
2ef6af9 
Due to the way our localization pipeline works, we cannot delete
resources in main until the resources in question are totally flushed
out of the active-servicing release branches. Unfortunately, in #13179,
we _did_ remove resource keys. Because the Color Schemes page in 1.14
and 1.15 uses the resources directly (rather than by way of x:Uid), it
is easier for us to backport the resource changes now than to
reintroduce the old keys on main.

Closes #13606
@DHowett DHowett mentioned this pull request Aug 9, 2022
Merged
DHowett added a commit that referenced this pull request Aug 9, 2022
@DHowett
Propagate the color scheme resource changes from df67137
19665a7 
Due to the way our localization pipeline works, we cannot delete
resources in main until the resources in question are totally flushed
out of the active-servicing release branches. Unfortunately, in #13179,
we _did_ remove resource keys. Because the Color Schemes page in 1.14
and 1.15 uses the resources directly (rather than by way of x:Uid), it
is easier for us to backport the resource changes now than to
reintroduce the old keys on main.

Closes #13606
@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

@carlos-zamora carlos-zamora mentioned this pull request Oct 6, 2022
Closed
3 tasks
PKRoma pushed a commit to PKRoma/Terminal that referenced this pull request Oct 15, 2022
@DHowett
[1.14/5] Propagate the color scheme resw changes from df67137 (micros…
6933429 
…oft#13712)

Due to the way our localization pipeline works, we cannot delete
resources in main until the resources in question are totally flushed
out of the active-servicing release branches. Unfortunately, in microsoft#13179,
we _did_ remove resource keys. Because the Color Schemes page in 1.14
and 1.15 uses the resources directly (rather than by way of x:Uid), it
is easier for us to backport the resource changes now than to
reintroduce the old keys on main.

Closes microsoft#13606
PKRoma pushed a commit to PKRoma/Terminal that referenced this pull request Oct 15, 2022
@DHowett
[1.14/5] Propagate the color scheme resw changes from df67137 (micros…
13236a2 
…oft#13712)

Due to the way our localization pipeline works, we cannot delete
resources in main until the resources in question are totally flushed
out of the active-servicing release branches. Unfortunately, in microsoft#13179,
we _did_ remove resource keys. Because the Color Schemes page in 1.14
and 1.15 uses the resources directly (rather than by way of x:Uid), it
is easier for us to backport the resource changes now than to
reintroduce the old keys on main.

Closes microsoft#13606

(cherry picked from commit 6933429)
Service-Card-Id: 84870299
Service-Version: 1.14
@PankajBhojwani PankajBhojwani mentioned this pull request Dec 16, 2022
Merged
2 tasks
@PankajBhojwani PankajBhojwani mentioned this pull request Jan 4, 2023
Merged
DHowett pushed a commit that referenced this pull request Jan 18, 2023
@PankajBhojwani
Update Profile and Appearance VMs to use CSVM; remove nav state (#14572)
5614116 
ColorScheme MVVM was implemented in #13179. This PR updates the
ProfileViewModel/AppearanceViewModels to use color scheme view models
instead of the raw settings model objects.

* [x] Updates ProfileViewModel/AppearanceViewModel to use
  ColorSchemesPageViewModel/ColorSchemeViewModel implemented in #13179
* [x] Removes ProfilePageNavigationState

## Validation Steps Performed
Settings UI still works (we _probably_ want to bug bash this at some
point though as with all SUI changes)
ghost pushed a commit that referenced this pull request Jan 19, 2023
@PankajBhojwani
Automatically focus the ColorSchemeListView when we navigate to the C…
96a9dd5 
…olorSchemes page (#14631)

This regressed when we implemented ColorSchemeViewModel in #13179.
This commit fixes that by automatically focusing the ColorSchemeListView
when we navigate to the ColorSchemes page, which makes sense from a
user experience perspective anyway.

Closes #11971
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhecker lhecker lhecker approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@DHowett DHowett

Labels
Area-SettingsUI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Terminal v1.16
Development

Successfully merging this pull request may close these issues.
5 participants
@PankajBhojwani @carlos-zamora @zadjii-msft @DHowett @lhecker