Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tessus
Copy link
Contributor

@tessus tessus commented May 31, 2024

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

image

invert = true

image

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

This change will move the preview to the top, so that the search box and results
are not shifted around.
If `invert` is true, the preview is at the bottom (current behavior).
@ellie
Copy link
Member

ellie commented May 31, 2024

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
Copy link
Contributor Author

tessus commented May 31, 2024

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 tessus changed the title feat: make the position of the preview conditional feat: add option to set the position of the preview window Jun 8, 2024
@tessus
Copy link
Contributor Author

tessus commented Jun 8, 2024

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
Copy link
Contributor Author

tessus commented Jun 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants