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

Avoid flashes in dynamic picker #11193

Open
Philipp-M opened this issue Jul 15, 2024 · 3 comments
Open

Avoid flashes in dynamic picker #11193

Philipp-M opened this issue Jul 15, 2024 · 3 comments
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements
Milestone

Comments

@Philipp-M
Copy link
Contributor

It seems that the issue I've discovered in #9647 still persists, now in master.

It's minor, but I would expect that updating (i.e. clearing and populating) the picker item list happens in one frame before flushing the framebuffer to the terminal, to avoid flashes.

An asciinema to show the issue

@Philipp-M Philipp-M added the C-enhancement Category: Improvements label Jul 15, 2024
@archseer archseer added this to the next milestone Jul 16, 2024
@darshanCommits
Copy link

So like,
Don't update if there's no change in results?

@pascalkuthe
Copy link
Member

This is not simple and requires a fundamental refactor of how redraws are handeled with regard to jobs so we delayed it to future work.

@the-mikedavis
Copy link
Member

I meant to respond to this in the pickers v2 PR but I must've forgotten. @pascalkuthe and I discussed it and to add context to what Pascal is saying above: the issue is that when any job completes we re-render. The dynamic picker uses a job to get the injector from the picker and set up the async search:

job::dispatch_blocking(move |editor, compositor| {
let Some(Overlay {
content: picker, ..
}) = compositor.find::<Overlay<Picker<T, D>>>()
else {
return;
};
// Increment the version number to cancel any ongoing requests.
picker.version.fetch_add(1, atomic::Ordering::Relaxed);
picker.matcher.restart(false);
let injector = picker.injector();
let get_options = (callback)(&query, editor, picker.editor_data.clone(), &injector);
tokio::spawn(async move {
if let Err(err) = get_options.await {
log::info!("Dynamic request failed: {err}");
}
// NOTE: the Drop implementation of Injector will request a redraw when the
// injector falls out of scope here, clearing the "running" indicator.
});
})

So we currently render both when the injector pushes new options (which we do want to cause a render) and when the job that calls picker.matcher.restart(false); completes (which we don't want, causes the flash). the-mikedavis@02853e7 is a draft of how it would look to have jobs return whether or not they need a render. It's a fairly verbose change so I'd like to see if there are any other ways around it but that's what we're thinking at the moment

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements
Projects
None yet
Development

No branches or pull requests

6 participants