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

Improve ivy TRAMP support #285

Merged
merged 1 commit into from Nov 19, 2015
Merged

Improve ivy TRAMP support #285

merged 1 commit into from Nov 19, 2015

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2015

Modifications for better ivy/TRAMP support
for /protocol: syntax as well as /path/to//protocol:.

@ghost
Copy link
Author

ghost commented Nov 6, 2015

Sorry for the noise -- turns out I have no idea how to comment my PR on GitHub.

The commit is available with annotations:
d7328ec

@abo-abo
Copy link
Owner

abo-abo commented Nov 6, 2015

Thanks, it looks like a lot of changes. Since Ivy is in GNU ELPA, you can contribute only up to 15 lines without an Emacs Copyright Assignment. Do you have one?

@ghost
Copy link
Author

ghost commented Nov 6, 2015

Will shoot assign@gnu.org a message for the CA. I'll let you know once I've gotten it in order.

@abo-abo
Copy link
Owner

abo-abo commented Nov 6, 2015

Great! If you're in USA, it can be done faster electronically. I had to go through snail mail since I was in Germany back then, so it took around 1 month.

@PythonNut
Copy link
Contributor

I did mine electronically, but it took about a month anyway (no idea why).

Hope it gets done in short order! :)

@ghost
Copy link
Author

ghost commented Nov 18, 2015

Received confirmation of my Emacs assignment.

I noticed that there was a bug in my previous patch (6666cf8) when checking against the expanded filename (fixed in b39e20c). I expand them in the above PR in some places as well, so I will take some time to investigate if there are any related issues a bit later this week and update against the latest master.

In the meantime, if there is any commentary on the patch, especially related to the (= ivy--length 1) case that I'm proposing removing, it would be appreciated.

@abo-abo
Copy link
Owner

abo-abo commented Nov 18, 2015

Could you update the PR? I only see a version of code from 2 weeks ago. Is that still current?

@ghost
Copy link
Author

ghost commented Nov 18, 2015

Still current as of now; I'll have time tomorrow to update against master and check to make sure it isn't affected by the expanded path problem. Updated PR will be available at that time.

The part I was wondering though was the purpose of this line in master.

It seems like removing that condition ((= ivy--length 1)) results in better behavior in all cases, but I wanted to check to make sure that would not break an edge case that I'm not thinking about.

It could also be removed separately from my PR, if that's the case.

@abo-abo
Copy link
Owner

abo-abo commented Nov 18, 2015

The part I was wondering though was the purpose of this line in master.

It seems like removing that condition ((= ivy--length 1)) results in better behavior in all cases, but I wanted to check to make sure that would not break an edge case that I'm not thinking about.

It could also be removed separately from my PR, if that's the case.

I think it can be removed. It carried over from the general logic of trying to partial-complete on the first TAB and exit on the second TAB. But if the input is a perfect directory name, might as well exit on the first TAB.

@ghost
Copy link
Author

ghost commented Nov 19, 2015

PR is now updated against master.

@abo-abo abo-abo merged commit 39e6733 into abo-abo:master Nov 19, 2015
@abo-abo
Copy link
Owner

abo-abo commented Nov 19, 2015

Thanks, the change looks good to me. Could you maybe give some examples of the new things that work now and didn't work before? Should be useful for the manual.

abo-abo added a commit that referenced this pull request Nov 19, 2015
* ivy.el (ivy-alt-done): Match not only `ivy-text' but also
  `ivy--current' for TRAMP regex.

Re #285
@ghost
Copy link
Author

ghost commented Nov 19, 2015

Sure. In most cases it gives /protocol: matching behavior to /path/to//protocol::

Hostname completion:

Completions:

Prior: /path/to//protocol: TAB
Additional: /protocol: TAB

ivy:

Prior: /path/to//protocol: C-j
Additional: /protocol: C-j

Find file (continue in ivy):

Prior: /path/to//protocol:host: TAB <-- better behavior, continues in ivy rather than Completions
Additional: /protocol:host: TAB

Prior: /path/to//protocol:host: C-j
Additional: /protocol:host: C-j

File open (if path is a valid directory, it continues in ivy):

Prior: /path/to//protocol:host:/some/path C-j
Additional: /protocol:host:/some/path C-j

Quick jump to same directory as sudo:

Prior: /path/to//sudo:: C-j
Additional: No analogous, user probably wants ~root rather than / here.

@ghost ghost deleted the improve-tramp branch November 19, 2015 11:07
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.

None yet

2 participants