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

Transform TerminalApp settings objects into WinRT objects #7141

Closed
carlos-zamora opened this issue Jul 31, 2020 · 1 comment · Fixed by #7457
Closed

Transform TerminalApp settings objects into WinRT objects #7141

carlos-zamora opened this issue Jul 31, 2020 · 1 comment · Fixed by #7457
Assignees
@carlos-zamora
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.4

Comments

@carlos-zamora
Copy link
Member

Description of the new feature/enhancement

  • Profile
  • ColorScheme
  • GlobalSettings
  • Keybindings (this may be moved out to be its own PR)

This will include the JSON (de)serializers and layering logic.

See #885 and #6904 for more details.
@carlos-zamora carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Jul 31, 2020
@carlos-zamora carlos-zamora added this to the Terminal v1.3 milestone Jul 31, 2020
@carlos-zamora carlos-zamora self-assigned this Jul 31, 2020
@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 Jul 31, 2020
@DHowett-MSFT DHowett-MSFT mentioned this issue Jul 31, 2020
Closed
4 tasks
@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 Jul 31, 2020
This was referenced Aug 10, 2020
Merged
Merged
DHowett pushed a commit that referenced this issue Aug 15, 2020
@carlos-zamora
Make ColorScheme a WinRT object (#7238)
e9a7053 
ColorScheme is now a WinRT object.

All of the JSON stuff can't be exposed via the idl. So the plan here is
that we'll have the TerminalSettingsModel project handle all of the
serialization when it's moved over. These functions will be exposed off
of the `implementation` namespace, not projected namespace.

References #7141 - ColorScheme is a settings object
References #885 - this new settings object will be moved to a new
TerminalSettingsModel project

## Validation Steps Performed
- [x] Tests passed
- [x] Deployment succeeded
@carlos-zamora carlos-zamora mentioned this issue Aug 19, 2020
Merged
3 tasks
This was referenced Aug 20, 2020
Closed
Closed
ghost pushed a commit that referenced this issue Aug 28, 2020
@carlos-zamora
Make Profile a WinRT object (#7283)
a51091c 
Profile is now a WinRT object in the TerminalApp project.

As with ColorScheme, all of the serialization logic is not exposed via
the idl. TerminalSetingsModel will handle it when it's all moved over.

I removed the "Get" and "Set" prefixes from all of the Profile
functions. It just makes more sense to use the `GETSET_PROPERTY` macro
to do most of the work for us.

`CloseOnExitMode` is now an enum off of the Profile.idl.

`std::optional<wstring>` got converted to `hstring` (as opposed to
`IReference<hstring>`). `IReference<hstring>` is not valid to MIDL.

## References
#7141 - Profile is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project

## Validation Steps Performed
- [x] Tests passed
- [x] Deployment succeeded

Closes #7435
ghost pushed a commit that referenced this issue Aug 28, 2020
@carlos-zamora
Make GlobalAppSettings a WinRT object (#7349)
7803efa 
GlobalAppSettings is now a WinRT object in the TerminalApp project.

## References
#7141 - GlobalAppSettings is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project

## PR Checklist
* [x] Tests passed

## Detailed Description of the Pull Request / Additional comments
This one was probably the easiest thus far.

The only weird thing is how we handle InitialPosition. Today, we lose a
little bit of fidelity when we convert from LaunchPosition (int) -->
Point (float) --> RECT (long). The current change converts
LaunchPosition (optional<long>) --> InitialPosition (long) --> RECT
(long).

NOTE: Though I could use LaunchPosition to go directly from TermApp to
AppHost, I decided to introduce InitialPosition because LaunchPosition
will be a part of TerminalSettingsModel soon.

## Validation Steps Performed
- [x] Tests passed
- [x] Deployment succeeded
@carlos-zamora carlos-zamora mentioned this issue Aug 28, 2020
Merged
2 tasks
@ghost ghost added the In-PR This issue has a related PR label Aug 28, 2020
@carlos-zamora carlos-zamora mentioned this issue Sep 9, 2020
Merged
2 tasks
@ghost ghost closed this as completed in #7457 Sep 9, 2020
@ghost ghost removed the In-PR This issue has a related PR label Sep 9, 2020
ghost pushed a commit that referenced this issue Sep 9, 2020
@carlos-zamora
Make CascadiaSettings a WinRT object (#7457)
c5cf7b8 
CascadiaSettings is now a WinRT object in the TerminalApp project.

## References
#7141 - CascadiaSettings is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project

This one _looks_ big, but most of it is really just propagating the
changes to the tests. In fact, you can probably save yourself some time
because the tests were about an hour of Find&Replace.

`CascadiaSettings::GetCurrentAppSettings()` was only being used in
Pane.cpp. So I ripped out the 3 lines of code and stuffed them in there.

Follow-up work:
- There's a few places in AppLogic where I `get_self` to be able to get
  the warnings out. This will go away in the next PR (wrapping up #885)

## Validation Steps Performed
- [x] Tests passed
- [X] Deployment succeeded

Closes #7141
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Sep 9, 2020
DHowett pushed a commit that referenced this issue Sep 11, 2020
@carlos-zamora
Add serialization error handling to settings projection layer (#7576)
892cf05 
Now that CascadiaSettings is a WinRT object, we need to update the error
handling a bit. Making it a WinRT object limits our errors to be
hresults. So we moved all the error handling down a layer to when we
load the settings object.

- Warnings encountered during validation are saved to `Warnings()`.
- Errors encountered during validation are saved to `GetLoadingError()`.
- Deserialization errors (mainly from JsonUtils) are saved to
  `GetDeserializationErrorMessage()`.

## References
#7141 - CascadiaSettings is a settings object
#885 - this makes ripping out CascadiaSettings into
     TerminalSettingsModel much easier

## Validation Steps Performed
* [x] Tests passed
- [x] Deployment succeeded
   - tested with invalid JSON (deserialization error)
   - tested with missing DefaultProfile (validation error)
@ghost
Copy link

ghost commented Sep 22, 2020

🎉This issue was addressed in #7457, which has now been successfully released as Windows Terminal Preview v1.4.2652.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.4
Development

Successfully merging a pull request may close this issue.

Make CascadiaSettings a WinRT object microsoft/terminal
3 participants
@DHowett @carlos-zamora @cinnamon-msft