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

#85858 Allow breadcrumbs.symbolSortOrder per language #86430

Merged
merged 2 commits into from
Dec 9, 2019
Merged

#85858 Allow breadcrumbs.symbolSortOrder per language #86430

merged 2 commits into from
Dec 9, 2019

Conversation

jzyrobert
Copy link
Contributor

This PR fixes #85858
@jrieken Is this fine? I looked at how the other function uses OutlineModel, and it seems like it needs the input element, so I modified the function signature.

Example:
Code_-_OSS_rrrSXIlmlN
Javascript file without override uses default order:
Code_-_OSS_Ynj8nrJE6G
Typescript files use the overriden name order:
Code_-_OSS_vsHlKyvgAr

@jrieken jrieken added this to the December 2019 milestone Dec 6, 2019
@@ -440,7 +440,7 @@ export class BreadcrumbsOutlinePicker extends BreadcrumbsPicker {
this._symbolSortOrder = BreadcrumbsConfig.SymbolSortOrder.bindTo(this._configurationService);
}

protected _createTree(container: HTMLElement) {
protected _createTree(container: HTMLElement, element: BreadcrumbElement) {
Copy link
Member

Choose a reason for hiding this comment

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

Not so nice - given that there is _setInput. Why not update/modify the filter in there

Copy link
Contributor Author

@jzyrobert jzyrobert Dec 6, 2019

Choose a reason for hiding this comment

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

I don't think that works.
I tried:

	protected _setInput(input: BreadcrumbElement): Promise<void> {
		const element = input as TreeElement;
		const model = OutlineModel.get(element)!;
		const textModel = model.textModel;
		this._overrideConfiguration = {
			resource: textModel.uri,
			overrideIdentifier: textModel.getLanguageIdentifier().language
		};

and

	private _getOutlineItemCompareType(): OutlineSortOrder {
		switch (this._symbolSortOrder.getValue(this._overrideConfiguration)) {

However in testing, _getOutlineItemCompareType gets called before _setInput, making the override undefined.

This makes sense as _createTree is called before _setInput, and OutlineComparator only takes a value rather than a function, so you have to have the override ready at the time of _createTree

Copy link
Member

Choose a reason for hiding this comment

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

Just store the sorter in a property when _createTree is called and update it when _setInput is called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, like this?

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

looking good. thanks

@jrieken jrieken merged commit 7c18341 into microsoft:master Dec 9, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

Enable setting "breadcrumbs.symbolSortOrder" to be specific to languages
2 participants