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

cd auto-completion quoting issue on Windows hosts #897

Closed
TacoVox opened this issue Jan 16, 2020 · 7 comments
Closed

cd auto-completion quoting issue on Windows hosts #897

TacoVox opened this issue Jan 16, 2020 · 7 comments

Comments

@TacoVox
Copy link
Contributor

@TacoVox TacoVox commented Jan 16, 2020

Let me start with thanking you for this wonderful project. I really enjoy it! And it works like a charm on my macOS machine.


However, I am forced to use MS Windows 10 at work. As I want to use elvish for my daily work, I encountered an issue when using the auto-completion on the press of TAB.

To visualize the problem, let me show you how a path builds up using the feature:

~ > cd
[TAB]
~ > cd go'\'
[TAB]
~ > cd 'go\pkg''\'

Needless to say, the user would expect that to look like:

~ > cd 'go\pkg\'

I believe to have identified some lines that are partly causing the issue:

pkg/edit/complete/raw_item.go:41+

func (c ComplexItem) Cook(q parse.PrimaryType) completion.Item {
	quoted, _ := parse.QuoteAs(c.Stem, q)

	return completion.Item{
		ToInsert:  quoted + c.CodeSuffix, // <- a simple concatenation
		ToShow:    c.Stem + c.DisplaySuffix,
		ShowStyle: c.DisplayStyle,
	}
}

Due to the concatenation, we have these multiple single cuoted expressions after one another.

As I see it, one could argue that whenever there is such a case, all single-quoted expressions should be combined into a single one. Howevever, as I just started looking at the code, I have not yet a complete overview and this may be a bad idea.

@xiaq what do you think? How could this issue be effectively avoided? I would be glad to help out.


Thanks for your work. Much appreciated!

@TacoVox

This comment has been minimized.

Copy link
Contributor Author

@TacoVox TacoVox commented Jan 16, 2020

To follow up on my issue:

What I mean is an additional function in the parse package that is capable of combining n quoted expressions into a single one. Of course it would need to feature logic to check, if we need to quote with single quotes or double quotes.

Is there any case where such a function would cause harm? Otherwise the marked line above could look something along the lines of:

func (c ComplexItem) Cook(q parse.PrimaryType) completion.Item {
	[...]
	return completion.Item{
		[...]
		ToInsert: parse.CombineQuotes(quoted, c.CodeSuffix),
		[...]
	}
}
@xiaq

This comment has been minimized.

Copy link
Member

@xiaq xiaq commented Jan 17, 2020

Rather than combining quotes, a simpler approach is to make it possible to attach a code suffix before quoting.

@xiaq

This comment has been minimized.

Copy link
Member

@xiaq xiaq commented Jan 17, 2020

Hmm, an even simpler way is to just add the path separator to the completion item itself without relying on CodeSuffix. This has the side effect of showing the path separator in the listing, but that is what every other shell does anyway (I tested bash, fish and zsh and they all do this).

@TacoVox

This comment has been minimized.

Copy link
Contributor Author

@TacoVox TacoVox commented Jan 17, 2020

That does sound like a nice approach. Shall I look into it? Would be glad to assisst.

@xiaq

This comment has been minimized.

Copy link
Member

@xiaq xiaq commented Jan 17, 2020

@TacoVox Feel free to hack the code and open a PR!

@TacoVox

This comment has been minimized.

Copy link
Contributor Author

@TacoVox TacoVox commented Jan 20, 2020

Just looked into the code and wanted to follow-up with you.

I agree, we should be able to ditch code suffix in the context of directories. The simplest approach I see is to just append it in generators.go. So something along the lines of:

[...]
		// Full filename for source and getStyle.
		full := dir + name

		suffix := " "
		if info.IsDir() {
			full += pathSeparator // new var name - unquoted
		}
[...]

Is that what you had intended? Or were you talking about changing/adding code to completion.go//Item? That would have imho the disadvantage that we would need to check for directories in there. As we are already doing that in generators.go, it would make sense to me to just append the path separator there.

What do you think?

TacoVox added a commit to TacoVox/elvish that referenced this issue Jan 22, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary

This is a fix for elves#897.
TacoVox added a commit to TacoVox/elvish that referenced this issue Jan 29, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary
* Updates test caes

This is a fix for elves#897.
TacoVox added a commit to TacoVox/elvish that referenced this issue Jan 29, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary
* Updates test caes

This is a fix for elves#897.
TacoVox added a commit to TacoVox/elvish that referenced this issue Jan 29, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary
* Updates test caes

This is a fix for elves#897.
TacoVox added a commit to TacoVox/elvish that referenced this issue Feb 1, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary
* Updates test caes

This is a fix for elves#897.
@Xjs Xjs mentioned this issue Feb 13, 2020
xiaq added a commit that referenced this issue Feb 16, 2020
* The old quotedPathSeparator was renamed to pathSeparator
* pathSeparator is not put inside quotes before it is used
* Instead of using the pathSeparator in quotes a suffix, it is appended
  to the full directory name and afterwords wrapped into quotes iff
  necessary
* Updates test caes

This is a fix for #897.
@xiaq

This comment has been minimized.

Copy link
Member

@xiaq xiaq commented Feb 16, 2020

Fixed in #903.

@TacoVox TacoVox closed this Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.