helix
helix copied to clipboard
helix-editor
Configurable spinners
This PR adds the ability to customize the progress spinners. It changes the way they are created, as well, but this is not a huge issue because they weren't user-customizable beforehand anyway.
In summary:
The spinner can be defined in the config file using two keys under [editor] - spinner and spinner-interval. The former is a Vec<String> whose elements represent frames. The latter is the maximum update time in ms (equivalent to the interval that already exists in the code). Both default to the existing spinner.
This required allowing EditorView to take a ProgressSpinners argument, so I've changed the calling convention for EditorView::new.
If the spinner is multi-frame, then the current behavior where Instant::now() is repeatedly called still applies. However, for lower-powered systems, if the spinner is a single frame, then we can elide all the calls (and resulting syscalls) and save some performance.
The spinner can now be styled using ui.spinner in the theme. This allows the use of rapid_blink, for example, to simulate motion. (However - If the statusbar is non-black, I don't know yet how to make the background persist when the blink state goes to black.)
Anyway - please let me know what you think. I'm still not sure why we're using ProgressSpinners as a collection when it appears there's only ever one spinner. Could we simplify this?
@BearOve - I think I addressed/resolved all your suggestions. Mind taking another look? Thanks!
Yes you have. The only thing now is what happens if the frame specified is more than one visible character wide. However, it might even be a feature to have a 3 character spinner?
I will leave this for someone more familiar with the inner workings of Helix however.
Great work
The only thing now is what happens if the frame specified is more than one visible character wide. However, it might even be a feature to have a 3 character spinner?
I tried this last night and it works up to 3-4 chars before it bumps into the filename.
I was curious what you would want to replace the current spinner
Selfishly, I hate things that move erratically - I find them really distracting, and the LSP loading process stops and starts the spinner a few times (there's a CPU lag as well on my lower-performance VM). I replaced it with a single frame of a yellow dot. The single-frame notification also doesn't use Instant::now() so I save the syscalls.
But yes - I've been playing around with lots of alternate possibilities.
(Won't somebody think of the poor syscalls?)
Does it make sense for spinner-interval set to 0 to disable spinner display? We already have this functionality if spinner-frames is set to [].
Would it not make more sense to change this functionality to keep the previous logic (update when a key-press is made, instead of just removing the spinner)? I prefer this logic, because it prevents a render every time the spinner updates (which is 👍 if you're on a slow connection/ssh'd into a server).
Would it not make more sense to change this functionality to keep the previous logic (update when a key-press is made, instead of just removing the spinner)?
I'm not sure I understand. The current code doesn't allow you to configure the spinner interval at all, and if you manage to do it by editing the code, if you set it to zero, helix will panic on assert [1]. There's currently no logic that handles "update spinner on keypress".
Because the frame is empty, there will be no significant update - I run helix (and this proposed code) on a small VM over a slow SSH connection and haven't seen any issues. In fact, it's one of the primary reasons I'm submitting this PR, since the current behavior causes the spinner to move erratically.
[1] https://github.com/helix-editor/helix/blob/ad15e7b5e8918e1521683c7f5c63d77e00c58023/helix-term/src/ui/spinner.rs#L38
~~Changing this to WIP pending discussion of #2829~~ This was the wrong approach.
Does this PR affect how spinners work internally or just how they are displayed? While testing this PR I just ran into the situation where the spinner would not disappear, even after rust-analyzer finished loading.
Though, I've also recently updated rust-analyzer (427061da1 2022-06-19) which may have introduced this problem.
While testing this PR I just ran into the situation where the spinner would not disappear, even after rust-analyzer finished loading.
I've noticed this too, but I also noticed it without the PR, so I don't think it's the code here. The spinner indicator updated to the correct state (went away) when I pressed a key.
I didn't change any of the logic relating to spinner start/stop - just how it's displayed.
Also, it's not consistent. Here's a screen recording I just made with a single-char (yellow dot) spinner:
https://asciinema.org/a/Tjuz8EWfpLZiDHd6A24K1l9sW
I've noticed this too, but I also noticed it without the PR, so I don't think it's the code here. The spinner indicator updated to the correct state (went away) when I pressed a key.
I didn't change any of the logic relating to spinner start/stop - just how it's displayed.
Then it's probably an unrelated issue. In my case it really never disappeared, even after half an hour while editing the file.
Refactored for new customizable statusline. Width of spinner is now handled appropriately by the statusline!
Ready for review.
Been using this branch for some time with a simple one character custom spinner and have seen no obvious glitches.
