Add commandline --showing-suggestion#10586
Conversation
|
Added unit tests and documentation. |
mqudsi
force-pushed
the
commandline-showing-suggestion
branch
6 times, most recently
from
e512820 to
51f6a8d
Compare
mqudsi
force-pushed
the
commandline-showing-suggestion
branch
from
51f6a8d to
443514e
Compare
| // never called in a stock fish installation, and expected to generally never end up | ||
| // actually calling yield_now() except under CI or on really slow machines.) | ||
| while reader.autosuggestion.is_empty() && !reader.in_flight_autosuggest_request.is_empty() { | ||
| std::thread::yield_now(); |
There was a problem hiding this comment.
This won't have the intended effect - autosuggestions are determined in the background and are applied via a callback (see autosuggest_completed). Simply yielding to the scheduler won't cause this to run.
I suggest simply removing this unless something breaks without it, in which case we'll have to understand why.
There was a problem hiding this comment.
The competition runs on the same thread? Because CI is broken without this loop.
There was a problem hiding this comment.
Yes, the completion runs on the same thread - aka the "main" thread.
The CI passed for me without this loop. Indeed if the loop were ever entered, it would be an infinite, hang which I was able to reproduce with some testing.
CI failures here are probably the normal runners-are-slow stuff which can be addressed by increasing the sleep in the tests.
Please remove this bit and up the sleep if necessary. Otherwise this looks good.
There was a problem hiding this comment.
Thanks for checking it again. I can confirm it's passing now on the regular runs.
I don’t think adding sleeps helps. Those are for the active highlight thread for the tty/prompt but we start this reader and return a response immediately regardless of how long that sleep was.
ridiculousfish
left a comment
ridiculousfish
left a comment
There was a problem hiding this comment.
I like the new feature but the thread::yield looks wrong to me.
Returns 0 (true) in case an autosuggestion is currently being displayed. This was first requested in fish-shell#5000 then again in fish-shell#10580 after the existing workaround for this missing functionality was broken as part of a change to the overall behavior of `commandline` (for the better).
mqudsi
force-pushed
the
commandline-showing-suggestion
branch
from
443514e to
a59dcf5
Compare
2 participants
Returns 0 (true) in case an autosuggestion is currently being displayed.
This was first requested in #5000 then again in #10580 after the existing workaround for this missing functionality was broken as part of a change to the overall behavior of
commandline(for the better).I haven't written the documentation or put in much work besides adding it and checking it more or less works as I would like to see if anyone has any objections to this.