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

Investigate into 'launch.json' usability improvements #13353

Closed
isidorn opened this Issue Oct 7, 2016 · 40 comments

Comments

Projects
None yet
@isidorn
Contributor

isidorn commented Oct 7, 2016

We need to make the whole 'launch.json' experience more user friendly. Some ideas from @egamma:

  • Add comments with links to documentation in 'launch.json'
  • Indent advanced options so they get folded away

fyi @weinand

@isidorn isidorn self-assigned this Oct 7, 2016

@isidorn isidorn added this to the October 2016 milestone Oct 7, 2016

@isidorn isidorn changed the title from Debug - investigate into launch.json usability improvements @isi @andre to Investigate into 'launch.json' usability improvements Oct 7, 2016

@isidorn isidorn added plan-item and removed feature-request labels Oct 7, 2016

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 12, 2016

Contributor

After discussions with @weinand we have decided to introduce top level comments in the generated 'launch.json' which can point to some documentation (depending on the debug adapter). In the node case they point to https://go.microsoft.com/fwlink/?linkid=830387 . Due to that we need to imporve our documentation for 'launch.json', there is an issue which is tracking this Microsoft/vscode-docs#450

Instead of using smart indentation we have decided to remove all configurations which are not needed for the initial experience and which had the default value if not set anyways. Apart from that we also use a heuristic that we add the sourceMap and outDir properties only if a user has opened a typescript or a coffeescript file.

With all of this put together, here is how previously an initial 'launch.json' looked.

{
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Launch",
            "type": "node",
            "request": "launch",
            "program": "${workspaceRoot}/bin/www",
            "stopOnEntry": false,
            "args": [],
            "cwd": "${workspaceRoot}",
            "preLaunchTask": null,
            "runtimeExecutable": null,
            "runtimeArgs": [
                "--nolazy"
            ],
            "env": {
                "NODE_ENV": "development"
            },
            "console": "internalConsole",
            "sourceMaps": false,
            "outFiles": []
        },
        {
            "name": "Attach",
            "type": "node",
            "request": "attach",
            "port": 5858,
            "address": "localhost",
            "restart": false,
            "sourceMaps": false,
            "outFiles": [],
            "localRoot": "${workspaceRoot}",
            "remoteRoot": null
        },
        {
            "name": "Attach to Process",
            "type": "node",
            "request": "attach",
            "processId": "${command.PickProcess}",
            "port": 5858,
            "sourceMaps": false,
            "outFiles": []
        }
    ]
}

And here is the current structure. Much more clean and easier to use IMHO

{
    // See https://go.microsoft.com/fwlink/?linkid=830387
    // for setting up 'launch.json' for node debugging
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Launch",
            "type": "node",
            "request": "launch",

            "program": "${workspaceRoot}/bin/www",
            "args": [],
            "cwd": "${workspaceRoot}"
        },
        {
            "name": "Attach",
            "type": "node",
            "request": "attach",

            "port": 5858
            //"processId": "${command.PickProcess}"
        }
    ]
}

Feedback on the current strcture are very welcome.
In the next comment I will describe how other debug adapters can adopt and benefit from this feature.

fyi @weinand @egamma @chrisdias @gregvanl

Contributor

isidorn commented Oct 12, 2016

After discussions with @weinand we have decided to introduce top level comments in the generated 'launch.json' which can point to some documentation (depending on the debug adapter). In the node case they point to https://go.microsoft.com/fwlink/?linkid=830387 . Due to that we need to imporve our documentation for 'launch.json', there is an issue which is tracking this Microsoft/vscode-docs#450

Instead of using smart indentation we have decided to remove all configurations which are not needed for the initial experience and which had the default value if not set anyways. Apart from that we also use a heuristic that we add the sourceMap and outDir properties only if a user has opened a typescript or a coffeescript file.

With all of this put together, here is how previously an initial 'launch.json' looked.

{
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Launch",
            "type": "node",
            "request": "launch",
            "program": "${workspaceRoot}/bin/www",
            "stopOnEntry": false,
            "args": [],
            "cwd": "${workspaceRoot}",
            "preLaunchTask": null,
            "runtimeExecutable": null,
            "runtimeArgs": [
                "--nolazy"
            ],
            "env": {
                "NODE_ENV": "development"
            },
            "console": "internalConsole",
            "sourceMaps": false,
            "outFiles": []
        },
        {
            "name": "Attach",
            "type": "node",
            "request": "attach",
            "port": 5858,
            "address": "localhost",
            "restart": false,
            "sourceMaps": false,
            "outFiles": [],
            "localRoot": "${workspaceRoot}",
            "remoteRoot": null
        },
        {
            "name": "Attach to Process",
            "type": "node",
            "request": "attach",
            "processId": "${command.PickProcess}",
            "port": 5858,
            "sourceMaps": false,
            "outFiles": []
        }
    ]
}

And here is the current structure. Much more clean and easier to use IMHO

{
    // See https://go.microsoft.com/fwlink/?linkid=830387
    // for setting up 'launch.json' for node debugging
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Launch",
            "type": "node",
            "request": "launch",

            "program": "${workspaceRoot}/bin/www",
            "args": [],
            "cwd": "${workspaceRoot}"
        },
        {
            "name": "Attach",
            "type": "node",
            "request": "attach",

            "port": 5858
            //"processId": "${command.PickProcess}"
        }
    ]
}

Feedback on the current strcture are very welcome.
In the next comment I will describe how other debug adapters can adopt and benefit from this feature.

fyi @weinand @egamma @chrisdias @gregvanl

@isidorn isidorn closed this Oct 12, 2016

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 12, 2016

Contributor

In order to support this we changed how initial configurations are contributed by a command by the debug adapters. How this works in the current stable version is described here.

We changed this a bit and decided to give the full power of generating the initial 'launch.json' to the debug adapters - if a debug adapter contributes a command for providing the initial launch configuration.
This allows the content to contain comments. Inside the comments we recommend providing links to documentaiton so it is easier for the user to setup his 'launch.json'. Also providing a minimal numer of configurations initialy is a good idea.

The current node debugger is good example of this. Though there is some ugly string magic in order to make this possible for the node debug case here. For regular adapters this should be more straightforward as only the string content should be sent and no conversion from json is required.

@weinand should we ping the debug adapter writers so they are aware of the change on time (before our next release notes)?

Contributor

isidorn commented Oct 12, 2016

In order to support this we changed how initial configurations are contributed by a command by the debug adapters. How this works in the current stable version is described here.

We changed this a bit and decided to give the full power of generating the initial 'launch.json' to the debug adapters - if a debug adapter contributes a command for providing the initial launch configuration.
This allows the content to contain comments. Inside the comments we recommend providing links to documentaiton so it is easier for the user to setup his 'launch.json'. Also providing a minimal numer of configurations initialy is a good idea.

The current node debugger is good example of this. Though there is some ugly string magic in order to make this possible for the node debug case here. For regular adapters this should be more straightforward as only the string content should be sent and no conversion from json is required.

@weinand should we ping the debug adapter writers so they are aware of the change on time (before our next release notes)?

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand
Member

weinand commented Oct 12, 2016

@isidorn isidorn referenced this issue Oct 12, 2016

Closed

Test: launch.json usability improvements #13585

2 of 2 tasks complete

@isidorn isidorn added the on-testplan label Oct 12, 2016

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 12, 2016

Contributor

Is this a breaking change?

Contributor

felixfbecker commented Oct 12, 2016

Is this a breaking change?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 12, 2016

Contributor

@felixfbecker forgot to mention - we will still support contributing the 'initialConfigurations' from the 'package.json' of a debug adapter. But that approach has less power as it is impossible to add comments using that approach.

However it is a breaking change in a sense that contributing the 'initialConfigurations' via a command now always has to return the full content of a 'launch.json' - and not only the 'configurations' part.

Contributor

isidorn commented Oct 12, 2016

@felixfbecker forgot to mention - we will still support contributing the 'initialConfigurations' from the 'package.json' of a debug adapter. But that approach has less power as it is impossible to add comments using that approach.

However it is a breaking change in a sense that contributing the 'initialConfigurations' via a command now always has to return the full content of a 'launch.json' - and not only the 'configurations' part.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 12, 2016

Contributor

Okay, haven't implemented the initialConfigurations request yet, so shouldn't break anything on my side. Will you bump the major version then if this is a breaking protocol change?

Contributor

felixfbecker commented Oct 12, 2016

Okay, haven't implemented the initialConfigurations request yet, so shouldn't break anything on my side. Will you bump the major version then if this is a breaking protocol change?

@egamma

This comment has been minimized.

Show comment
Hide comment
@egamma

egamma Oct 12, 2016

Member

I suggest to

  • when an attribute like program is inferred we should add a postfix comment, how it was inferred, e.g.,
    // from package.json/'main'
  • why is cwd in the default list? It has a good default
  • it would be nice to colorize attributes that are 'read only' (version, type, request) differently than attributes that are 'editable' by the user. One way to achieve this is to have a custom grammar for launch.json that colorizes the read only attribute names in grey, for example.
  • group attributes that are edited by the user from attributes that are read only, that is, move name down, see below
        {
            "type": "node",
            "request": "launch",

            "name": "Launch",          
            "program": "${workspaceRoot}/bin/www", // from package.json/'main'
            "args": [],
            "cwd": "${workspaceRoot}"
Member

egamma commented Oct 12, 2016

I suggest to

  • when an attribute like program is inferred we should add a postfix comment, how it was inferred, e.g.,
    // from package.json/'main'
  • why is cwd in the default list? It has a good default
  • it would be nice to colorize attributes that are 'read only' (version, type, request) differently than attributes that are 'editable' by the user. One way to achieve this is to have a custom grammar for launch.json that colorizes the read only attribute names in grey, for example.
  • group attributes that are edited by the user from attributes that are read only, that is, move name down, see below
        {
            "type": "node",
            "request": "launch",

            "name": "Launch",          
            "program": "${workspaceRoot}/bin/www", // from package.json/'main'
            "args": [],
            "cwd": "${workspaceRoot}"

@egamma egamma reopened this Oct 12, 2016

@egamma

This comment has been minimized.

Show comment
Hide comment
@egamma

egamma Oct 12, 2016

Member

reopening to continue the discussion

Member

egamma commented Oct 12, 2016

reopening to continue the discussion

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 12, 2016

Contributor

args could be removed as well. But I disagree with the grey coloring, why should these attributes be "read-only". They are just as editable as any other attribute. The comments would introduce more clutters imo.

Contributor

felixfbecker commented Oct 12, 2016

args could be removed as well. But I disagree with the grey coloring, why should these attributes be "read-only". They are just as editable as any other attribute. The comments would introduce more clutters imo.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 12, 2016

Member

@egamma yes, cwd has a good default but the debug adapter does not know that value, so it cannot use that value if cwd is missing.
There is no hidden 'side-channel' for passing values like the workspace root from VS Code to the adapter; all values are passed explicitly via the launch config.

Member

weinand commented Oct 12, 2016

@egamma yes, cwd has a good default but the debug adapter does not know that value, so it cannot use that value if cwd is missing.
There is no hidden 'side-channel' for passing values like the workspace root from VS Code to the adapter; all values are passed explicitly via the launch config.

@delmyers

This comment has been minimized.

Show comment
Hide comment
@delmyers

delmyers Oct 12, 2016

@pieandcakes @edumunoz for visibility.

If I read this correctly, we should be expecting our debug adapters to just stop working in the near future. Is this already checked in? What is the best way to test?

delmyers commented Oct 12, 2016

@pieandcakes @edumunoz for visibility.

If I read this correctly, we should be expecting our debug adapters to just stop working in the near future. Is this already checked in? What is the best way to test?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 12, 2016

Member

I like this change, since I always thought the initial configs were a little overwhelming at first look, and I like stripping them down to the bare minimum.

Member

roblourens commented Oct 12, 2016

I like this change, since I always thought the initial configs were a little overwhelming at first look, and I like stripping them down to the bare minimum.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 12, 2016

Member

I would also like colorizing type/request - @felixfbecker I see them as special in that if you change one, you've totally changed the config and probably have to start over/change lots of other fields. If you need to edit a launch config, you probably start with the other fields, so makes sense to emphasize them.

Member

roblourens commented Oct 12, 2016

I would also like colorizing type/request - @felixfbecker I see them as special in that if you change one, you've totally changed the config and probably have to start over/change lots of other fields. If you need to edit a launch config, you probably start with the other fields, so makes sense to emphasize them.

@edumunoz

This comment has been minimized.

Show comment
Hide comment
@edumunoz

edumunoz Oct 12, 2016

Contributor

Awesome changes to make launch.json less daunting to use and configure 👍

@delmyers, my understanding is that this shouldn't break our existing C++ debug adapter given what @isidorn mentioned here:

we will still support contributing the 'initialConfigurations' from the 'package.json' of a debug adapter

Contributor

edumunoz commented Oct 12, 2016

Awesome changes to make launch.json less daunting to use and configure 👍

@delmyers, my understanding is that this shouldn't break our existing C++ debug adapter given what @isidorn mentioned here:

we will still support contributing the 'initialConfigurations' from the 'package.json' of a debug adapter

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 12, 2016

There are two primary reasons PowerShell folks even need to go into launch.json at all. The first reason is to provide args to the script they are about to execute. I think it would be far more intuitive if there was a way for the user to provide args via the Debug view toolbar. Perhaps there could be an icon before the "Configure or Fix 'launch.json'" that would allow a user to supply debug args or easily clear out the previous debug args.

The second reason is to set the "startup script". Right now the PowerShell package json configures the launch.json program: setting to be set to "${file}". This is a really good default as this allows the PowerShell user to open the script in the editor they want to debug and then press F5. However, occasionally you'll have a set of PowerShell scripts where there is always a single startup script yet you're doing the bulk of your debugging in a different script. Having to switch files in the editor in this scenario is annoying. So I find myself modifying program: to point to my startup script. However, when I do this I find myself wishing I could do this form the Explorer view and/or with a command pallete command.

BTW as a PowerShell user, I really wish "program" was called "script" as that is the proper vernacular in PowerShell. :-)

What if I could right click on a file in the Explorer and click "Set as debug start program/script"? I'd like that. And selecting that same menu item again would toggle it off. If no "file" is set as the debug startup program/script, then the default behavior would be pulled from the program: setting in launch.json.

Comments and docs would be nice but it only goes so far

rkeithhill commented Oct 12, 2016

There are two primary reasons PowerShell folks even need to go into launch.json at all. The first reason is to provide args to the script they are about to execute. I think it would be far more intuitive if there was a way for the user to provide args via the Debug view toolbar. Perhaps there could be an icon before the "Configure or Fix 'launch.json'" that would allow a user to supply debug args or easily clear out the previous debug args.

The second reason is to set the "startup script". Right now the PowerShell package json configures the launch.json program: setting to be set to "${file}". This is a really good default as this allows the PowerShell user to open the script in the editor they want to debug and then press F5. However, occasionally you'll have a set of PowerShell scripts where there is always a single startup script yet you're doing the bulk of your debugging in a different script. Having to switch files in the editor in this scenario is annoying. So I find myself modifying program: to point to my startup script. However, when I do this I find myself wishing I could do this form the Explorer view and/or with a command pallete command.

BTW as a PowerShell user, I really wish "program" was called "script" as that is the proper vernacular in PowerShell. :-)

What if I could right click on a file in the Explorer and click "Set as debug start program/script"? I'd like that. And selecting that same menu item again would toggle it off. If no "file" is set as the debug startup program/script, then the default behavior would be pulled from the program: setting in launch.json.

Comments and docs would be nice but it only goes so far

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 12, 2016

BTW ${workspaceRoot} isn't always the best default for CWD during debugging. On PowerShell we use ${file} and then extract the parent dir of the script and use that as the cwd during debug. This is handy for debugging scripts buried several directory levels down. I believe PowerShell users would expect path args to be relative to the script's location.

rkeithhill commented Oct 12, 2016

BTW ${workspaceRoot} isn't always the best default for CWD during debugging. On PowerShell we use ${file} and then extract the parent dir of the script and use that as the cwd during debug. This is handy for debugging scripts buried several directory levels down. I believe PowerShell users would expect path args to be relative to the script's location.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 12, 2016

Contributor

@rkeithhill Isn't there ${fileDirname} for that?

I also think there needs to be a way to define args without touching launch.json. PHPUnit doesn't have .only() like Mocha so I use the --filter arg a lot to run specific tests, and it is really annoying to go into launch.json everytime.

Contributor

felixfbecker commented Oct 12, 2016

@rkeithhill Isn't there ${fileDirname} for that?

I also think there needs to be a way to define args without touching launch.json. PHPUnit doesn't have .only() like Mocha so I use the --filter arg a lot to run specific tests, and it is really annoying to go into launch.json everytime.

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 12, 2016

@felixfbecker RE ${fileDirname} - cool if it does exist. That initialConfiguration section of the PowerShell extensions package.json has changed in a long while. At the time, I found ${file} and was able to make it work. :-)

rkeithhill commented Oct 12, 2016

@felixfbecker RE ${fileDirname} - cool if it does exist. That initialConfiguration section of the PowerShell extensions package.json has changed in a long while. At the time, I found ${file} and was able to make it work. :-)

@pieandcakes

This comment has been minimized.

Show comment
Hide comment
@pieandcakes

pieandcakes Oct 12, 2016

Contributor

@delmyers We will need to do work for this but I assume then we can control which launch.json parameters to provide based on their OS without the user seeing other parameters that are not for their OS.

I'll create a task and we can schedule the work. as @edumunoz and @isidorn mentioned, it seems like they will be backwards compatible for at least the near future.

Contributor

pieandcakes commented Oct 12, 2016

@delmyers We will need to do work for this but I assume then we can control which launch.json parameters to provide based on their OS without the user seeing other parameters that are not for their OS.

I'll create a task and we can schedule the work. as @edumunoz and @isidorn mentioned, it seems like they will be backwards compatible for at least the near future.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 13, 2016

Member

@felixfbecker you could use the same approach as @jrieken in #12735 for asking the user for a specific test.

Member

weinand commented Oct 13, 2016

@felixfbecker you could use the same approach as @jrieken in #12735 for asking the user for a specific test.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 13, 2016

Member

@rkeithhill above you said "BTW as a PowerShell user, I really wish "program" was called "script" as that is the proper vernacular in PowerShell. :-)"

You are free to rename "program" to "script" because that is defined in the schema of your extension
(or better introduce a new attribute "script" and deprecate "program").

Member

weinand commented Oct 13, 2016

@rkeithhill above you said "BTW as a PowerShell user, I really wish "program" was called "script" as that is the proper vernacular in PowerShell. :-)"

You are free to rename "program" to "script" because that is defined in the schema of your extension
(or better introduce a new attribute "script" and deprecate "program").

@gregg-miskelly

This comment has been minimized.

Show comment
Hide comment
@gregg-miskelly

gregg-miskelly Oct 13, 2016

One question: the support for providing the initial launch.json from a command like the mock adapter does, is this something that is in VS Code today? Is it going to stick around?

For C#, we would much rather generate launch.json from the language service than the debug adapter. So having an extension command sounds good.

gregg-miskelly commented Oct 13, 2016

One question: the support for providing the initial launch.json from a command like the mock adapter does, is this something that is in VS Code today? Is it going to stick around?

For C#, we would much rather generate launch.json from the language service than the debug adapter. So having an extension command sounds good.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 13, 2016

Member

@gregg-miskelly we shipped this feature in the September release:

2016-10-13_17-19-41

Member

weinand commented Oct 13, 2016

@gregg-miskelly we shipped this feature in the September release:

2016-10-13_17-19-41

@isidorn isidorn closed this in fc962e0 Oct 13, 2016

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 13, 2016

Contributor

After taking into account the feedback here is how the initial 'launch.json' for node now looks in dark and light theme

screen shot 2016-10-13 at 18 00 33

screen shot 2016-10-13 at 18 00 45

Closing this as we have added the initial features which we wanted. I expect seperate issues and feature requests on how to further improve this.

We are graying out the 'type', 'request' and 'version' since the user is usually not interested in editing those. All debug adapters should profit from this.

Contributor

isidorn commented Oct 13, 2016

After taking into account the feedback here is how the initial 'launch.json' for node now looks in dark and light theme

screen shot 2016-10-13 at 18 00 33

screen shot 2016-10-13 at 18 00 45

Closing this as we have added the initial features which we wanted. I expect seperate issues and feature requests on how to further improve this.

We are graying out the 'type', 'request' and 'version' since the user is usually not interested in editing those. All debug adapters should profit from this.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 13, 2016

Contributor

We are graying out the 'type', 'request' and 'version' since the user is usually not interested in editing those.

I still disagree. Of course I as a user am interested in editing those. When I want to add a new configuration, I actually have to edit those.

Contributor

felixfbecker commented Oct 13, 2016

We are graying out the 'type', 'request' and 'version' since the user is usually not interested in editing those.

I still disagree. Of course I as a user am interested in editing those. When I want to add a new configuration, I actually have to edit those.

@MythicAngel

This comment has been minimized.

Show comment
Hide comment
@MythicAngel

MythicAngel Oct 16, 2016

So, launch.json is a special type of file now which is not a valid json file but file-extension is json? In my opinion, YML would have been better choice for this. Ref: http://stackoverflow.com/questions/244777/can-comments-be-used-in-json

👎

MythicAngel commented Oct 16, 2016

So, launch.json is a special type of file now which is not a valid json file but file-extension is json? In my opinion, YML would have been better choice for this. Ref: http://stackoverflow.com/questions/244777/can-comments-be-used-in-json

👎

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 16, 2016

Member

@MythicAngel VS Code always supported comments in all of its own setting files including launch.json. We were just not using them until this change.
Where are you seeing a problem with this?

Member

weinand commented Oct 16, 2016

@MythicAngel VS Code always supported comments in all of its own setting files including launch.json. We were just not using them until this change.
Where are you seeing a problem with this?

@MythicAngel

This comment has been minimized.

Show comment
Hide comment
@MythicAngel

MythicAngel Oct 16, 2016

Invalid JSON in a file with extension .json: the filenaming is strange to me.

No problem from my side, I'm just a normal (read: js, sql and php) user of vscode and didn't even know about the comment thing in settings area.

MythicAngel commented Oct 16, 2016

Invalid JSON in a file with extension .json: the filenaming is strange to me.

No problem from my side, I'm just a normal (read: js, sql and php) user of vscode and didn't even know about the comment thing in settings area.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 17, 2016

Contributor

@MythicAngel I proposed this once: #4851

Contributor

felixfbecker commented Oct 17, 2016

@MythicAngel I proposed this once: #4851

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 17, 2016

Member

Yes, this was already discussed in #4851 and the argumentation is still valid.

Member

weinand commented Oct 17, 2016

Yes, this was already discussed in #4851 and the argumentation is still valid.

@octref

This comment has been minimized.

Show comment
Hide comment
@octref

octref Oct 21, 2016

Member

While simplifying the config and reducing information overload are good, I think the syntax highlighting could be improved.

I agree with @felixfbecker . As user of course I'm interested in my debug target's name, as it's the label I'll be dealing with in Debug Viewlet. I certainly care if it's node or chrome and if it's launch or attach.

Contrary to what the highlighting suggests, the currently grayed-out options are the most important ones.

Member

octref commented Oct 21, 2016

While simplifying the config and reducing information overload are good, I think the syntax highlighting could be improved.

I agree with @felixfbecker . As user of course I'm interested in my debug target's name, as it's the label I'll be dealing with in Debug Viewlet. I certainly care if it's node or chrome and if it's launch or attach.

Contrary to what the highlighting suggests, the currently grayed-out options are the most important ones.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 21, 2016

Contributor

Okay, I've been using this new highlighting for a week now in insiders and want to share my honest feedback. I can only say it is horribly confusing. At first sight it just looks like the highlighting is plain broken. But it also goes against the point of syntax highlighting. Syntax highlighting is there so I can read/scan code quicker. In the JSON case that means keys and values are colored differently. So in my mind I scan the JSON from top to bottom and "read" the keys and their values. And of course I also scan the target and request, no matter if they are "important" in your opinion or not, because the eyes just go from top to bottom. Having both the key and the value grey just makes it harder to scan the whole file, it disturbs.

Besides that, the claim that these are not edited is wrong - adding a new launch config for me means selecting an existing, and using the keyboard shortcut to copy the lines down. And then go and edit the values, including request and type.

Contributor

felixfbecker commented Oct 21, 2016

Okay, I've been using this new highlighting for a week now in insiders and want to share my honest feedback. I can only say it is horribly confusing. At first sight it just looks like the highlighting is plain broken. But it also goes against the point of syntax highlighting. Syntax highlighting is there so I can read/scan code quicker. In the JSON case that means keys and values are colored differently. So in my mind I scan the JSON from top to bottom and "read" the keys and their values. And of course I also scan the target and request, no matter if they are "important" in your opinion or not, because the eyes just go from top to bottom. Having both the key and the value grey just makes it harder to scan the whole file, it disturbs.

Besides that, the claim that these are not edited is wrong - adding a new launch config for me means selecting an existing, and using the keyboard shortcut to copy the lines down. And then go and edit the values, including request and type.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 21, 2016

Member

This is great feedback, thanks a lot!

Member

weinand commented Oct 21, 2016

This is great feedback, thanks a lot!

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 21, 2016

Member

And after using regularly for a week, I still like it :)

I am often looking at launch configs and toggling 'sourceMap', or changing the args or program, but a 'node' launch config will always be 'node', so I want to focus on the props that distinguish one 'node' launch config from another. And I can still edit the other props if needed, but rarely do.

My only concern would be that it's not obvious at first why it's gray. Will people think that those lines are somehow disabled?

Member

roblourens commented Oct 21, 2016

And after using regularly for a week, I still like it :)

I am often looking at launch configs and toggling 'sourceMap', or changing the args or program, but a 'node' launch config will always be 'node', so I want to focus on the props that distinguish one 'node' launch config from another. And I can still edit the other props if needed, but rarely do.

My only concern would be that it's not obvious at first why it's gray. Will people think that those lines are somehow disabled?

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 21, 2016

Contributor

I should mention that I also have many projects with mixed launch configs. For example a repository with configs for debugging the PHP backend, the Chrome frontend or the Gulp build script.

Contributor

felixfbecker commented Oct 21, 2016

I should mention that I also have many projects with mixed launch configs. For example a repository with configs for debugging the PHP backend, the Chrome frontend or the Gulp build script.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Oct 21, 2016

Member

My 2 cents here after using it for a week.

As I'm using a color theme I wrote, at the first sight of this feature, the first thing that came into my mind is my color theme is terrible with this file. But later on I figured out that's a new feature and I started to like it as it greyed out both type and request which I don't want to ppl to modify if they don't understand what it means. So the only catch IMHO is how to educate users about this feature instead of confusing them.

Member

rebornix commented Oct 21, 2016

My 2 cents here after using it for a week.

As I'm using a color theme I wrote, at the first sight of this feature, the first thing that came into my mind is my color theme is terrible with this file. But later on I figured out that's a new feature and I started to like it as it greyed out both type and request which I don't want to ppl to modify if they don't understand what it means. So the only catch IMHO is how to educate users about this feature instead of confusing them.

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 23, 2016

or better introduce a new attribute "script" and deprecate "program"

@weinand Is there an officially supported way to declare a configuration attribute as deprecated?

Right now I'm leaving in program but changing the comment to indicate that it is deprecated and that the script property should be used instead.

rkeithhill commented Oct 23, 2016

or better introduce a new attribute "script" and deprecate "program"

@weinand Is there an officially supported way to declare a configuration attribute as deprecated?

Right now I'm leaving in program but changing the comment to indicate that it is deprecated and that the script property should be used instead.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Oct 24, 2016

Contributor

would be nice to set deprecated: true in the JSON schema for this.

Contributor

felixfbecker commented Oct 24, 2016

would be nice to set deprecated: true in the JSON schema for this.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 24, 2016

Member

@rkeithhill no, it is not yet possible to declare a configuration attribute as deprecated.
But I agree with @felixfbecker that we should introduce this.

I've created this feature request: #14260

Member

weinand commented Oct 24, 2016

@rkeithhill no, it is not yet possible to declare a configuration attribute as deprecated.
But I agree with @felixfbecker that we should introduce this.

I've created this feature request: #14260

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.