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

Settings editor saves settings.json lines with trailing whitespace #10887

Open
mrjoel opened this issue Aug 6, 2021 · 6 comments
Open

Settings editor saves settings.json lines with trailing whitespace #10887

mrjoel opened this issue Aug 6, 2021 · 6 comments
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@mrjoel
Copy link

mrjoel commented Aug 6, 2021

Windows Terminal version (or Windows build number)

1.9.1942.0

Other Software

No response

Steps to reproduce

I have my settings.json tracked in a dotfiles git repo. Recently (now that settings UI is available) I've noticed that the settings.json is save with a trailing space after a name. This appears to be the case when the name is an array or object whose contents continue onto the next line (i.e. the space after the name is always added without considering whether a string value or array/object value will follow.

Expected Behavior

Lines are trimmed when saving

Actual Behavior

Extra spaces are left behind for entries which span multiple lines.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 6, 2021
@zadjii-msft
Copy link
Member

Can you give a specific example? I think I'm having a hard time trying to mentally picture what's wrong here.

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 10, 2021
@mrjoel
Copy link
Author

mrjoel commented Aug 11, 2021

Sure, the specific cases where I see this is happening on my settings.json are listed below, where an extra trailing whitespace character is added.

  • .actions[].command
  • .profiles
  • .profiles.defaults
  • .profiles.list
  • .profiles.schemes

For all of these cases the space is added after the name and colon, but before the value is added, which happens to be on the next line. Instead, I'd expect either to trim each string, or better to use the context to only add the space separator if the next character to be added is not a newline. I haven't taken a look at the code to see how it's handled, but it's curious that .actions is not written with the trailing space, perhaps it gets special handling?

For the case of .actions[].command, what is actually emitted is:

{
    "$schema": "https://aka.ms/terminal-profiles-schema",
    "actions":
    [
        {
            "command": "find", //mrjoel: note that string values on single line are fine, but object values later are not
            "keys": "ctrl+shift+f"
        },
        {
            "command":␣ // mrjoel: '␣' visually represents literal space character here
            {
                "action": "splitPane",
                "split": "auto",
                "splitMode": "duplicate"
            },
            "keys": "alt+shift+d"
        }
    ],

What is expected is below, note the lack of trailing space (fake depicted as '␣' to be easier to visualize in the diff).

-            "command":␣
+            "command":

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 11, 2021
@zadjii-msft zadjii-msft added good first issue This is a fix that might be easier for someone to do as a first contribution 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. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Issue-Task It's a feature request, but it doesn't really need a major design. labels Feb 17, 2022
@zadjii-msft zadjii-msft added this to the 22H2 milestone Feb 17, 2022
@zadjii-msft zadjii-msft added Area-SettingsUI Anything specific to the SUI and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Attention The core contributors need to come back around and look at this ASAP. labels Feb 17, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 17, 2022
@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal and removed Area-SettingsUI Anything specific to the SUI labels Feb 17, 2022
@zadjii-msft
Copy link
Member

these docs for jsoncpp are relevant. IIRC the flag was enableYAMLCompatibility. I could have swore we had this, even some time after #2515

@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Dec 5, 2022
@zadjii-msft zadjii-msft moved this to Should be written in Terminal Walkthroughs Feb 6, 2023
@networkhermit
Copy link

these docs for jsoncpp are relevant. IIRC the flag was enableYAMLCompatibility. I could have swore we had this, even some time after #2515

@zadjii-msft Hello. Does it suggest that it's an upstream issue? This issue still exists in Windows Terminal Version: 1.19.10573.0. Renaming settings.json and let the WT recreate a default json file have trailing whitespaces too.

@zadjii-msft
Copy link
Member

I'm not sure! Someone would have to dig into the codebase and make sure we're setting that whenever we're saving the settings. If we are, then yea, I'd bet it's an upstream issue.

@fallenwood
Copy link
Member

these docs for jsoncpp are relevant. IIRC the flag was enableYAMLCompatibility. I could have swore we had this, even some time after #2515

@zadjii-msft Hello. Does it suggest that it's an upstream issue? This issue still exists in Windows Terminal Version: 1.19.10573.0. Renaming settings.json and let the WT recreate a default json file have trailing whitespaces too.

Looks like a known upstream issue at jsoncpp open-source-parsers/jsoncpp#1154

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 good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
Status: Should be written
Development

No branches or pull requests

4 participants