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

doc: Add a spec for Application State #7972

Merged
merged 4 commits into from
Dec 4, 2020
Merged

doc: Add a spec for Application State #7972

merged 4 commits into from
Dec 4, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Oct 20, 2020 •
edited
Loading

This draft pull request (Review link) introduces a specification for "cross-session" app state that isn't stored in the user's settings.json.

PR Checklist

  • Specs Feature: Application State #8324
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • 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

Review link

zadjii-msft
zadjii-msft approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yea this all seems reasonable to me.

doc/specs/#xxxx - Application State (TSM).md Outdated
Comment on lines 73 to 76
[comment]: # Will the proposed change improve reliability? If not, why make the change?

The comment in this section heavily suggests that we should only make changes that improve reliability, not ones that
maintain it.
Copy link
Member

Choose a reason for hiding this comment

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

<aside> Yea, this "Capabilities" section I've always hated. I think if you're outside MSFT, then you don't have any idea what to put here, and "capabilities" is a terrible heading for 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.

agree

doc/specs/#xxxx - Application State (TSM).md Outdated

This has no direct impact on the UI/UX; however, we may want to add a button to general settings page titled "reset all
dialogs" or "don't not ask me again about those things that, some time ago, I told you to not ask me about".

Copy link
Member

Choose a reason for hiding this comment

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

I'd maybe add a note that we're not intending for the user to hand-edit state.json, so we're not as concerned about how the contents of that file are serialized. This is unlike settings.json, which has to have a semblance of user friendliness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. Thanks!

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Oct 21, 2020
@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:26
@zadjii-msft zadjii-msft mentioned this pull request Oct 25, 2020
Closed
@zadjii-msft zadjii-msft mentioned this pull request Nov 17, 2020
Closed
@DHowett DHowett marked this pull request as ready for review November 18, 2020 21:49
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Nov 19, 2020
carlos-zamora
carlos-zamora approved these changes Nov 21, 2020
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.

More curious about the applications than the overall design, itself. But it's fair to say that that's out of scope so meh.

@DHowett
Copy link
Member Author

DHowett commented Nov 21, 2020

@carlos-zamora clarify? A whole section was dedicated to why (the applications). Any questions I can answer?

@carlos-zamora
Copy link
Member

@carlos-zamora clarify? A whole section was dedicated to why (the applications). Any questions I can answer?

I don't mean it as a bad thing haha. I mean more that I'm curious what the design for "don't ask me again" or "keep the profile deleted" would look like. But I'm guessing the specific design for those settings is a bit out of scope here (if they even need a design, frankly).

Like, I'm curious if we should move disabledProfileSources over or how that would interact with the plan for "keep the dynamic profile deleted"? But that's probably out of scope.

@DHowett
Copy link
Member Author

DHowett commented Nov 21, 2020

So, disabledSources is twofold:

  1. It means “do not run the expensive profile search code” (like the VS Generator!)
  2. When extensions exist, they could be suppressed.

Disabling the source is a bigger hammer than deleting one profile generated by the source.

@DHowett
Copy link
Member Author

DHowett commented Nov 21, 2020

Reasonable question though!

miniksa
miniksa reviewed Dec 3, 2020
View reviewed changes
doc/specs/#8324 - Application State (TSM).md

## UI/UX Design

This has no direct impact on the UI/UX; however, we may want to add a button to general settings page titled "reset all
Copy link
Member

Choose a reason for hiding this comment

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

I think this is pretty important and we should ensure there's an easy reset.

miniksa
miniksa approved these changes Dec 3, 2020
View reviewed changes
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.

Looks fine to me.

@DHowett DHowett changed the title Add a spec for Application State doc: Add a spec for Application State Dec 4, 2020
@DHowett DHowett merged commit 956a045 into main Dec 4, 2020
@DHowett DHowett deleted the dev/duhowett/spec/appstate branch December 4, 2020 01:52
@DHowett DHowett mentioned this pull request Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa miniksa approved these changes

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

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DHowett @carlos-zamora @miniksa @zadjii-msft