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

add . and ../ completions #237836

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

add . and ../ completions #237836

wants to merge 41 commits into from

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Jan 13, 2025

fixes #234351

@meganrogge meganrogge requested a review from Tyriar January 13, 2025 19:59
@meganrogge meganrogge self-assigned this Jan 13, 2025
@meganrogge meganrogge added this to the January 2025 milestone Jan 13, 2025
@meganrogge meganrogge enabled auto-merge (squash) January 13, 2025 20:00
@Tyriar
Copy link
Member

Tyriar commented Jan 13, 2025

Testing this out now. The ../ here should re-trigger the completions on Windows only, just as ..\ does. Right now it just continues the filter the items:

image

EDIT: Just closing the widget and triggering again shows this:

image

So I think we need to handle / on Windows too when resolving the resources.

@Tyriar
Copy link
Member

Tyriar commented Jan 13, 2025

I'm not seeing . show up on Windows. This is what I see:

image

Bottom of the list:

image

@Tyriar
Copy link
Member

Tyriar commented Jan 13, 2025

When you complete cd ..\ I see this:

image

Notice the odd filtering of the very last item ..\. It should show up first in the list and be fully blue? This would make it work really well with "terminal.integrated.suggest.runOnEnter": "exactMatchIgnoreExtension" and exactMatch.

@meganrogge meganrogge marked this pull request as draft January 13, 2025 20:15
auto-merge was automatically disabled January 13, 2025 20:15

Pull request was converted to draft

@meganrogge
Copy link
Contributor Author

When you complete cd ..\ I see this:

image

Notice the odd filtering of the very last item ..\. It should show up first in the list and be fully blue? This would make it work really well with "terminal.integrated.suggest.runOnEnter": "exactMatchIgnoreExtension" and exactMatch.

that's because looks like it has the wrong replacement index?

@Tyriar
Copy link
Member

Tyriar commented Jan 14, 2025

Some test cases to work through when we meet:

Test cases:

  • Windows
    • .| should show . (top) and ..\
    • ..\| should show ..\ (top) and ..\..\
    • ../| should show ..\ (top) and ..\..\
    • ..\ should show directories of ..\ (top)
    • ../..\ should show ..\..\ (top) and ..\..\..\
  • Mac
    • When directories are completed, the completed dir should always show at the top result (eg. ., .., ../, ../../, ./src/, ../xterm.js, etc.)
  • Both
    • Resolve actual folder name as detail for ., ..\, ~, etc.
    • Filtering on front end
      • Triggering at ./sr| should show the exact same results as when it's triggered at ./ and sr is typed
      • Triggering at ./src/cm| should find ./src/common
    • Without ./
      • Any ./ query should show exactly the same results as without the ./. Eg. ./src/| should be the same as src/|
      • cd c| should show src/ (or ./src/ as result)
    • Folders should be resolved even if the trailing / is missing
      • ./src should show ./src/ (top) and ./src/<other_dirs>
    • cd ./src/.. should not show multiple entries for cd ./src/../

Discussion:

  • ../ Scoring

    • Since this is easy to type and probably a low chance of being completed, should this go to the bottom unless it's an exact match?
      • eg. ./src/ shows ./src/ then ./src/../, taking up valuable real estate
  • Define behavior for when ../ shows up exactly

    • Should we show when another folder is typed?
      • Eg. ./src/ showing ./src/..?
        • If so, should that completion be smarter and "complete" to ./?
      • Should we just not show <prefix>../ unless it matches /(\.\.\/)+/?

@meganrogge meganrogge marked this pull request as ready for review January 14, 2025 22:24
@Tyriar
Copy link
Member

Tyriar commented Jan 14, 2025

We need to handle the Windows primary paths in the test:

Screenshot 2025-01-14 at 3 30 09 pm

@meganrogge meganrogge requested a review from Tyriar January 15, 2025 16:05
@meganrogge meganrogge enabled auto-merge (squash) January 15, 2025 16:13
Tyriar and others added 9 commits January 15, 2025 09:07
This was used for pwsh completions to ensure the last completed completion
always shows up so runOnEnter exactMatch works well. Path completions are
entirely handled in completion service now so we shouldn't need this special
behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have . and .. as completions when completing directories
2 participants