fix: Rewrite the insert_before method on Terminal to fix a bug and remove the height cap

[danny-burrows]

This PR fixes some issues encountered in #584 by way of rewriting the insert_before method.

The function signature for reference:

pub fn insert_before<F>(&mut self, height: u16, draw_fn: F) -> io::Result<()>

Ultimately it does two things:

1. It fixes a bug triggered by the height parameter of a call to insert_before being greater than self.viewport_area.height - This bug would cause an extra newline to be inserted leaving a gap in the output.
2. Removes the height cap by rendering output in chunks. if height is too large to fit on the screen (with the viewport) then chunks are rendered as the screen is scrolled.

A Note About Test Coverage

There are currently no tests for the insert_before method; I've attempted to test this change with the TestBackend but unfortunately due to it not supporting clear ClearType::AfterCursor this would prove difficult and I think the noise of adding this functionality wouldn't be worth adding to this PR. As it's a very minimal hit I wondered if it's acceptable?

Update: I've now added some test cases for the issues encountered in the tests/ folder. I know this is now discouraged but there was already a pattern of using the TestBackend in this location, but I'd be open to moving them if requested. I've also added the clear_region and append_lines methods to the TestBackend with accompanying tests.

[codecov]

Codecov Report

Merging #596 (4b0bc23) into main (a2f2bd5) will increase coverage by 0.91%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
+ Coverage   89.56%   90.47%   +0.91%     
==========================================
  Files          41       41              
  Lines       11571    11890     +319     
==========================================
+ Hits        10363    10758     +395     
+ Misses       1208     1132      -76     

Files	Coverage Δ
src/backend.rs	71.42% <ø> (-9.53%)	⬇️
src/backend/test.rs	97.79% <100.00%> (+2.94%)	⬆️
src/terminal.rs	88.60% <100.00%> (+30.80%)	⬆️

[joshka]

There are currently no tests for the insert_before method; I've attempted to test this change with the TestBackend but unfortunately due to it not supporting clear ClearType::AfterCursor this would prove difficult and I think the noise of adding this functionality wouldn't be worth adding to this PR. As it's a very minimal hit I wondered if it's acceptable?

I think it's probably worth adding the clear ops to the TestBackend to help with this change. There are three main reasons I like to add test coverage to existing code before changing it:

1. When the existing code is complex or just not well understood, tests help document the existing behavior (particularly the edge cases)
2. When the existing code is changed, the tests help document how the behavior changes from the perspective of the consuming code
3. When the existing code is tested, any new implementation can be fearlessly refactored for simplicity and readability while maintaining correctness of implementation

These reasons each make changes easier to review as otherwise a reviewer has to determine whether the code did the right thing originally by fully understanding the code, and whether it now still does the right thing by understanding the new code. Tests like this also help capture if there are breaking changes that other users need to know about.

Here the code is not too complex, but I don't fully understand it. It currently does a particular thing and I'm not sure I know why it does that enough to understand whether the change is right.

My approach generally would be to write characterization tests that fully cover the problem code, commit them, then make the changes and see which tests break. How about adding the missing TestBackend implementations for clear that would show how this works and then adding the tests for this? Do you think this sound reasonable?

[danny-burrows]

That's reasonable yes, I'll work on adding that functionality to the TestBackend and some tests to showcase the issues.

[danny-burrows]

I've added the functionality we discussed above, the clear_region and append_lines methods. I've also added some tests for the insert_before method. I hope this helps! Things do look a lot cleaner now having these tests, thanks for the suggestion! :)

[kdheepak]

Slightly related, maybe we should set a threshold in codecov.yml?

https://docs.codecov.com/docs/commit-status#threshold

[danny-burrows]

Slightly related, maybe we should set a threshold in codecov.yml?

https://docs.codecov.com/docs/commit-status#threshold

I'd be happy to add one if you think it makes sense in this PR, do you have an acceptable value in mind? Alternatively, if this requires further discussion maybe it would be better suited to its own Issue/PR?

[joshka]

The overarching context of my review is that I'm not as familiar with the workings of this code as you are, so I'm looking for help with the changes and tests to make it more clear that the changes are correct. They might mean that the feedback is a bit pedantic, and for that effect I apologize in advance. But I'd encourage you to think about this change with the perspective of "how would I explain these changes to someone that is reading it for the first time?" (because that's not far from the truth)

[joshka]

I'd be happy to add one if you think it makes sense in this PR, do you have an acceptable value in mind? Alternatively, if this requires further discussion maybe it would be better suited to its own Issue/PR?

Another PR would be right for this #612

[joshka]

Thanks for making the changes - I understand TestBackend::append_lines() much better now. Lots of detailed feedback again that I hope will make this better, and I think there's a couple of possible bugs that I'm not sure of.

[joshka]

I've merged this - thanks for sticking with it to get it over the line. It should be available in the alpha release this Saturday.