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.

Sign up for GitHub

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

Jump to bottom

#34685 Collapse search results one level at a time #37451

Merged
merged 2 commits into from
Nov 4, 2017
Merged

#34685 Collapse search results one level at a time #37451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
263575c
#34685 Collapse search results one level at a time
Nov 1, 2017
9a4d3f0
Separate CollapseAll and CollapseDeepestExpandedLevel actions
Nov 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/vs/base/parts/tree/browser/tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,10 @@ export interface ITree extends Events.IEventEmitter {

/**
* Collapses several elements.
* Provide no arguments and it will recursively collapse all elements in the tree
* Collapses all elements at the greatest tree depth that has expanded elements.
* The returned promise returns a boolean for whether the elements were collapsed or not.
*/
collapseAll(elements?: any[], recursive?: boolean): WinJS.Promise;
collapseDeepestExpandedLevel(): WinJS.Promise;
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave collapseAll in place, having collapseDeepestExpandedLevel as a new method


/**
* Toggles an element's expansion state.
Expand Down
2 changes: 1 addition & 1 deletion src/vs/base/parts/tree/browser/treeDefaults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ export class CollapseAllAction extends Action {
return TPromise.as(null); // Global action disabled if user is in edit mode from another action
}

this.viewer.collapseAll();
this.viewer.collapseDeepestExpandedLevel();
Copy link
Member

Choose a reason for hiding this comment

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

This action shouldn't be called CollapseAllAction anymore

this.viewer.clearSelection();
this.viewer.clearFocus();
this.viewer.DOMFocus();
Expand Down
4 changes: 2 additions & 2 deletions src/vs/base/parts/tree/browser/treeImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ export class Tree extends Events.EventEmitter implements _.ITree {
return this.model.collapse(element, recursive);
}

public collapseAll(elements: any[] = null, recursive: boolean = false): WinJS.Promise {
return this.model.collapseAll(elements, recursive);
public collapseDeepestExpandedLevel(): WinJS.Promise {
return this.model.collapseDeepestExpandedLevel();
}

public toggleExpansion(element: any, recursive: boolean = false): WinJS.Promise {
Expand Down
38 changes: 30 additions & 8 deletions src/vs/base/parts/tree/browser/treeModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,17 @@ export class Item extends Events.EventEmitter {
return result;
}

public getChildren(): Item[] {
var child = this.firstChild;
var results = [];
while (child) {
results.push(child);
child = child.next;
}

return results;
}

private isAncestorOf(item: Item): boolean {
while (item) {
if (item.id === this.id) {
Expand Down Expand Up @@ -882,18 +893,29 @@ export class TreeModel extends Events.EventEmitter {
return item.collapse(recursive);
}

public collapseAll(elements: any[] = null, recursive: boolean = false): WinJS.Promise {
if (!elements) {
elements = [this.input];
recursive = true;
}
var promises = [];
for (var i = 0, len = elements.length; i < len; i++) {
promises.push(this.collapse(elements[i], recursive));
public collapseDeepestExpandedLevel(): WinJS.Promise {
var levelToCollapse = this.findDeepestExpandedLevel(this.input, 0);

var items = [this.input];
for (var i = 0; i < levelToCollapse; i++) {
items = items
.map(node => node.getChildren())
.reduce((prev, current) => prev.concat(current), []);
Copy link
Member

Choose a reason for hiding this comment

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

We have a helper for this, arrays.flatten

}

var promises = items.map(child => this.collapse(child, false));
return WinJS.Promise.join(promises);
}

private findDeepestExpandedLevel(item: Item, currentLevel: number): number {
var expandedChildren = item.getChildren().filter(child => child.isExpanded());
if (!expandedChildren.length) {
return currentLevel;
}

return Math.max(...expandedChildren.map(child => this.findDeepestExpandedLevel(child, currentLevel + 1)));
}

public toggleExpansion(element: any, recursive: boolean = false): WinJS.Promise {
return this.isExpanded(element) ? this.collapse(element, recursive) : this.expand(element);
}
Expand Down
29 changes: 5 additions & 24 deletions src/vs/base/parts/tree/test/browser/treeModel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,21 +614,18 @@ suite('TreeModel - Expansion', () => {
});
});

test('collapseAll', (done) => {
test('collapseDeepestExpandedLevel', (done) => {
model.setInput(SAMPLE.DEEP2).done(() => {
model.expand(SAMPLE.DEEP2.children[0]).done(() => {
model.expand(SAMPLE.DEEP2.children[0].children[0]).done(() => {

assert(model.isExpanded(SAMPLE.DEEP2.children[0]));
assert(model.isExpanded(SAMPLE.DEEP2.children[0].children[0]));

model.collapseAll().done(() => {
assert(!model.isExpanded(SAMPLE.DEEP2.children[0]));

model.expand(SAMPLE.DEEP2.children[0]).done(() => {
assert(!model.isExpanded(SAMPLE.DEEP2.children[0].children[0]));
done();
});
model.collapseDeepestExpandedLevel().done(() => {
assert(model.isExpanded(SAMPLE.DEEP2.children[0]));
assert(!model.isExpanded(SAMPLE.DEEP2.children[0].children[0]));
done();
});
});
});
Expand Down Expand Up @@ -684,22 +681,6 @@ suite('TreeModel - Expansion', () => {
});
});

test('top level collapsed', (done) => {
model.setInput(SAMPLE.AB).done(() => {

model.collapseAll([{ id: 'a' }, { id: 'b' }, { id: 'c' }]);

var nav = model.getNavigator();
assert.equal(nav.next().id, 'a');
assert.equal(nav.next().id, 'b');
assert.equal(nav.next().id, 'c');
assert.equal(nav.previous().id, 'b');
assert.equal(nav.previous().id, 'a');
assert.equal(nav.previous() && false, null);
done();
});
});

test('shouldAutoexpand', (done) => {
// setup
const model = new TreeModel({
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/browser/viewlet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,12 +259,12 @@ export class ToggleViewletAction extends Action {
export class CollapseAction extends Action {

constructor(viewer: ITree, enabled: boolean, clazz: string) {
super('workbench.action.collapse', nls.localize('collapse', "Collapse All"), clazz, enabled, (context: any) => {
super('workbench.action.collapse', nls.localize('collapse', "Collapse"), clazz, enabled, (context: any) => {
if (viewer.getHighlight()) {
return TPromise.as(null); // Global action disabled if user is in edit mode from another action
}

viewer.collapseAll();
viewer.collapseDeepestExpandedLevel();
Copy link
Member

Choose a reason for hiding this comment

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

I think we have two actions that do the same thing. Let's replace one with an action that calls collapseDeepestExpandedLevel, and leave the other. And let's be explicit in which trees were are changing the behavior of. We should change it for search results in this PR and maybe the file explorer. If we want other trees to have this behavior, we can change that later.

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've scoped it to just search results for now

viewer.clearSelection();
viewer.clearFocus();
viewer.DOMFocus();
Expand Down