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

Added option whether to show terminal reuse alert #44461

Merged
merged 6 commits into from Jun 7, 2018

Conversation

Projects
None yet
6 participants
@oriash93
Contributor

oriash93 commented Feb 27, 2018

Allow user to disable "Terminal will be reused by tasks, press any key to close it" message.

fixes #42993

@msftclas

This comment has been minimized.

msftclas commented Feb 27, 2018

CLA assistant check
All CLA requirements met.

@@ -193,6 +193,11 @@ configurationRegistry.registerConfiguration({
'type': 'boolean',
'default': false
},
'terminal.integrated.hideTerminalReuseAlert': {

This comment has been minimized.

@Tyriar

Tyriar Feb 27, 2018

Member

The terminal.integrated namespace currently doesn't have anything related to tasks/debug in it AFAIK. @dbaeumer is there a tasks settings namespace?

This comment has been minimized.

@dbaeumer

dbaeumer Feb 28, 2018

Member

Yes, everything in a tasks.json is basically a setting. So whether the message is shown or not should be a task settings and a global setting in the tasks.json.

The corresponding schema for this is here: https://github.com/Microsoft/vscode/blob/master/src\vs\workbench\parts\tasks\electron-browser\jsonSchema_v2.ts#L7

@dbaeumer dbaeumer self-requested a review Feb 28, 2018

@dbaeumer dbaeumer assigned dbaeumer and unassigned Tyriar Feb 28, 2018

@oriash93

This comment has been minimized.

Contributor

oriash93 commented Mar 1, 2018

I hope I got it right this time.

@dbaeumer dbaeumer added this to the March 2018 milestone Mar 1, 2018

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Mar 14, 2018

I looked at the PR and it goes into the right direction. Since tasks have already a presentation property I opt to include it there and not make it a first level attribute. Something:

"presentation": {
    ....
    "reuseMessage": boolean
}

The default should be true although not a falsify value. But it makes more sense then negating the key.

And you only need to adopt the V2 schema since the presentation attribute is only supported on V2.

@oriash93

This comment has been minimized.

Contributor

oriash93 commented Mar 15, 2018

@dbaeumer, I force-pushed the whole branch, but take a look at the last commit.

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Mar 16, 2018

Code looks OK to me. Can you add a test to ensure the configuration is passed correctly.

@bpasero bpasero modified the milestones: March 2018, April 2018 Apr 6, 2018

@oriash93

This comment has been minimized.

Contributor

oriash93 commented Apr 7, 2018

@dbaeumer I did the same as with "focus" property in configuration.test.ts.

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Apr 24, 2018

@oriash93 I need to push this to Mai. I was extremely busy with other things in April. Sorry for that.

@dbaeumer dbaeumer modified the milestones: April 2018, May 2018 Apr 24, 2018

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented May 30, 2018

Sorry for moving it again. I will promise to look into it first thing in June.

@dbaeumer dbaeumer modified the milestones: May 2018, June 2018 May 30, 2018

@dbaeumer dbaeumer merged commit f3966af into Microsoft:master Jun 7, 2018

1 of 2 checks passed

VSTS: VS Code 20180530.71 failed
Details
license/cla All CLA requirements met.
Details
@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Jun 7, 2018

I merged in the PR however I will tweak it a little. Mostly rename reuseMessage to showReuseMessage.

dbaeumer added a commit that referenced this pull request Jun 7, 2018

@oriash93 oriash93 deleted the oriash93:oriash93/42993 branch Jun 7, 2018

@gwk

This comment has been minimized.

gwk commented Jun 9, 2018

Is this available in the latest insiders build? I can't seem to get it to work but maybe I'm missing something.

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Jun 11, 2018

I should be in today's insider.

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