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

Use the correct case in completion pager #7744

Merged

Conversation

krobelus
Copy link
Member

@krobelus krobelus commented Feb 25, 2021

This resolves the regression mentioned in #7738 (comment) and the comments below.

The previous version revived b38a23a, but I didn't manage to fix all problems.
This version uses a simpler fix.

Add a tmux-based tests, including tests for recent interactive regressions #7526 and #7738.

(I coudn't get this pexpect test to work)
sendline("complete -e echo; complete echo -a 'abc aBd' -f")
send("echo a\t\t\t")
expect_re(r"echo aBc\b")

TODOs:

  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@krobelus krobelus added this to the fish 3.3.0 milestone Feb 26, 2021
@krobelus krobelus changed the title Use the correct case in completion pager WIP: Use the correct case in completion pager Mar 6, 2021
@krobelus krobelus force-pushed the faithful-case-in-completion-pager branch from 807cb57 to 1b014a8 Compare March 13, 2021 09:13
@krobelus krobelus modified the milestones: fish 3.3.0, fish 3.2.1 Mar 13, 2021
@krobelus krobelus changed the title WIP: Use the correct case in completion pager Use the correct case in completion pager Mar 13, 2021
@krobelus krobelus force-pushed the faithful-case-in-completion-pager branch from 1b014a8 to 5220d3b Compare March 13, 2021 09:35
@zanchey zanchey requested a review from faho March 14, 2021 12:29
Copy link
Member

@faho faho left a comment

Choose a reason for hiding this comment

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

This seems to work, but I've never been comfortable with this bit of the completions, so I wouldn't want to merge this for 3.2.1.

Unless you're really really sure that this time it's correct.

@krobelus
Copy link
Member Author

I'm sure it works, I just wasn't sure if it's okay to remove this feature. (Given touch someCompletion1 somecompletion2 we would insert the entire prefix when completing rm some.)

@krobelus krobelus force-pushed the faithful-case-in-completion-pager branch from 5220d3b to 8e1b937 Compare March 18, 2021 19:20
@ridiculousfish
Copy link
Member

Probably fine to merge now. Neat test!

Consider

	$ complete -c foo -a 'aab aaB' -f
	$ foo A<TAB>

since 28d67c8 we would insert the common prefix AND show the pager.
Due to case-insensitive comparison, "b/B" was considered to be part
of the prefix. Since the prefix is added to each pager item [1]
we get wrong results. Fix this by removing the insensitive comparison
between completions - I don't think it was of much use anyway.
Commandline tokens are still matched case-insensitively, this is
just about completions.

Test this by running interactive fish inside tmux (pexpect's terminal
emulation not have enough capabilities).  Also add tests for recent
interactive regressions fish-shell#7526 and fish-shell#7738.

Closes fish-shell#3978

[1]: b38a23a would solve this differently by giving every pager item
its own prefix, but was reverted since it needs more fixes.
@krobelus krobelus force-pushed the faithful-case-in-completion-pager branch from 8e1b937 to a4d0fdd Compare March 21, 2021 08:18
@krobelus
Copy link
Member Author

Added changelog.

The tmux tests are only run if tmux is installed - which is the case with GitHub Actions.
In future we can install it explicitly, and maybe document it.

@krobelus krobelus merged commit 7fea321 into fish-shell:master Mar 21, 2021
@zanchey
Copy link
Member

zanchey commented Apr 1, 2021

OK, I'm trying to get my head around this for the release notes. I can't reproduce the "wrong" behaviour in 3.2.1, which I think is because we backed out the case-insensitive completion stuff before the 3.2.0 release? And it doesn't look like we've added it back in?

@krobelus
Copy link
Member Author

krobelus commented Apr 1, 2021

We didn't back out smartcase completion. This bug was that the pager entries were incorrect after the pager's common-prefix-insertion.

Reproduce with
fish-3.2.1 -C 'complete -c foo -a "aab aaB" -f' and typing foo A followed by Tab.
The pager was aaB aaB but should be aab aaB.

@zanchey
Copy link
Member

zanchey commented Apr 6, 2021

Thanks. I think I'm confused about what smartcase completions actually are; with a directory containing a single file aBC, the behaviour of vim abTab is the same in 3.0.2 and 3.1.1. With a directory containing aBC and abC, likewise.

@krobelus
Copy link
Member Author

krobelus commented Apr 6, 2021

Fish uses "tiered" completions,

  1. If any completion prefix-matches, then only prefix matches are shown
  2. Else, if any completion substring-matches, then show only these matches (which includes 1.)
  3. Else, if any completion substring-matches case-insensitively, then show only these matches (includeing 1 & 2)
  4. Finally, show all completions that "subsequence-match", for example fbr matches foobar

So if some completion on 2 matches the token, then the case-insensitive-only matches (3) are not displayed, if that makes sense.

AIUI, smartcase is about showing both 2 and 3, if the string on the command line is all-lowercase.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants