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.

Sign up for GitHub

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

Jump to bottom

cd auto-completion quoting issue on Windows hosts #897

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

cd auto-completion quoting issue on Windows hosts #897

donkahlero opened this issue Jan 16, 2020 · 7 comments

Comments

@donkahlero
Copy link
Contributor

donkahlero commented Jan 16, 2020 •
edited

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!
@donkahlero
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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).

@donkahlero
Copy link
Contributor Author

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

@xiaq
Copy link
Member

xiaq commented Jan 17, 2020

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

@donkahlero
Copy link
Contributor Author

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?

donkahlero added a commit to donkahlero/elvish that referenced this issue Jan 22, 2020
@donkahlero
Appends pathSeparator to dir name
35935f1 
* 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.
donkahlero added a commit to donkahlero/elvish that referenced this issue Jan 29, 2020
@donkahlero
Appends pathSeparator to dir name
9b22ab3 
* 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.
donkahlero added a commit to donkahlero/elvish that referenced this issue Jan 29, 2020
@donkahlero
Appends pathSeparator to dir name
c718160 
* 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.
donkahlero added a commit to donkahlero/elvish that referenced this issue Jan 29, 2020
@donkahlero
Appends pathSeparator to dir name
25d2363 
* 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.
donkahlero added a commit to donkahlero/elvish that referenced this issue Feb 1, 2020
@donkahlero
Appends pathSeparator to dir name
add057f 
* 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
Closed
xiaq pushed a commit that referenced this issue Feb 16, 2020
@donkahlero @xiaq
Appends pathSeparator to dir name
8214ca3 
* 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
Copy link
Member

xiaq commented Feb 16, 2020

Fixed in #903.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@xiaq @donkahlero