-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Asynchronous suggestions #180
Conversation
76b5763
to
73f6e24
Compare
73f6e24
to
6d00850
Compare
@ericfreese ping? |
@balta2ar Still alive, but I've been spending a lot of time on other projects lately. Still intending to get to this when I have more time. |
Ping? |
// @mafredri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was summoned so I added some comments, hope that's cool 😛.
Is there any particular reason for not using zsh-async
here, btw? You would get the older zsh support for free. Either way, if there's any improvements I can make to zsh-async
that would make you consider it here, let me know.
Also, this project looks interesting. Going to have to try it out 👍!
src/async.zsh
Outdated
|
||
while read -d $'\0' cmd; do | ||
# Kill last bg process | ||
kill -KILL $last_pid &>/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be safer here to kill -KILL %1 &>/dev/null
. There isn't a big risk for $last_pid
colliding with a new, existing process, but a tiny chance nonetheless. With the above command you guarantee that no other process than the child gets killed since zpty
has job control, just like a regular shell.
src/widgets.zsh
Outdated
if [ $#BUFFER -gt 0 ]; then | ||
if [ -z "$ZSH_AUTOSUGGEST_BUFFER_MAX_SIZE" -o $#BUFFER -lt "$ZSH_AUTOSUGGEST_BUFFER_MAX_SIZE" ]; then | ||
suggestion="$(_zsh_autosuggest_suggestion "$BUFFER")" | ||
_zsh_autosuggest_async_fetch_suggestion "$BUFFER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might I suggest feature detection? Takes care of supporting both sync/async, keeps the code simple and doesn't require a config option.
src/async.zsh
Outdated
|
||
_zsh_autosuggest_async_suggestion_ready() { | ||
# while zpty -rt $ZSH_AUTOSUGGEST_PTY_NAME suggestion 2>/dev/null; do | ||
while read -u $_ZSH_AUTOSUGGEST_PTY_FD -d $'\0' suggestion; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using zpty -r
to read the FD here would allow supporting older versions of zsh e.g. through kill
-signals. Doesn't support delimiter, but you can use a PATTERN as the last parameter, e.g. zpty -rt $ZSH_AUTOSUGGEST_PTY_NAME suggestion '*'$'\0'
. I've not used pattern before, though, so I could be wrong on the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was able to get rid of the carriage return by running stty -onlcr
in the pty
See #134 (comment) |
Thanks @z0rc. I think it makes sense seeing as z-asug has quite simple requirements. Benefits of |
@ericfreese: Remembered you had this goal here, so I thought I'd inform you I've figured out a way to do this in mafredri/zsh-async#13. So far it has worked reliably for me on a bunch of different systems and versions of zsh. |
Thanks for all the notes here @mafredri! I have been busy with holidays and other projects for a while but I'm hoping to get back to this soon. I've just started implementing high-level integration tests using tmux and RSpec (see tmux_integration_testing branch) to hopefully have a more robust test framework in place before continuing this async work. Will definitely dig into your comments more as I get back into this 😄 |
@ericfreese you're welcome! Just a FYI, |
6d00850
to
b530b0c
Compare
a1067d3
to
ed8056c
Compare
I don't think this is necessary. A little testing on my system shows sending
Honestly... I don't remember now that it's been a couple weeks since I made that comment. Everything seems to work fine now. I've also addressed silencing stderr output from the zpty server process. Mind taking another look and verifying that it is indeed fixed? I wasn't able to reproduce the The suggestions should work much better now. Please try the newest version and let me know if it solves the problems you were seeing earlier. I think we're getting pretty close here! Would love another round of feedback 🙏 @PythonNut, you might also be interested in checking this out. |
Well, with this branch the git status in pure prompt is broken. It uses zsh-async for fetching all git information and puts it in placeholder. When I switched to this branch the placeholder is always empty. Sorry, but I think I'll be sticking with z-asug async fork for a while. I'm on macOS 10.12 and zsh 5.3.1 installed via brew if this matters. I don't want to be rude, but given amount of code duplication and hacks in place I believe it would be more viable to use zsh-async rather than reimplementing the wheel here. |
@z0rc that pure fork has an old version of |
@mafredri I'm not using async.zsh from that fork and load it directly from https://github.com/mafredri/zsh-async. |
@z0rc Thank you for the feedback, but I do think we're very close here. I've fixed the issue you reported with the @mafredri In case you're curious, I had to also add the Edit: Was also able to reproduce and fix the issue with |
Went ahead and merged this into |
@ericfreese this seems to break my own usage of |
@PythonNut Can you verify the commit sha that you're using? There is a bug in |
@ericfreese interestingly, I was fine on 06fca77 but after testing develop (4321fc0), it's not behaving well with Pure / zsh-async. Also ran into this after a while: _zsh_autosuggest_async_pty_create:zpty:12: pty command name already used: zsh_autosuggest_pty
_zsh_autosuggest_async_pty_create:zle:22: Bad file descriptor number for -F: _zsh_autosuggest_async_response
_zsh_autosuggest_async_pty_create:zpty:12: pty command name already used: zsh_autosuggest_pty
_zsh_autosuggest_async_pty_create:zle:22: Bad file descriptor number for -F: _zsh_autosuggest_async_response |
Actually, a bisect shows 4321fc0 to be the offending commit, which seems weird. |
@mafredi You rock, thanks for the bisect. I was also intermittently seeing the |
@mafredri Are you still using z? Did you manage to come up with something about that? I ended up making regular backups of my ~/.z and adding functions to manually restore it. Of course, this is silly and it's not solving the root cause, but at least I can quickly restore ~/.z. |
@ericfreese I noticed another issue where zsh-asug still can interfere with other What I think can happen is that zsh-asug creates and deletes it's Example: diff --git a/zsh-autosuggestions.zsh b/zsh-autosuggestions.zsh
index 34ca080..dc5f7c2 100644
--- a/zsh-autosuggestions.zsh
+++ b/zsh-autosuggestions.zsh
@@ -529,6 +529,7 @@ _zsh_autosuggest_async_server() {
(
local suggestion
_zsh_autosuggest_strategy_$ZSH_AUTOSUGGEST_STRATEGY "$query"
+ sleep 0.5
echo -n -E "$suggestion"$'\0'
) &
@@ -589,6 +590,7 @@ _zsh_autosuggest_async_pty_destroy() {
}
_zsh_autosuggest_async_pty_recreate() {
+ sleep 0.5
_zsh_autosuggest_async_pty_destroy
_zsh_autosuggest_async_pty_create
} Here, the sleep inside The second sleep in Now I'm not 100% on the root issue, I'm just seeing the correlation here. @balta2ar I do use |
@mafredri I was just trying this out and I'm not able to reproduce the issue. I'm running Do you have any more information on how you were able to reproduce this? A failing spec would be awesome (relevant spec is |
@ericfreese are you inside a git folder with e.g. changes when testing? When I have the sleep there, the preprompt is never updated with a Should be even more easily visible / reproducible with sindresorhus/pure#273 because it relies on async even more. |
Yeah I was manipulating a git repo, staging, committing, resetting, etc. Everything seemed to work as expected (including seeing the
I'll try with this branch to see if I can reproduce. Edit: Maybe has to do w/ OS or zsh version? Would you mind giving your specs and I can try reproducing in a VM? |
@ericfreese sorry, missed your edit. I can reproduce on macOS ( |
@ericfreese here's a recording demonstrating the issue. I do have other plugins as well, e.g. |
Uses zpty to fetch suggestions asynchronously.
Work in progress. Based on mafredri's zsh-async.
See issue #170 and previous PR #134.
TODO
zpty
doesn't give a file descriptor.zpty
doesn't work (see zpty errors sindresorhus/pure#117)mc
zpty
receivingSIGHUP
when anyzpty
is deleted.zpty -d
killing all previously createdzpty
commands (or if no workaround, default to synchronous suggestions for now).