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

#84678 add language-specific overrides for breadcrumb and outline settings #85081

Conversation

@petevdp
Copy link
Contributor

petevdp commented Nov 18, 2019

This PR fixes #84678 by setting the relevant configuration options as overridable and including override details when getting these values from the configuration.

I'm still working on getting the outline settings working the same way, but I'm opening this PR up early for any potential feedback and guidance.

Affected Settings

Affected settings include all of the settings in outline.contribution.tsand breadcrumbs.ts that have a corresponding filteredTypes setting.

Questions / Guidance

  • I'm currently trying to find out how to get the class responsible for filtering outlines OutlineFilter to have the correct context when reading the config. It seems like I either have to include a relevant service codeEditorService or pass OutlineFilter.update the necessary context. Does anyone have any thoughts? Currently leaning towards the former but I don't know the full implications that that might have.

  • When listening for configuration updates in the breadcrumbs model, I'm just checking for changes to configurations with either breadcrumbs or [<current language>] and updating that breadcrumb's outline if those are affected. Is this the way to do it or can I be more specific? It seems like the configuration service can only notify changes from the first layer of the configuration tree.

Testing

I haven't written any tests yet because I'm not sure if features like this are supposed to get them, but I can write some if that would be preferred.

});
}

return this._configurationService.getValue<boolean>(key);

This comment has been minimized.

Copy link
@jrieken

jrieken Nov 19, 2019

Member

You can use the ITextResourceConfigurationService which makes this a lot simpler

This comment has been minimized.

Copy link
@petevdp

petevdp Nov 19, 2019

Author Contributor

How's this? 9d9a99d

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 19, 2019

r pass OutlineFilter.update the necessary context. Does anyone have any thoughts? Currently leaning towards the former but I don't know the full implications that that might have.

Also use the ITextResourceConfigurationService and inside filter use the OutlineModel.get(element).textModel.uri

petevdp added 3 commits Nov 19, 2019
…guration overides when reading breadcrumb configurations involving filteredTypes
… corresponding filteredTypes
@petevdp

This comment has been minimized.

Copy link
Contributor Author

petevdp commented Nov 19, 2019

r pass OutlineFilter.update the necessary context. Does anyone have any thoughts? Currently leaning towards the former but I don't know the full implications that that might have.

Also use the ITextResourceConfigurationService and inside filter use the OutlineModel.get(element).textModel.uri

implementation: 25a1bbb

@petevdp

This comment has been minimized.

Copy link
Contributor Author

petevdp commented Nov 19, 2019

A callback is registered here which refilters the outline tree on config reload. Like the similar logic from BreadcrumbsModel, should we try to figure out what language is being used? I'm not sure where to get this information, or if that even makes sense in this context. A temporary fix would be to always filter on configuration change, but that wouldn't be ideal.

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 20, 2019

A temporary fix would be to always filter on configuration change, but that wouldn't be ideal.

Ideally we get a better API here but what you could try is e.affectedKeys - it lists all changed keys and you could check for the occurrence of outline in there, e.g search for /$\.?outline\./

@petevdp

This comment has been minimized.

Copy link
Contributor Author

petevdp commented Nov 20, 2019

e.affectedKeys is subject to the same limitation unfortunately, but maybe I could regex outline and language specific sections e.g /(outline|\[\w+\])/
edit: 33d8569

…new OutlineFilter implementation
@@ -53,9 +55,9 @@ export class EditorBreadcrumbsModel {
private readonly _uri: URI,
private readonly _editor: ICodeEditor | undefined,
@IConfigurationService private readonly _configurationService: IConfigurationService,
@ITextResourceConfigurationService private readonly _textResourceConfigurationService: ITextResourceConfigurationService,

This comment has been minimized.

Copy link
@jrieken

jrieken Nov 28, 2019

Member

The "normal" ICOnfigurationService shouldn't be needed anymore and everything should be possible using the text resource based service

This comment has been minimized.

Copy link
@petevdp

petevdp Nov 28, 2019

Author Contributor

TextResourceConfigurationService doesn't appear to be a strict superset of ConfigurationService, and unfortunately BreadcrumbsConfig uses one of the unported methods updateValue right here in method BreadcrumbsConfig.updateValue This method is, in turn, bound and used by two breadcrumb configuration options, here and here.

This more directly relates to your other coment on BreadcrumbsControl since the references to updateValue are all in there, but I'm guessing we don't want to have to remove BreadcrumbsConfig as a dependency of either class.

What I have right now definitely isn't the ideal solution though, so I could try to bring TextResourceConfigurationService in line with ConfigurationService either in this PR, or get another pr merged first if this issue is too tangential.

I didn't really check to see if I could just use the one config service though, so thanks for pointing it out!

This comment has been minimized.

Copy link
@jrieken

jrieken Nov 29, 2019

Member

That's for the explaining - I learnt something :-). Yeah, the TextResourceConfigService seems to be limiting and causing less pretty code /cc @sandy081. Let's keep the PR free from cleaning that up.

This comment has been minimized.

Copy link
@sandy081

sandy081 Dec 1, 2019

Member

Filed #85884 to enhance ITextResourceConfigService

@@ -168,6 +169,7 @@ export class BreadcrumbsControl {
@IThemeService private readonly _themeService: IThemeService,
@IQuickOpenService private readonly _quickOpenService: IQuickOpenService,
@IConfigurationService private readonly _configurationService: IConfigurationService,
@ITextResourceConfigurationService private readonly _textResourceConfigurationService: ITextResourceConfigurationService,

This comment has been minimized.

Copy link
@jrieken

jrieken Nov 28, 2019

Member

Don't have two configuration services (see my other comment)

This comment has been minimized.

Copy link
@petevdp

petevdp Nov 28, 2019

Author Contributor

Left a reply over there as well which is relevant here.

src/vs/editor/contrib/documentSymbols/outlineTree.ts Outdated Show resolved Hide resolved
@petevdp petevdp requested a review from jrieken Nov 28, 2019
@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 29, 2019

This is looking good. @petevdp Can you fix merge conflicts and then I'll bring this in for the Nov release

@petevdp

This comment has been minimized.

Copy link
Contributor Author

petevdp commented Nov 29, 2019

Apologies, this merge is going to be broken... Not sure why I thought it would be a good idea to push this without at least testing it. doh. I'll try to resolve this shortly.

edit: I think everything still works actually. All tests are passing and after manually testing the feature I added, it all works still. Again, if any automated testing should be included, let me know and I'll see what I can do.

this._treeFilter.update();
// This is a temporary solution to try and minimize refilters while
// ConfigurationChangeEvents only provide the first section of the config path.
if (e.affectedKeys.some(key => key.search(/(outline|\[\w+\])/))) {

This comment has been minimized.

Copy link
@jrieken

jrieken Dec 2, 2019

Member

fyi - this lacks a check for === 0. I will fix this one master after merge.

@jrieken jrieken added this to the November 2019 milestone Dec 2, 2019
@jrieken jrieken merged commit 574f66e into microsoft:master Dec 2, 2019
4 checks passed
4 checks passed
linux
Details
windows
Details
darwin
Details
license/cla All CLA requirements met.
Details
@petevdp

This comment has been minimized.

Copy link
Contributor Author

petevdp commented Dec 2, 2019

Thanks for everyone's help!

@petevdp petevdp deleted the petevdp:language-specific-outline-and-breadcrumbs-settings-overrides branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.