Refactored line.rs height calculation and drawing. #216
Conversation
osa1
left a comment
There was a problem hiding this comment.
Thanks @trevarj . It makes sense to reuse existing code, but this uses more memory than the current representation, as String is much more compact than Vec<char>.
To see this, rebase your branch first, then run the bench example on a large file. If I run it on a 134,224 line file I get 99.4 Mb max residency with master branch. With this branch I get 151.9 Mb. That's 52.7% increase in memory.
(This is less that what I expected, actually, because the file only has ASCII characters which are 1 byte in UTF-8 encoding vs. 4 bytes in char, so I'd expect almost 4x increase. No idea why this isn't the case.)
|
Hmm.. I wonder if we could get rid of the |
Ok, I will do this. |
|
@osa1 I used a smaller file than you, but I actually saw a better result with this new change than what's on master. I did verify with bench.rs that the previous commit was inefficient . Let me know if you can send me that massive text file, or try it out on your PC. |
osa1
left a comment
There was a problem hiding this comment.
Thanks @trevarj, this is slightly better in memory and also in runtime now. Code looks good too.
I'm reading the diff now. If next time you also give a summary of changes (both in behavior and in code) that'd make it easier to review and I could respond faster.
Added some inline comments.
libtiny_tui/src/msg_area/line.rs
Outdated
| let mut line = Line::new(); | ||
| line.add_text("abcde"); | ||
| for i in 0..6 { | ||
| for i in 6..0 { |
There was a problem hiding this comment.
The original test doesn't meet the requirement. Now that I look closer at it, this one doesn't either because it's caching the first value. I think it's ok to remove it.
There was a problem hiding this comment.
The long, spaceless lines no longer get cut off.
Before it took "abcde" and rendered it cut off.
Ex.
When i/width is 1, the string renders as "a" and the line count is 1.
width = 2 -> "ab", still 1 line
width = 3 -> "abc"...
My tests - test_calculate_height()- in input_line.rs cover cases like this.
Reused LineDataCache. Closes osa1#202 Don't draw whitespaces when we split on them Changed back to using String for Line's data PR fixes
|
Rebased with master. |
Reused LineDataCache. Basically a drop-in fix.
I did have to manually go through each of the failing tests, because they weren't accurate to how text should get rendered.
Closes #202