TUI: Fix line heights on /resize
#356
Conversation
There was a problem hiding this comment.
This obviously fixes the panic as there isn't an unwrap anymore, but I'm not sure that this is the right fix. Right after /clear we know that the total height of rendered lines is 0. So we could reset total_height as Some(0) in clear, which is probably something we should do regardless of this bug, but it should also fix the bug.
I'm also a bit confused about the lines_height field. It seems like if I call resize twice without a draw in between that would also crash with the unwrap, right? I think it would be better (avoid crashes in all cases) if we have an accessor function for lines_height that calculates it when needed, effectively making it a cached value like the other cached values we have in e.g. messaging area lines. When it's None, we calculate the value, and return it. Otherwise return the existing value. No unwrap needed. WDYT?
| let ratio = (self.scroll as f32 + old_height as f32) / old_total_lines as f32; | ||
| let total_lines = self.update_total_visible_lines(); | ||
| self.scroll = max(0, ((ratio * total_lines as f32) as i32) - self.height); | ||
| fn recalculate_scroll(&mut self, old_height: i32, old_total_lines: Option<i32>) { |
There was a problem hiding this comment.
This function cannot handle None case, so if you keep the type as before it will become evident from the type that this really needs a old_total_lines argument. Please revert this and call this function only when old_total_line is available. It will be less surprising when you look at this code in a few months.
There was a problem hiding this comment.
So we could reset total_height as Some(0) in clear, which is probably something we should do regardless of this bug, but it should also fix the bug.
This is right. Sorry
When you /clear the screen the total lines in the MsgArea gets cleared and set to None. Then if you resize after that, we were unwrapping total lines to recalculate the scroll amount and panicking. This fix sets the lines_height to Some(0) on /clear Fixes osa1#355
trevarj
force-pushed
the
recalc-scroll-unwrap
branch
from
0bce195
to
010fcfe
Compare
| self.recalculate_scroll(old_height, old_total_lines.unwrap()); | ||
| if let Some(old_total_lines) = old_total_lines { | ||
| self.recalculate_scroll(old_height, old_total_lines); | ||
| } |
There was a problem hiding this comment.
Let's also revert this. As mentioned in my other comment, this code is currently buggy and removing the unwrap will hide the bug. We should fix this in another PR.
|
Thanks @trevarj . Did you check that this fixes the issue? I thought it should fix it, but haven't tested it myself. Would be good to add a test also, if possible. |
|
@osa1 yeah, tested as follows:
Added a similar unit test |
|
Thanks @trevarj . The test does not test the bug though. The idea is that if you revert the fix the test should fail, and with the fix it should pass. That's how you make sure it tests the bug you fixed. If I revert the fix in this PR the test still passes. Looking at the test, I think it makes sense and I would expect it to fail without the fix. I don't understand why it passes. Maybe we misunderstand the issue? |
|
Ah, sorry, I was running a wrong test. OK this looks good. Will merge soon. |
osa1
changed the title
/resize
When you /clear the screen the total lines in the MsgArea gets cleared
and set to None. Then if you resize after that, we were unwrapping total
lines to recalculate the scroll amount and panicking.
We know the total line height is 0 after /clear so we now set total line
heights
Some(0).Fixes #355