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

[typescript] Better paths matching for move to existing file quickpick #181231

Merged
merged 6 commits into from
Aug 9, 2023

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented May 1, 2023

It's an improvement for #178535.

Moving statements to a sibling file is pretty common. In my case, I have nested file structure (5-6 levels deep) and it's hard to do this with current quickpick implementation, so I added simple filtering for ./ and ../ values. Now it can be done much faster than with a native file picker.

Screenshot 2023-05-01 at 07 56 35

Summarized:

  1. Now always adding full path to item description so it can be filtered with folder names in path e.g.
    Screenshot 2023-05-01 at 07 37 17 and added matchOnDescription
  2. Converted showQuickPick to createQuickPick API to implement relative path logic
  3. Make select existing file native picker open in file containing folder

Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. Looks like a nice addition but a few things look into before this can be merged

const filename = Utils.basename(uri);

let description;
const filterQuery = ['./', '../'].find(str => quickPick.value.startsWith(str));
Copy link
Contributor

Choose a reason for hiding this comment

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

On windows, paths will use \ instead. If possible though, it would be nice to support letting users type either ../ or ..\ on windows to access parent directories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course it is possible, not a big deal. Just didn't add support for it in the first place as never had to use \ in VS Code

const quickPick = vscode.window.createQuickPick();
const body = response.body;
const updateItems = () => {
const destinationItems = body.files.map((file): DestinationItem | undefined => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be optimized to not compute anything if the user has not typed ./ or ../ to start the path

use coalesce
@zardoy
Copy link
Contributor Author

zardoy commented May 2, 2023

By the way last time I tried it in insiders, I did see move to file code action (TS server didn't see interactive arg). Does it work on your end?

Update: I see it also works in latest insiders, my bad

@mjbvz mjbvz added this to the May 2023 milestone May 19, 2023
@mjbvz mjbvz modified the milestones: May 2023, June 2023 May 31, 2023
@mjbvz mjbvz modified the milestones: June 2023, July 2023 Jun 27, 2023
@mjbvz mjbvz modified the milestones: July 2023, August 2023 Jul 24, 2023
@mjbvz mjbvz enabled auto-merge (squash) August 9, 2023 20:05
@mjbvz mjbvz merged commit 432aac1 into microsoft:main Aug 9, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2023
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.

None yet

3 participants