powerlevel9k icon indicating copy to clipboard operation
powerlevel9k copied to clipboard

Add Async Functionality per Segment

Open dritter opened this issue 5 years ago • 9 comments

This is my fourth iteration on async functionality and adds such (currently based solely on ZSH-Async) to every segment that is tagged as async. So, opposed to our earlier approaches, we don't render the whole prompt asynchronously, but we can render a mixture.

There is a major change how the segments work internally. Instead of printing each segment directly in p9k::prepare_segment this outputs now just the metadata of the segment. Additionally we build up a segment cache (in __p9k_build_segment_cache) that holds these metadata. When we are finished with building up the cache, we trigger rendering the prompt (__p9k_render is called), which stitches together all cached segments. With async segments, we wait for the callback (__p9k_async_callback), update the cache and re-render the prompt.

Preconditions

This branch is based on the segment_tags branch (#1171 ).

Caveat

This is still in a pretty raw stage. So don't expect everything to be working. I drafted this at an early stage, to get feedback as early as possible.

Todo

These things are still open:

  • [x] Avoid warning, if no segment is tagged as async, and therefore ZSH-Async is not loaded. This happens because async_flush_jobs and async_worker_eval are always called.
  • [ ] Support other async methods. See https://github.com/agkozak/agkozak-zsh-prompt#asynchronous-methods
  • [x] Avoid old prompts being eaten up. This happens on my machine when more than one segment is async, and I just press enter.
  • [ ] Make path to ZSH-Async configurable.
  • [ ] Make Placeholder configurable
  • [ ] Fix tests
  • [ ] Make gitstatus segment call async callback if timed out
  • [x] Fix status segment
  • [x] Fix command_execution_time segment
  • [x] Fix vi_mode segment. This contains old code based on generators/async.p9k..
  • [ ] Improve code structure. The clarity of the functions in generator/default.p9k could be improved..
  • [x] Every segment must report their metadata to the P9K engine. This is important as this example illustrates: If a segment is conditional and that condition is met if we enter a special directory, the cached data of the segment must be reset, if we leave the special directory. So p9k::prepare_segment must always be called (we have to look on conditions enclosing p9k::prepare_segment and early return statements). This is already done for most of the segments, but not for all:
    • [x] battery
    • [x] nvm
    • [x] rspec_stats
    • [x] symfony2_tests

dritter avatar Mar 03 '19 00:03 dritter

This breaks status. Reverting stauts.p9k to its previous commit (def5835e95ef106162a8443232f643e2317b4cf7) fixes it. The prompt is MUCH faster tho! Thank you!

Quentin-M avatar Mar 09 '19 03:03 Quentin-M

@Quentin-M Thanks for letting me know! I'll have a look at the status segment in the next days.

dritter avatar Mar 10 '19 15:03 dritter

The last prompt may be removed if too many async segments are enabled and newlines are added by

P9K_PROMPT_ADD_NEWLINE='true'
P9K_PROMPT_ON_NEWLINE='true'

In my test, I enabled as many async segments as possible, and entered a directory supporting many segment features. It turned out the last prompt may be removed/overwritten by the new prompt once you press <Enter> continuously.

Is there a competition among prompt redrawing activated by async segments?

Update:

Another problem is that, the async_jobs is unable to use environment variables set later. I tried export PYENV_VERSION=<some-venv>, the command pyenv version-name run by async_job is unable to detect the value. But in the main shell, pyenv version-name will output the value set by export PYENV_VERSION=... correctly.

laggardkernel avatar Jun 30 '19 06:06 laggardkernel

Good findings @laggardkernel . I'll have a look at that in the next couple of days.

dritter avatar Jul 02 '19 06:07 dritter

It turns out the async worker should be restarted in precmd to refresh the shell environment.

https://github.com/robobenklein/zinc/blob/28bea2b33e8fe1fc1319eab3c034f81bec8601b5/zinc_functions/prompt_zinc_setup#L102-L109

# restarts just the worker - in order to update worker with current shell values
function zinc_worker_restart () {
  # _ZINC_DBG_OUT "restarting worker..."
  # async_worker_eval zinc_segment_worker "zinc_reload"
  async_stop_worker zinc_segment_worker
  async_start_worker zinc_segment_worker
  async_register_callback zinc_segment_worker _zinc_segment_async_callback
}

laggardkernel avatar Jul 03 '19 01:07 laggardkernel

@laggardkernel Thanks for pointing me to this. I changed the code, so that the worker gets restarted in the precmd hook.

It still seems odd to me, that we have to restart the whole worker just to update the variables. Maybe a better approach would be to use async_worker_eval to update the necessary variables inside the worker.

dritter avatar Jul 04 '19 06:07 dritter

@dritter I think one of the benefits of this "conditional-async" is to make the segment functions work without any change of the code. Anyone could continue to contribute to the sections without realizing the existence of the async worker. Using async_worker_eval to update the necessary variables means that the contributors should take care of these variables, update the needed variables, and unset deprecated vars in time. Handing the job to the contributors may contradict the purpose above.

laggardkernel avatar Jul 04 '19 07:07 laggardkernel

The current implementation requires the segment functions pass out an additional conditional string to decide whether render the segment or not. Another function p9k::segment_no_print is added to handle early exit.

I have some idea to avoid this and make the async work with segment functions untouched. I'd like to make a pr to achieve this once you make a decision on my current pr.

Update: new pr is opened under your fork to implement the idea above.

laggardkernel avatar Jul 05 '19 11:07 laggardkernel

Using async_worker_eval to update the necessary variables means that the contributors should take care of these variables, update the needed variables, and unset deprecated vars in time. Handing the job to the contributors may contradict the purpose above.

Btw. @laggardkernel I was thinking of a general approach to transport the current env into the worker (might use a temp file for transport, like here). This could be done in the precmd hook, so you don't have to worry about that when writing new segments. I haven't profiled it, but it might be faster than restarting the whole async worker..

dritter avatar Jul 09 '19 23:07 dritter