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

Fix explorer item creation and disposal #79383

Merged
merged 3 commits into from Sep 10, 2019

Conversation

@jeanp413
Copy link
Contributor

commented Aug 18, 2019

This PR reapplies the changes from #72627 and the improvements from here and here.
Adittionaly has some changes to fix #78153 reported issues

src/vs/base/browser/ui/tree/indexTreeModel.ts Outdated
@@ -219,7 +219,7 @@ export class IndexTreeModel<T extends Exclude<any, undefined>, TFilterData = voi

const result = this._setListNodeCollapsed(node, listIndex, revealed, collapsed!, recursive || false);

if (this.autoExpandSingleChildren && !collapsed! && !recursive) {
if (node !== this.root && this.autoExpandSingleChildren && !collapsed! && !recursive) {

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Aug 18, 2019

Author Contributor

This fixes the bug from the second gif from #78153 (comment)

This comment has been minimized.

Copy link
@isidorn

isidorn Aug 20, 2019

Contributor

Can you please put this in a seperate PR. I like this simple change and it is in the tree land so I would love if we merge it in unrelated to this.

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Aug 21, 2019

Author Contributor

Ok

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Aug 21, 2019

Author Contributor

Opened #79540 and it's corresponding PR #79542

@isidorn isidorn self-assigned this Aug 19, 2019
@isidorn isidorn added this to the August 2019 milestone Aug 19, 2019
src/vs/base/browser/ui/tree/indexTreeModel.ts Outdated
@@ -219,7 +219,7 @@ export class IndexTreeModel<T extends Exclude<any, undefined>, TFilterData = voi

const result = this._setListNodeCollapsed(node, listIndex, revealed, collapsed!, recursive || false);

if (this.autoExpandSingleChildren && !collapsed! && !recursive) {
if (node !== this.root && this.autoExpandSingleChildren && !collapsed! && !recursive) {

This comment has been minimized.

Copy link
@isidorn

isidorn Aug 20, 2019

Contributor

Can you please put this in a seperate PR. I like this simple change and it is in the tree land so I would love if we merge it in unrelated to this.

@@ -335,6 +343,13 @@ export class ExplorerView extends ViewletPanel {

this._register(this.tree.onContextMenu(e => this.onContextMenu(e)));

this._register(this.tree.onDidScroll(e => {
let editable = this.explorerService.getEditable();

This comment has been minimized.

Copy link
@isidorn

isidorn Aug 20, 2019

Contributor

Why not use getEditableData?
That API is already there and gives you everything you need.

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Aug 21, 2019

Author Contributor

getEditableData just return the data, I need the stat so I can call this.tree.getRelativeTop(editable.stat)

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@jeanp413 thanks a lot for your PR.
I have added some comments inline, can you please address them?

Also did you intensivly test this out? There are quite some corner cases (as you are probably aware of). And this code is quite delicate (hacky).
I tried it out quickly and seems to work well (but plan to test it more once we adress the things I mentioned in the review).

@jeanp413 jeanp413 force-pushed the jeanp413:fix-explorer-edit-item branch to 8e86e73 Aug 21, 2019
@jeanp413

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I tested it for the bugs reported in #72626, #75693. #75624, #78153
Did you find a corner case that's not covered?

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@jeanp413 thanks for the PR and for updating it.
However for now I have decided to go with the current approach. And we can change this next milestone after we get some feedback, thus assigning it to september. The current limitations of the approach are dangling input elements and the input box not closing on scroll. For now I can live with that.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

I have tested this now more. Seems to work fine and looks good. Let's merge it in to get feedback from insiders users.
Thanks a lot for this PR.

@isidorn isidorn merged commit af53133 into microsoft:master Sep 10, 2019
2 checks passed
2 checks passed
VS Code #20190821.5 succeeded
Details
license/cla All CLA requirements met.
Details
@jeanp413 jeanp413 deleted the jeanp413:fix-explorer-edit-item branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.