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

adding is_empty flag for prevent panic #145

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kyoto7250
Copy link
Contributor

@kyoto7250 kyoto7250 commented Apr 12, 2022

close #142

  • This PR adds offset is_empty flag for preventing panic in a filter.
    • this flag looks at the length of the list rather than the offset, so I'll rename it (PS. 2022/04/15)

This bug is caused by a moving ownership in a closure.

fn filterd_candidates(&self) -> impl Iterator<Item = &String> {
self.candidates.iter().filter(move |c| {
(c.starts_with(self.word.to_lowercase().as_str())
|| c.starts_with(self.word.to_uppercase().as_str()))
&& !self.word.is_empty()
})
}

I think if ListState is hidden and input is down or up arrow, then the ListState is not updated, and try to acccess to already moved variable again.

However, I didn't see a problem with the current closure implementation, so I responded by adding a flag to not call the closure if ListState is not displayed.

Thank you in advance.

changelog: Adding is_empty flag for prevent panic.

panic reason is closure move in filterd_candidates

if tui::ts PR merged, and component will not need to have offset flag

@see: fdehau/tui-rs#408
@kyoto7250 kyoto7250 changed the title adding offset for prevent panic adding ~~offset~~ is_draw for prevent panic Apr 15, 2022
@kyoto7250 kyoto7250 changed the title adding ~~offset~~ is_draw for prevent panic adding is_draw flag for prevent panic Apr 15, 2022
@kyoto7250 kyoto7250 changed the title adding is_draw flag for prevent panic adding is_empty flag for prevent panic Apr 15, 2022
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.

Press under arrow twice times in filter, and application is paniced
1 participant