-
Notifications
You must be signed in to change notification settings - Fork 990
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
fix(TUI): Fixed panic when changing order of items in TableView #3160
fix(TUI): Fixed panic when changing order of items in TableView #3160
Conversation
…e the number of items in the table is reduced
Nice. |
@@ -1,6 +1,6 @@ @@ -32,14 +32,14 @@ term = "0.5"
-grin_keychain = { path = "./keychain", version = "3.0.0-alpha.1" } [target.'cfg(windows)'.dependencies] @@ -53,5 +53,5 @@ cursive = "0.12"
-grin_chain = { path = "./chain", version = "3.0.0-alpha.1" } |
I've definitely encountered this before - and always failed to investigate it... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -616,7 +616,7 @@ impl<T: TableViewItem<H>, H: Eq + Hash + Copy + Clone + 'static> TableView<T, H> | |||
}); | |||
self.rows_to_items = rows_to_items; | |||
|
|||
self.set_selected_item(old_item); | |||
old_item.map(|o| self.set_selected_item(o)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's really nice - just using map()
rather than an if let Some(x) = old_item { ... }
.
@@ -492,7 +492,7 @@ impl<T: TableViewItem<H>, H: Eq + Hash + Copy + Clone + 'static> TableView<T, H> | |||
/// Returns the index of the currently selected item within the underlying | |||
/// storage vector. | |||
pub fn item(&self) -> Option<usize> { | |||
if self.items.is_empty() { | |||
if self.items.is_empty() || self.focus > self.rows_to_items.len() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this actually be >=
?
I just happened to see the following panic locally -
thread 'main' panicked at 'index out of bounds: the len is 15 but the index is 15': /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/slice/mod.rs:2717stack backtrace:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right I think it should. I can do a fix for that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its just this one line change I think we can get it in for beta.2
.
👍
This is awkward to reproduce but can be done reliably if you get the timing right.
thread 'main' panicked at 'index out of bounds: the len is 8 but the index is 14':
...
6: grin::tui::table::TableView<T,H>::sort_by::haee3b0fe6388d824 (0x55986b9dd817)
7: grin::tui::table::TableView<T,H>::set_items::h6712930b43f95168 (0x55986b9de079)
Have changed it so that sort function does not try to set_selected_item if it is outside the range of the new peers list and the panic no longer occurs.