tiny icon indicating copy to clipboard operation
tiny copied to clipboard

Input area improvements/tasks

Open osa1 opened this issue 5 years ago • 5 comments

I was reading the code today and found some potential improvements and a problem in the code.

  • [x] Starting with the problem: InputArea::height is never set. It starts its life as None and stays that way. That means InputArea::get_height always recomputes the height even if nothing changes in InputArea.

  • [x] I think it'd be better to invalidate InputLine::ws_indices (renamed from InputLine::line_starts) on update. Currently we don't update it until calculate_height is called, and if I update a line and forget to call calculate_height then draw will render input area incorrectly.

  • [x] We should have a fast path for the common case of inserting at the end. ~~Currently this causes quadratic behavior on paste, where we turn a long string into a series of key press events (see handle_input_event), and for every key press we compute the whitespace indices from scratch.~~ (this comments was incorrect, see my update in the comments section below) I think InputLine::insert could be easily updated to handle this and efficiently update ws_indices when inserting at the end.

    This also means InputLine::calculate_height shouldn't generate ws_indices from scratch if it's already up-to-date (related to the second point above), otherwise the update in insert will never be used.

  • [ ] Similar to the item above, deleting from the end shouldn't require generating the state from scratch.

cc @trevarj

osa1 avatar Jun 12 '20 06:06 osa1

Thanks for taking a look and fixing it up. I read through your changes and they were really helpful for my learning.

trevarj avatar Jun 12 '20 07:06 trevarj

I edited my original post: pasting causes "quadratic" behavior, not "exponential"!

osa1 avatar Jun 12 '20 07:06 osa1

  1. This first issue seems like it may be as as simple as putting self.height = Some(line_count); at the end of InputArea::calculate_lines(). I tested it a little, but not much.

  2. Invalidating ws_indices seems like a good idea. When would you update and forget to call calculate height though? Does this just mean if the code were to change?

  3. Should we also re-write the paste functionality to do a memcpy into the line buffer instead, then invalidate ws_indices and InputArea::height?

trevarj avatar Jun 12 '20 20:06 trevarj

Copying my comment from #210:

Btw, I realized that my remark about quadratic behavior on paste in #208 is incorrect: we call TUI::keypressed for every char in the pasted string, but keypressed doesn't draw after handling the key. So we call draw just once after handling all the keypresses. Sorry for the false alarm!

osa1 avatar Jun 17 '20 10:06 osa1

#210 fixed the issues here, but I just added one more relevant item which is for efficiently removing from the end.

osa1 avatar Jun 20 '20 13:06 osa1