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

feat: animating spinner #39

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Conversation

aidanaden
Copy link
Contributor

adds animation to the loading indicator

Comment on lines 69 to 102
set.spawn(async move {
let magnetease = Magnetease::new();
let res = magnetease.search(&search_phrase).await;
res
});

set.spawn(async move {
let tick_rate = tokio::time::Duration::from_millis(100);
let mut last_tick = tokio::time::Instant::now();
loop {
if last_tick.elapsed() >= tick_rate {
state_clone.lock().unwrap().calc_next();
last_tick = tokio::time::Instant::now();
// ctx_clone_next.send_action(Action::Render);
}
}
});

while let Some(res) = set.join_next().await {
match res {
Ok(res) => {
if res.is_empty() {
search_result_info_clone.lock().unwrap().not_found();
} else {
search_result_info_clone.lock().unwrap().found(res.len());
}

// TODO: add an X icon if no results, else V when results
table_clone.lock().unwrap().set_items(res);
ctx_clone.send_action(Action::Render);
}
Err(e) => {}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@micielski im trying to create a thread that updates the position of the loading spinner every 100ms - but the main thread seems to get stuck in the loop? is there a better way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining a tick() method in the component trait, and caling it from the app main loop every 100ms or so. But you still have to find a way to only draw this one widget instead of the whole window to make it reasonably efficient

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be okay to re-render the entire window every 100-200ms no? apparently ratatui only renders diffs so in the case where only the spinner's state changes it shouldn't be an issue?

Copy link
Contributor

@micielski micielski Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter whether ratatui only renders diffs or not; there's still widgets creation (especially table), mutexes etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to ratatui/ratatui#409, it seems like only rendering specific widgets isn't possible in ratatui.

i can experiment with rendering the entire window every 100-200ms when the loading indicator is rendered and see how it goes first

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more items in a table the worse it gets: ratatui/ratatui#1004
I'm currently in the process of planning on how to refactor this app to make state and ui more separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR currently adds the Tick action that triggers a re-render if the current SearchResultState is SearchResultState::Searching

could u help me test if there are any performance issues on your end ? would be good to know the amount of torrents (items in the list) that begin causing performance/rendering issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll investigate the performance later with more torrents because FreeBSD for some reason reports me much smaller CPU usage than Linux

@aidanaden aidanaden marked this pull request as draft June 24, 2024 05:57
@aidanaden aidanaden marked this pull request as ready for review June 25, 2024 19:01
Copy link
Contributor

@micielski micielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix these and I'll merge this

rm-main/Cargo.toml Outdated Show resolved Hide resolved
rm-main/src/app.rs Outdated Show resolved Hide resolved
@aidanaden aidanaden requested a review from micielski June 26, 2024 12:54
@micielski micielski merged commit 96cc5ea into intuis:main Jun 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants