vscode icon indicating copy to clipboard operation
vscode copied to clipboard

Handle workspace URIs that rely on query string

Open gjsjohnmurray opened this issue 11 months ago • 3 comments

This PR fixes #212363.

It supersedes PR #212436 which @bpasero pointed out was only targeting the breadcrumb issue.

Instead this PR ensures that all calls to getWorkspaceFolder() will prioritize matching on query and fragment over path-prefix matching on path. This idea was previously floated in https://github.com/microsoft/vscode/issues/139383#issuecomment-1089404482

This PR redefines the optional. second parameter of the UriIterator constructor (formerly _ignoreQueryAndFragment, now _prioritizeQueryAndFragment). I have reviewed how #215932 used that parameter to resolve #214329 and believe the behaviour will still be correct.

Pinging @isc-bsaviano who has a major interest in this fix.

gjsjohnmurray avatar Jan 15 '25 19:01 gjsjohnmurray

To be clear, I am not liking the original fix from Jackson that already added this inconsistency and removing his change, I cannot reproduce the issue it attempted to fix anymore in https://github.com/microsoft/vscode/issues/139383, but maybe I am testing it wrong 🤔

bpasero avatar Jan 27 '25 08:01 bpasero

@bpasero to test without Jackson's #146859 change did you simply remove the !this._ignoreQueryAndFragment condition within the reset method of UriIterator?

If so, and if after doing that the #139383 issue originally reported by @isc-bsaviano no longer repros, then perhaps some other change has cured that issue.

I noticed that a few months ago in #231715 @andreamah fixed another query string issue for @isc-bsaviano (https://github.com/microsoft/vscode/issues/227836) by subclassing TST. I doubt this could have rendered Jackson's change obsolete, but it's an example of another context in which special handling got added. Assuming @andreamah tried the TST ignoreQueryAndFragment option that Jackson had added, presumably it was insufficient.

Shall we try making TST's findSubstr always prioritize query and fragment matching over path matching, then see if anything breaks?

gjsjohnmurray avatar Jan 27 '25 14:01 gjsjohnmurray

Shall we try making TST's findSubstr always prioritize query and fragment matching over path matching, then see if anything breaks?

@bpasero any further thoughts about my January response to your feedback? @isc-bsaviano and I would like to make progress resolving this issue.

gjsjohnmurray avatar Apr 07 '25 11:04 gjsjohnmurray