[Bug] Search matches are lost after resizing the terminal

[playwmadness]

Steps to reproduce

1. Search for something that exists in the file (e.g. /you in README.md of this repo).
2. Resize the terminal window.
3. All highlighted search matches are lost, n/N keys don't do anything.

• Version: 72e7345 (current latest commit in master)
• Terminal: kitty 0.44.0

[playwmadness]

It seems scroll search into view also breaks after resize if the new height is less than the old one.

[playwmadness]

I think the current interface around Model::add_searches isn't very good. It takes a String, but doesn't actually make any use of ownership. A simple &str should be enough.
But since it's always called with a clone of model.cursor.<search state>.needle, if we try to avoid cloning, we have to borrow mut Model as self and borrow search state needle at the same time, which is illegal, so we have to mem::take.

If I naively replace Option<String> with Option<&str> and get rid of the unnecessary needle.clone() in main.rs, then it becomes a bit verbose (also bug-prone -- you might forget to reinit the needle):

mode.needle.pop();
let needle = mode.needle.clone();
model.add_searches(Some(needle));

becomes

let mut needle = std::mem::take(&mut mode.needle);
needle.pop();
model.add_searches(Some(&needle));
let Cursor::Search(mode, _) = &mut model.cursor else {
    unreachable!("model.add_searches should not modify cursor");
};
mode.needle = needle;

Maybe Model itself should be responsible for adding/removing chars to/from its search needle.

[playwmadness]

I stumbled upon this while adding add_searches to the resize handler.

[benjajaja]

I find that section awkward too. I would like keyboard event handler to map keys to operations on model. That is, I would avoid pushing too much "key event handler" logic into the model, in any case there should be a clear isolated layer in the future where custom keybinds and such could happen.

But then this part is awkward, because depending on the model's (search-) mode, we need to handle the keypress differently. And it seemed to make sense to have access to mode/SearchState in the event handler. Maybe the event handler should just call a method is_in_seach_text_entering_mode() (but better naming), and then call the model to edit the search-text/needle, and we would just have to deal with either matching on model.cursor again inside the methods, or something like that.

[playwmadness]

I feel like the interactive part could be handled entirely on the view logic side, and transfer needle ownership only once we commit search (hitting Enter). And this way it should also work with add_searches taking a &str slice, instead of an owned String.

[benjajaja]

I want to more or less follow The Elm Architecture, keeping the view pure. The sacrifice with TEA is that it's hard to make "components" like I think you are describing.

I'll add some overall "architecture" docs when I'm done with the tree-sitter stuff!

[playwmadness]

Check this wip c8601df.
I made it as a stateful layer with handlers which translate input (just key presses for now) into model calls.
Currently it's owned by the model, though I think it could be its own thing.

[benjajaja]

@playwmadness so that commit is an alternative or extension of your other PR?

[playwmadness]

I built it on top of that PR to see how these features would interact with each other.
But I didn't add it to that PR because it looks to me like a separate almost unrelated change.

[benjajaja]

Thank you for doing that! I'll review it soon, I have some doubts about using traits for modes, could maybe just be an enum. Perhaps we just need to put mode + command-queue into the model.