Skip to content

Add property default show context value proeprty#97920

Merged
JacksonKearl merged 4 commits intomicrosoft:masterfrom
Dzejkop:default_search_context_value
May 27, 2020
Merged

Add property default show context value proeprty#97920
JacksonKearl merged 4 commits intomicrosoft:masterfrom
Dzejkop:default_search_context_value

Conversation

@Dzejkop
Copy link
Contributor

@Dzejkop Dzejkop commented May 15, 2020

This PR fixes (implements?) #97915

Add the search.searchEditor.defaultShowContextValue settings option to configure the default value for the number of context lines.

To test add

"search.searchEditor.defaultShowContextValue": 2

to settings, a new search editor should now show 2 lines of context.

reusePriorSearchConfiguration should take precedence though, so with settings like

"search.searchEditor.reusePriorSearchConfiguration": true,
"search.searchEditor.defaultShowContextValue": 2

new search editor windows should show the previously set number of context lines.

@msftclas
Copy link

msftclas commented May 15, 2020

CLA assistant check
All CLA requirements met.

@JacksonKearl
Copy link
Contributor

I wonder how this should interact with reusePriorSearchConfiguration. Currently if that is enabled this does nothing, I think this should possibly take precedence over the prior configuration.

@Dzejkop
Copy link
Contributor Author

Dzejkop commented May 16, 2020

Makes sense to me.

If someone wants to have the old reusePriorSearchConfiguration behaviour they could just disable defaultShowContextValue. On the other hand with the current implementation if somebody wanted to reuse the prior config but override the default show context value - that wouldn't be possible.

Dzejkop added 2 commits May 16, 2020 15:38
defaultShowContextValue takes precedence
over
reusePriorSearchConfiguration
@JacksonKearl JacksonKearl self-requested a review May 27, 2020 18:59
Copy link
Contributor

@JacksonKearl JacksonKearl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I made one small change.


let config = { ...defaultConfig, ...priorConfig, ...existingData.config };

if (defaultShowContextValue !== null && defaultShowContextValue !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this so that setting the property to 0 has an effect.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants