Skip to content

Conversation

@jrieken
Copy link
Member

@jrieken jrieken commented Nov 9, 2020

This PR fixes #110188

@jrieken jrieken added this to the October 2020 Recovery milestone Nov 9, 2020
@jrieken jrieken self-assigned this Nov 9, 2020
@jrieken jrieken requested review from isidorn and sandy081 November 9, 2020 13:36
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

LGTM

Note: Did not verify the fix.

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

While this improves the situation with the workspace check. The Explorer still does not reveal. I am investigating further...

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

Ok, an issue remains, and that is that the fileService.resolveTo does not properly resolve when I pass in the wrongly cased drive letter.
@bpasero might you have an idea if the fileService also uses TernarySearchTree?

Code pointer where I use it
Since I pass in the wrong cased drive letter the resolveTo does not seem to be respected.

To repro on Win

  1. Put a breakpoint in my code pointer
  2. Open some workspace. Drag from Windows Explorer a file deep inside that workspace -> it does not get revealed

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

Ok here it is

trie = TernarySearchTree.forUris<true>();

Checking if changing it there also helps...

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

Yeah, that would also have to be changed. I have verified with that change that the issue is fixed.

@bpasero
Copy link
Member

bpasero commented Nov 9, 2020

This code is actually literally copied from how @jrieken had it before I rewrote file service. I think it should use the file system provider's capability for the ternary tree there as well.

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

@bpasero yeah I just pushed that. Please review and let me know what you think.
If you think it is good I will also push to master.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Left a comment. Created #110241 for the actual solution.

// lazy trie to check for recursive resolving
if (!trie) {
trie = TernarySearchTree.forUris<true>();
trie = TernarySearchTree.forUris<true>(!this.hasCapability(resource, FileSystemProviderCapabilities.PathCaseSensitive));
Copy link
Member

Choose a reason for hiding this comment

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

use isPathCaseSensitive in that file

@jrieken
Copy link
Member Author

jrieken commented Nov 9, 2020

Hm... Should we simply bring back the old behaviour instead of picking on a few places?

@isidorn
Copy link
Collaborator

isidorn commented Nov 9, 2020

@jrieken yeah that might be best.

@jrieken jrieken closed this Nov 9, 2020
@jrieken
Copy link
Member Author

jrieken commented Nov 9, 2020

closing and creating another PR which basically undoes my initial refactoring

@jrieken jrieken deleted the joh/fix110188 branch November 9, 2020 17:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 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.

5 participants