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

Regression for .. completion #111

Closed
EdVanDance opened this issue May 4, 2021 · 11 comments
Closed

Regression for .. completion #111

EdVanDance opened this issue May 4, 2021 · 11 comments
Labels
enhancement New feature or request

Comments

@EdVanDance
Copy link

I recently updated clink after a very long time (> 4y) and noticed that an issue I reported previously mridgers#277 appeared again.

A lot has changed since then, I couldn't figure out anything that might help to fix this issue.

Very happy, that you took over maintenance for clink @chrisant996. Kudos to you 👍

@chrisant996
Copy link
Owner

Are you saying the exact same issue occurs?
mridgers 277 says that ..Tab inserts a space.
When I use any of the completion commands in Clink v1.2.5, it does not insert a space.
It also does not insert a \, which I think is the main point, but I want to make sure that I'm understanding correctly -- because if it's also inserting a space then I need additional information to understand what's happening.

@chrisant996
Copy link
Owner

Also, it looks like how bash works is that . and .. are only considered possible completions if the current word is . or ...

That's a significant nuance, since otherwise menu-complete would always start out with .\ and then ..\, and that would be very annoying for anyone who uses menu-complete (like myself).

And this is another case where completion results are unstable based on the word that's typed. Clink v1.0.0 introduced a performance improvement that caches the completions for a word's position. It builds the completion list assuming the current word is empty ("") and then filters the completions based on the actual typed word. Clink's current design and implementation are incompatible with the requested behavior.

I'll see what I can do about it, though. I had to make changes 5e46b9a and 3146af2 to support ~ completion, which has a similar characteristic of "completion results are unstable based on the word that's typed". Hopefully a similar technique can be used to accommodate . and ...

@chrisant996 chrisant996 added the enhancement New feature or request label May 4, 2021
@EdVanDance
Copy link
Author

EdVanDance commented May 4, 2021

Are you saying the exact same issue occurs?

You are right, it's not exactly the same. Current clink does not insert a space, just nothing happens.

It also does not insert a , which I think is the main point

Exactly.

I'm not familiar with menu-complete, but I think I know what you mean.

that would be very annoying for anyone who uses menu-complete

Yeah, I can relate to that.

Clink's current design and implementation are incompatible with the requested behavior.

That's unfortunate.

I'll see what I can do about it, though.

Alright, thank you very much.

I'm wondering why nobody else has this "issue". As a workaround I'm going to try to get used to type ../ instead, at least on english keyboard layouts / is next to ..

@chrisant996
Copy link
Owner

chrisant996 commented May 8, 2021

Actually bash behaves interestingly:

  1. Type . then Alt+=: bash reports exactly one match, ./ -- this is a bit strange, isn't it? Why doesn't it report ./ and ../ as matches? Why doesn't it report ..foo.exe when one exists?
  2. Type .. then Alt+=: bash reports exactly one match, ../ -- this makes sense at first, but it's also strange: Why doesn't it report ..foo.exe when one exists?
  3. Type abc . then Alt+=: bash reports any matches that begin with . -- e.g. ./, ../, .inputrc, ..foo.exe, etc.
  4. Type abc .. then Alt+=: bash reports any matches that being with .. -- e.g. ../, ..foo.exe, etc.

Case 1 and 2 seem strange. Cases 3 and 4 seem like what I'd expect.

I will make Clink mimic bash for all 4 cases, though. (I mean that case 1 and 2 will mimic bash when exec.enable is on, but will behave like case 3 and 4 when exec.enable is off.)

@chrisant996
Copy link
Owner

For future reference:

I'm torn on this.

Yes, bash behaves this way.
But Readline does not.
And Clink is not trying to be bash nor is it trying to be a shell -- it is trying to be an enhancement for the CMD shell, using Readline for input.

This particular issue is in the gray area of bash vs Readline vs CMD, and so I'll mimic this bash behavior at least for now (pending complaints in the opposite direction).

There are many ways that Clink does not behave the same as bash, and that's both ok and intentional. For example, when completing the first word, bash only considers directories as possible completions if there is no file match, but Clink uses the exec.dirs setting to control whether to consider directories. Bash (or at least git-bash) is also very slow at collecting matches, but Clink is very fast -- part of the improved speed comes from little differences like the preceding.

@garoto
Copy link

garoto commented May 8, 2021

How is having to type an extra \ and the end of a cd .. a burden? \ is also very close to the ENTER key on a US-English layout keyboard. Looks like picking nits to me.

@chrisant996
Copy link
Owner

chrisant996 commented May 8, 2021

How is having to type an extra \ and the end of a cd .. a burden? \ is also very close to the ENTER key on a US-English layout keyboard. Looks like picking nits to me.

Thanks, @garoto, I don't disagree. At the same time, I recognize that a lot of Clink users are trying to get a bash-like experience with CMD, and muscle memory can be hard/annoying to retrain. I'm willing to support this particular case, especially since Martin had added support for it on request in the past (and his v1.0.0 rewrite had some internal plumbing to continue to support it, but the plumbing hadn't been fully hooked up before he disappeared).

Btw, Martin didn't support it the same as bash: Martin's fix made .Tab list .\ and ..\ as possible completions. But in bash .Tab completes uniquely to ./ (but only at the beginning of the first word in a command line). I've made Clink mimic bash in that respect as well, which I presume is related to some convention about how relative paths are used in *nix land. I spent a bit of time analyzing the many edge cases with bash and . or .. completions (as the first word in the input line, as a later word, at the beginning of a word, at the end of a word, complete command, menu-complete command, old-menu-complete command, and so on -- these all have slight differences, but they follow from specific principles so it made sense to support them). Clink should behave very closely to bash now for . and .. completions.

chrisant996 added a commit that referenced this issue May 8, 2021
And also fix any Lua scripts that add directory matches without trailing
path separators.

(Follow-up change to more thoroughly fix #111.)
chrisant996 added a commit that referenced this issue May 8, 2021
For example `bl"""ah` is recognized as `blah`, and so for consistency
the fix for #111 should recognize `.""".` as `..`.
@EdVanDance
Copy link
Author

@chrisant996 Thank you very much for bringing this behavior back.

@garoto Yeah I agree for US english layouts. I switched to US english a couple months ago, after using the German layout for 20+ years, where / is Shift+7 and \ is AltGr+- (AltGris the right Alt key) or Ctrl+Alt+- (- as in next to 0). Just pressing Tab after .. is pretty handy in that case.

@garoto
Copy link

garoto commented Dec 1, 2022

I go to great lenghts to try and always buy EN-US layout keyboards instead of a kb with my local layout even if that means expending more monies. Way less headaches for my specific computer-usage cases.

@chrisant996 chrisant996 reopened this Dec 28, 2022
@chrisant996
Copy link
Owner

chrisant996 commented Dec 29, 2022

I noticed all 4 cases of this problem are present again. I'll track down when/how it got reintroduced, and fix it again (and hopefully in a more future-proof way).

@chrisant996
Copy link
Owner

The regression was introduced in v1.3.1 in commit 9abce03 when converting the default file match generator from C++ to Lua, as part of introducing the autosuggest.async setting to stay responsive to input while generating suggestions.

I made two mistakes while converting the check for the input ending with one or two dots, which existed specifically (and only) to fix issue 111. The check isn't just a literal check for one or two dots -- it has to make sure the input is either . or .. or ends with /.. or \.., and it has to ignore all quotes in the input (for example, \"."""""."" must be considered a match to \..). I didn't fully/accurately convert a couple of C++ idioms into their corresponding Lua idioms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants