feat: add option to set the position of the preview window

[tessus]

This change adds an option to set the position of the preview window.
The default is bottom, which is the current behavior.

## Sets the position of the preview window.
## possible values: bottom, top
#position = "bottom"

See the forum discussion here.

For this particular TUI change there are no test cases to write. I thought how I could write any, but none of them made any sense.
I tested all variations (compact/invert/preview) manually.

invert = false

invert = true

Checks

• I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
• I have checked that there are no existing pull requests for the same thing

[ellie]

Why are we moving the preview to the top in non-inverted mode? It's been at the bottom ever since it was introduced. I find this change jarring, and am likely not alone there.

I think realistically this needs to be configurable and opt in, because I can pretty much guarantee as soon as this is released we will have unhappy users in the issues. Probably a "preview position" or something would make sense, and leave the default as they have been

[tessus]

I was testing with invert = false a while back, and depending on the result set, the preview size changed. Originally I thought that the preview window size is more "stable" and the size is determined by the longest command of all commands in the history.
But I was mistaken. The height was always calculated from the current result set.

This means that while you are searching the preview window changes size and the search and result window move up and down. I was very distracted by this behavior and thought it was a bug. And apparently somebody else (Adda in the forum) also thought it was strange and described it as a flickering of the screen.

Just to clarify, this has nothing to do with my change I introduced a while back to only show a preview when the command is longer than the terminal width. When using this new option it only makes this behavior stand out even more. I have been verifying my findings by testing with an atuin version (897af9a) before my PR was merged.

Here is a video of what I mean (also taken from atuin 897af9a):

jump.mp4

However, I also understand that people might be shocked and annoyed by this change. I am more than happy to add a setting to make the position configurable.
It might just take a bit, because the TUI code is not always easy to handle.
But I am ecstatic that you are ok with an option.

[tessus]

I've updated the title and description of this PR.

Unfortunately the code is not as pretty as I hoped. The TUI components are a pain and there are not many options to make it more succinct.

I set one #[allow(clippy::if_same_then_else)] to make that part easier to read. If you don't mind a less legible code, I can get rid of 2 if branches (16 lines). I am more than happy to make that change. Please advise.

[tessus]

@ellie this one is ready for another review and also I'm more than happy to change it as mentioned in my previous comment.

[ellie]

I appreciate you trying to solve this, but given that it's not a particularly high priority issue + that the code here would reduce maintainability of the TUI, I'd rather not proceed.

As I've said in previous PRs, even breaking things down a bit to be more legible and testable can be really helpful. Otherwise, Ratatui allows the creation of custom widgets, like our history list

[tessus]

Understood, although I want to mention a few things:

The maintainability of the TUI code is already bad. When writing this PR, it was hard to understand what the code was actually doing due to the lack of comments. Oh, and using .title(format!("{:─>width$}", "", width = style.inner_width - 2)) to create separator lines... yep, it took me a while, probably because I am not a ratatui expert.

My change makes the code longer (4 instead of 2 branches), but I'd argue that the maintainability hasn't changed, since the flow/logic is consistent and no async/channel/trait constructs have been introduced. Also, one can always add more comments.

Testing TUI code is always a challenge, otherwise somebody would have done it already. The only tests in this file are the ones that I wrote.

I really understand and accept that you don't want to merge PRs you don't agree with, but the current behavior of the TUI is inconsistent. The default behavior after this PR doesn't change that, but at least people can choose where the preview window is.
It's not that I want my code to be merged. There are way better Rust programmers out there. But I think for consistency and usability, the option to set the position of the preview window is an important feature. I hope that something like this will be implemented in the future.

[ellie]

The maintainability of the TUI code is already bad.

You're absolutely correct! That's on me. Previously, I've been too hasty to merge work that focuses too much on new features and too little on improving the health of the codebase. Both my own and that of contributors.

Contributions should aim to improve the situation, not compound it. Especially when you've contributed a few times and been around for a while, we should try to enhance maintainability and readability.

Oh, and using .title(format!("{:─>width$}", "", width = style.inner_width - 2)) to create separator lines... yep, it took me a while, probably because I am not a ratatui expert.

In that case, perhaps you could have added a comment explaining it? Or perhaps refactor it to make a bit more sense? Nit picking at the current state of things isn't particularly constructive, and in this case just feels like a bad reaction to my critique and feedback on your work.

but I'd argue that the maintainability hasn't changed

Your implementation significantly increases code complexity, requiring #[allow(clippy::cognitive_complexity)]. I think drawing the line there totally makes sense.

Clippy even highlights that as applying to "functions that should be split up into multiple functions", which is feedback I've given you about previous contributions.

The only tests in this file are the ones that I wrote.

I appreciate it! I'd also like to note that those tests weren't there until you were pushed for them in a previous review cycle.

I really understand and accept that you don't want to merge PRs you don't agree with

I don't have any issues with the feature, but I do have issues with the implementation. I don't have the time or energy to go through another review cycle where I ask you to improve the quality of the code and have to spell out precisely how.

The default behavior after this PR doesn't change that, but at least people can choose where the preview window is.

I don't think this feature is high enough priority, given the lack of many requests from other users and the issues I've mentioned previously.

[tessus]

In that case, perhaps you could have added a comment explaining it? Or perhaps refactor it to make a bit more sense?

You are correct, I should have added a comment. I would have liked to refactor it, but I am not a ratatui person. I wouldn't know how.

Your implementation significantly increases code complexity, requiring #[allow(clippy::cognitive_complexity)]. I think drawing the line there totally makes sense.

This I do not understand. Drawing the line I underatand. It is fine and I accept that. What I don't understand is why clippy throws that warning, since the complexity is not really increased. I was in DB2 development. Having nested structs (up to 12 levels) used with thread synchronization and latching is complex. Adding 2 more branches to a linear workflow is not.

I don't have any issues with the feature, but I do have issues with the implementation. I don't have the time or energy to go through another review cycle where I ask you to improve the quality of the code and have to spell out precisely how.

You must have noticed by now that I am not a Rust programmer. I've never learned how to write code in Rust. I understand the basic concepts, but that is all. I wish I had more time to learn Rust properly.
But I do understand that you don't want to mentor and don't have the time to do so.

I can only say that I would work on a better implementation, if I knew how.

This leaves me at hoping for someone to implement this properly. Who knows, maybe someone will.