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.

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

Try to flow together JSON serialization for keybindings #1378

Closed
Jaykul opened this issue Jun 22, 2019 · 3 comments · Fixed by #2515
Closed

Try to flow together JSON serialization for keybindings #1378

Jaykul opened this issue Jun 22, 2019 · 3 comments · Fixed by #2515
Assignees
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. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@Jaykul
Copy link
Contributor

Jaykul commented Jun 22, 2019

I would love it if we could see all the keybindings ...

Wouldn't it be better if the default profiles.json could be formatted like ...

        "keybindings": [
            { "command": "closeTab",       "keys": [ "ctrl+w" ] },
            { "command": "newTab",         "keys": [ "ctrl+t" ] },
            { "command": "newTabProfile0", "keys": [ "ctrl+shift+1" ] },
            { "command": "newTabProfile1", "keys": [ "ctrl+shift+2" ] },
            { "command": "newTabProfile2", "keys": [ "ctrl+shift+3" ] },
            { "command": "newTabProfile3", "keys": [ "ctrl+shift+4" ] },
            { "command": "newTabProfile4", "keys": [ "ctrl+shift+5" ] },
            { "command": "newTabProfile5", "keys": [ "ctrl+shift+6" ] },
            { "command": "newTabProfile6", "keys": [ "ctrl+shift+7" ] },
            { "command": "newTabProfile7", "keys": [ "ctrl+shift+8" ] },
            { "command": "newTabProfile8", "keys": [ "ctrl+shift+9" ] },
            { "command": "nextTab",        "keys": [ "ctrl+tab" ] },
            { "command": "prevTab",        "keys": [ "ctrl+shift+tab" ] },
            { "command": "openSettings",   "keys": [ "ctrl+," ] },
            { "command": "scrollDown",     "keys": [ "ctrl+shift+down" ] },
            { "command": "scrollDownPage", "keys": [ "ctrl+shift+pgdn" ] },
            { "command": "scrollUp",       "keys": [ "ctrl+shift+up" ] },
            { "command": "scrollUpPage",   "keys": [ "ctrl+shift+pgup" ] },
            { "command": "switchToTab0",   "keys": [ "ctrl+1" ] },
            { "command": "switchToTab1",   "keys": [ "ctrl+2" ] },
            { "command": "switchToTab2",   "keys": [ "ctrl+3" ] },
            { "command": "switchToTab3",   "keys": [ "ctrl+4" ] },
            { "command": "switchToTab4",   "keys": [ "ctrl+5" ] },
            { "command": "switchToTab5",   "keys": [ "ctrl+6" ] },
            { "command": "switchToTab6",   "keys": [ "ctrl+7" ] },
            { "command": "switchToTab7",   "keys": [ "ctrl+8" ] },
            { "command": "switchToTab8",   "keys": [ "ctrl+9" ] }
        ],

And I just mean getting rid of the terribly redundant white space so that all the keys fit on screen at the same time ... not the fact that these key defaults are clearly superior (who's idea was it to use ALT for switching tabs, anyway?).

@Jaykul Jaykul added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 22, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 22, 2019
@fedfigca
Copy link

Yes, please.
If the configuration is going to be via text file, which I'm perfectly fine with, there should be some good information architecture in the layout and order of the items.
IMHO, alphabetic order is not the best way to sort the items here, because it hurts discoverability of all the features and related items, since a human is reading the file, fields like name or guid should come first, and then grouped by what the features modify, layout, colors, fonts, cursors, etc so that the user can find the group it's interested in and explore only those options instead of having to read through some 15 items and figure out what each one is for.

Thanks for the great work, I'm really happy to see it come to reality, it's going to make our developer lives even better.

@DHowett-MSFT DHowett-MSFT changed the title Better formatting for readability Try to flow together JSON serialization for keybindings Jun 24, 2019
@DHowett-MSFT DHowett-MSFT added Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 24, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 24, 2019
@zadjii-msft
Copy link
Member

@fedfigca Hopefully this won't be as much of a problem once #754 is complete. The "defaults" file will be in a sane, non-alphabetic order, similar to how they were before #1005. Then, the user settings will be in whatever order the user puts them in.

@zadjii-msft zadjii-msft self-assigned this Aug 22, 2019
@zadjii-msft zadjii-msft removed the Help Wanted We encourage anyone to jump in on these. label Aug 22, 2019
@zadjii-msft zadjii-msft added this to the Terminal 1909 milestone Aug 22, 2019
@ghost ghost added the In-PR This issue has a related PR label Aug 23, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 16, 2019
zadjii-msft added a commit that referenced this issue Sep 16, 2019
This PR represents the start of the work on Cascading User + default settings, #754.

Cascading settings will be done in two parts: 
* [ ] Layered Default+User settings (this PR)
* [ ] Dynamic Profile Generation (#2603).

Until _both_ are done, _neither are going in. The dynamic profiles PR will target this PR when it's ready, but will go in as a separate commit into master.

This PR covers adding one primary feature: the settings are now in two separate files:
* a static `defaults.json` that ships with the package (the "default settings")
* a `profiles.json` with the user's customizations (the "user settings)

User settings are _layered_ upon the settings in the defaults settings.

## References

Other things that might be related here:
* #1378 - This seems like it's definitely fixed. The default keybindings are _much_ cleaner, and without the save-on-load behavior, the user's keybindings will be left in a good state 
* #1398 - This might have honestly been solved by #2475 

## PR Checklist
* [x] Closes #754
* [x] Closes #1378 
* [x] Closes #2566
* [x] I work here
* [x] Tests added/passed
* [x] Requires documentation to be updated - it **ABSOLUTELY DOES**


## Detailed Description of the Pull Request / Additional comments

1. We start by taking all of the `FromJson` functions in Profile, ColorScheme, Globals, etc, and converting them to `LayerJson` methods. These are effectively the same, with the change that instead of building a new object, they are simply layering the values on top of `this` object. 
2. Next, we add tests for layering properties like that.
3. Now, we add a `defaults.json` to the package. This is the file the users can refer to as our default settings.
4. We then take that `defaults.json` and stamp it into an auto generated `.h` file, so we can use it's data without having to worry about reading it from disk.
5. We then change the `LoadAll` function in `CascadiaSettings`. Now, the function does two loads - one from the defaults, and then a second load from the `profiles.json` file, layering the settings from each source upon the previous values.
6. If the `profiles.json` file doesn't exist, we'll create it from a hardcoded `userDefaults.json`, which is stamped in similar to how `defaults.json` is.
7. We also add support for _unbinding_ keybindings that might exist in the `defaults.json`, but the user doesn't want to be bound to anything.
8. We add support for _hiding_ a profile, which is useful if a user doesn't want one of the default profiles to appear in the list of profiles.

## TODO:
* [x] Still need to make Alt+Click work on the settings button
* [x] Need to write some user documentation on how the new settings model works
* [x] Fix the pair of tests I broke (re: Duplicate profiles)


<hr>

* Create profiles by layering them

* Update test to layer multiple times on the same profile

* Add support for layering an array of profiles, but break a couple tests

* Add a defaults.json to the package

* Layer colorschemes

  * Moves tests into individual classes
  * adds support for layering a colorscheme on top of another

* Layer an array of color schemes

* oh no, this was missed with #2481

  must have committed without staging this change, uh oh. Not like those tests actually work so nbd

* Layer keybindings

* Read settings from defaults.json + profiles.json, layer appropriately

  This is like 80% of #754. Needs tests.

* Add tests for keybindings

  * add support to unbind a key with `null` or `"unbound"` or `"garbage"`

* Layer or clear optional properties

* Add a helper to get an optional variable for a bunch of different types

  In the end, I think we need to ask _was this worth it_

* Do this with the stretch mode too

* Add back in the GUID check for profiles

* Add some tests for global settings layering

* M A D  W I T H  P O W E R

  Add a MsBuild target to auto-generate a header with the defaults.json as a
  string in the file. That way, we can _always_ load the defaults. Literally impossible to not.

* When the user's profile.json doesn't exist, create it from a template

* Re-order profiles to match the order set in the user's profiles.json

* Add tests for re-ordering profiles to match user ordering

* Add support for hiding profiles using `"hidden": true`

* Use the hardcoded defaults.json for the exception->"use defaults" case

* Somehow I messed up the git submodules?

* woo documentation

* Fix a Terminal.App.Unit.Tests failure

* signed/unsigned is hard

* Use Alt+Settings button to open the default settings

* Missed a signed/unsigned

* Some very preliminary PR feedback

* More PR feedback

  Use the wil helper for the exe path
  Move jsonutils into their own file
  kill some dead code

* Add templates to these bois

* remove some code for generating defaults, reorder defaults.json a tad

* Make guid a std::optional

* Large block of PR feedback

  * Remove some dead code
  * add some comments
  * tag some todos

* stl is love, stl is life

* add `-noprofile`

* Fix the crash that dustin found

* -Encoding ASCII

* Set a profile's default scheme to Campbell

* Fix the tests I regressed

* Update UsingJsonSetting.md to reflect that changes from these PRs

* Change how GenerateGuidForProfile works

* Make AppKeyBindings do its own serialization

* Remove leftover dead code from the previous commit

* Fix up an enormous number of PR nits

* Fix a typo; Update the defaults to match #2378

* Tiny nits

* Some typos, PR nits

* Fix this broken defaults case
@ghost
Copy link

ghost commented Sep 24, 2019

🎉This issue was addressed in #2515, which has now been successfully released as Windows Terminal Preview v0.5.2661.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. Needs-Tag-Fix Doesn't match tag requirements 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
Development

Successfully merging a pull request may close this issue.

4 participants