-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Settings UI reformats settings.json in an hard to read way #8991
Comments
Sure, I'll add this to the SUI backlog. This one is Hard, because we have to determine which settings changed and only overwrite those specific settings, exactly where they were in the file. That's not impossible, but it's Hard, especially with settings layering. |
Yeah I'm wondering if the inheritance might render this issue pointless. If every setting able to be configured in the json is configurable in the UI (as well as any other future settings), then that should prevent the need to ever go into the json and it simply becomes an implementation detail. |
Ideally, yes. When the SUI is totally done, then theoretically, you shouldn't need to go into json anymore. So it'll become more of a "I'm only using JSON" / "I'm only using the SUI" split. |
I am in annoying mode today (@zadjii-msft started first) - why is it in a backlog then? Sounds like something hard to implement and not aligned with a long-term vision (I am stressed that we moved from 1.1K to 1.2K open items) 😊 |
I'll throw my hat in the ring: as a mostly JSON user who might want to toggle a setting here and there, I feel like high fidelity is "worth it". |
Exactly what Dustin said. Someday, we might do it. I'd love to do it, but there are a ton of things that are higher priority right now. |
Until then the setting files might become so large that we will need to split into several files, solving this inherently. But I see your point! |
And I am reminded that we will soon have extensions and manipulation of the settings file is part of the support coming if I recall correctly. So it's entirely possible an extension author will solve this issue first, possibly not even intentionally and that would provide a good "workaround" until this could be done natively in Terminal itself. |
Note: reveals IEEE954 disparity in float precision |
I came here from #10889 which is regarding deletion of comments. I was wondering how VS Code does this? Their settings file doesn't overwrite the comments or anything. |
I'm heavily editor-based and I write my settings in the format and order that makes sense to me. It's annoying to have Terminal overwrite the effort I put into that. Just like it was mentioned in the comment above, VSCode doesn't overwrite anything... My manual writing and formatting of actions below feels much more organized and easier to look at to me than the arbitrary overwritten version of it, or the larger list of actions I don't care about in the settings UI: // My version
"actions":
[
// ...
// Press Ctrl+Shift+F to open the search box
{ "command": "find", "keys": "ctrl+f" },
{ "command": { "action": "switchToTab", "index": 0}, "keys": "ctrl+1" },
{ "command": { "action": "switchToTab", "index": 1}, "keys": "ctrl+2" },
{ "command": { "action": "switchToTab", "index": 2}, "keys": "ctrl+3" },
{ "command": { "action": "switchToTab", "index": 3}, "keys": "ctrl+4" },
{ "command": { "action": "switchToTab", "index": 4}, "keys": "ctrl+5" },
{ "command": "newWindow", "keys": "ctrl+n" },
{ "command": "newTab", "keys": "ctrl+t" },
{ "command": "closeTab", "keys": "ctrl+w" }
] // Overwritten version
"actions":
[
// ...
{
"command":
{
"action": "switchToTab",
"index": 2
},
"keys": "ctrl+3"
},
{
"command": "find",
"keys": "ctrl+f"
},
{
"command":
{
"action": "switchToTab",
"index": 4
},
"keys": "ctrl+5"
},
{
"command":
{
"action": "switchToTab",
"index": 0
},
"keys": "ctrl+1"
},
{
"command":
{
"action": "switchToTab",
"index": 1
},
"keys": "ctrl+2"
},
{
"command":
{
"action": "switchToTab",
"index": 3
},
"keys": "ctrl+4"
},
{
"command":
{
"action": "newWindow"
},
"keys": "ctrl+n"
},
{
"command":
{
"action": "newTab"
},
"keys": "ctrl+t"
},
{
"command":
{
"action": "closeTab"
},
"keys": "ctrl+w"
}
] |
How VSCode does it is completely irrelevant in this case because VS Code is an Electron-based application and so does things differently. The Settings UI is almost it's own app, completely capable of being built on it's own. There is data manipulation being done at the native windows level for WT and the Settings UI, so something likely within one of the user APIs or with the implementation of Settings UI itself needs to be refactored to restore the precision. For myself, if there's a new setting I want added that isn't easy to find in documentation, I'll make a backup of the settings.json, use the Settings UI to toggle the new setting on, copy the setting entry over to the backup and then replace the settings.json generated by the Settings UI with the backup. A bit clunky but it works. |
I think that what's missing is an intentional decision to take the settings file more seriously as its own thing, similar to what VSCode did. IMO, there is a valid reason for doing that for VSCode since developers are extremely likely to be very aware of their setup (unlike people who would be fine with switching some random gui knob), and terminal targets a very similar crowd. (I raised that explicitly in #18110, which was marked as a dup of this one.) Also, I disagree with VSCode being irrelevant: the missing functionality which could be made into an independent library is basically taking a JSON config file (not a value!), and an edit operation, and applying it into the existing file. It doesn't depend on the application that uses the file. (It could probably even be independent of the settings format (ie, convert json + comments into yaml + comments) with some more work.) In case it helps anyone, after the nth time that Terminal mangled my settings, I finally sat down to avoid such disasters in the future:
(All of this probably sounds like a whole bunch of useless work for most people, but for people who found themselves rewriting stuff as in the above comment and repeating it periodically, this is a very nice way to address it.) |
FWIW, I don't disagree with you. Older versions of Terminal treated the settings file with the care of a UI, because it's all they had. It's just that our JSON serializer and deserializer is a pain, and our "user layout preserving" JSON patch implementation was a significant burden1, and we haven't booked the engineering effort to make either of those things better. I did an experiment with comment/content preservation, but I do not appear to have pushed that branch. Footnotes
|
FWIW, I'm also a heavy JSON-specific user. I've kinda taken to this hack where I've got most of my Then I just roam that file around to different machines. Terminal will load and parse actions (but not keybindings) from that file. But notably, Terminal won't modify that file either. So I can just add the key-> It's mental, but I'm working on like 3 machines and 4 different builds of the terminal all at once, so this setup lets me stay relatively sync'd up in all of them. I'd love to have Terminal not reformat the settings if at all possible. Alas, it's just expensive from an engineering standpoint to do that. If someone wants to chip in here and figure that out, I'd happily review that PR. |
Environment
Steps to reproduce
Before ever opening the Settings UI, open the settings.json. Make a copy of it for comparison and keep it open in an editor such as VS Code. Close the active settings.json. Now open the Settings UI. Then open json from either the Settings dropdown or from the Settings UI. Notice how the file is completely reformatted in a user unfriendly way.
Expected behavior
I expect the settings.json to remain in the same readable format it's always been.
Actual behavior
The settings.json is now harder to read and unorganized compared to the normal format.
If the Settings UI is never opened, the original format remains.
The text was updated successfully, but these errors were encountered: