Use current preview command length for preview height

[happenslol]

I noticed that my result window height would randomly resize while typing, and I narrowed it down to the calculated preview height. Currently, it searches the results for the longest command, calculates the lines that would fill, and then uses that. That means that if you start typing and a result that fills up 4 lines shows up somewhere in the results, the preview will take up 4 lines even though it's currently displaying one line.

I changed this to always take the line height of the currently previewed command, which works very well for me. I think eventually I would want to separately be able to set max preview height and number of results (and have the global height limit be optional), but that's a different topic 😛

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atuin	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 5:44pm
atuin-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 5:44pm

[ellie]

I'm not sure how I feel abotu this one - it would mean that the UI would jump around a bunch if you scroll through the results, rather than staying consistently one size. Would you be able to elaborate a bit about why you find this preferable?

[happenslol]

Hey, thanks for your response! Sure, I can give you an explanation of how I came to implement this and why I think it makes sense.

I'm only using the inline version of atuin. Here's the relevant config:

style = "compact"
invert = true
show_preview = true
show_help = false
inline_height = 10

Let's say I execute a really long command, and it gets saved in my atuin history. In my case, I sometimes paste long base64-encoded values and decode them using base64 -d. Multiline commands have the same effect, though. My history now contains an entry that takes up 4 lines. Now, I start typing, and as expected, atuin has 10 lines (8 results + header + footer):

As soon as I type the next character, the fuzzy matcher matches some string in the long base64 data I extracted. The preview now takes up 4 lines, which means I only see 5 results:

I would have to scroll down 100 entries to actually get to the one entry that actually takes up 4 lines in the preview, but as soon as that result enters the matches, I can only ever see 5 results.

Additionally, since items of varying length enter the matches list while I type, that means that the amount of results is constantly changing, but not due to the actual amount of results (which is what I initially thought was happening before I started debugging this), but due to the fact that a multiline result might have entered the matches (or left it again).

I understand that the tradeoff here is that the amount of results has to change at some point - either while typing, or while scrolling through the list. My reason for advocating for this option is that the alternative is non-obvious, from my point of view. If we resize the results list while typing, it looks like the actual amount of results is increasing or decreasing, since the preview might still only be one line high. If we resize while scrolling and the currently selected entry is multiline, it's obvious since the whole thing will be shown in the preview, and there are no empty lines.

Sorry for the lengthy explanation, I hope this clears it up. Please do ask if anything was unclear!

[happenslol]

Any updates on this? Or could you tell me what's preventing you from maybe just adding this as an option?

[ellie]

I'm really sorry for being slow to review this, but PRs that

• are mostly from personal preference without a clear issue to solve/resolve
• are adding or changing a feature that hasn't been discussed prior
• require me to test on a bunch of platforms and setups
• are non-optional and could break a bunch of other users preference

Are really difficult for me to prioritise reviewing, given the volume of work that's required to maintain this. Soon I'll be able to dedicate much more time here, but if you were to gate this behind an option + document it I'd be far more likely to review it soon.

Apologies again, I know it sucks to have an open PR for so long

[happenslol]

I completely understand, no apology needed on your part. I'll add an option for this as well as documentation if that helps.

Would you prefer having a toggle for this that is off by default (e. g. use_selected_command_for_preview_height) or an enum that could be expanded in the future (e. g. preview_height_strategy = "selected_command"/"all_results")?

[ellie]

I completely understand, no apology needed on your part. I'll add an option for this as well as documentation if that helps.

Thank you!

Would you prefer having a toggle for this that is off by default (e. g. use_selected_command_for_preview_height) or an enum that could be expanded in the future (e. g. preview_height_strategy = "selected_command"/"all_results")?

I'd prefer the enum please! It would also be great to make this a child struct of the settings object, so that the config in the end would look like

[preview]
height_strategy = "all_results/selected_command"

We're getting to the point where there's a fairly large number of flags, so this is the approach I'd like to start building in. Thank you!

[happenslol]

Done as asked. Please tell me if I need to update docs anywhere, or if you require any other changes.

[tessus]

Hmm, I totally missed this PR. It seems that I implemented something similar in #1804, but I think I like the enum and your height_strategy setting better than what I used.
My feature wasn't included in a release yet, so changing it shouldn't be an issue.

If @ellie is ok with it, maybe you can just add your enum and a restructuring of the settings to a new PR (or I can also do this).

e.g.:

[preview]
show = false                                           # was show_preview
strategy = auto (default) | static (max_of_all_cmds)   # was show_preview_auto
max_height = 4                                         # was max_preview_height

static would be the current behavior when show_preview is set to true (w/o my show_preview_auto code, or your all_results option). But I think auto/static are nicer options for the strategy.

The only question is how do we make changes to the settings seamless (and without breaking current users' configs). This is something that hasn't been decided yet, but would be required for #1789 and #1793 as well.

[happenslol]

Hey, I'm quite busy (and have been for a while), so I don't know if I can put any more work into this. The original behavior (and current behavior on main) seemed like a bug to me, since I've had many cases where I would only get shown one result even though I had the results height set to more than that. Maybe I didn't adequately communicate the issue, or it's just that not many other people are bothered by it.

I've been running this patch in my fork ever since I submitted this PR, and at the moment it's less work for me to occasionally update my fork than to try to reconcile the issues you mentioned.

I'm sorry about that and you're free to use any of the changes made here. I'll resolve any conflicts the next time I update my fork, but that will probably introduce breaking changes, so it might not do this PR any good.

[tessus]

Thanks for the reply.

I guess I'll wait for @ellie's input. But I'd like to use an enum and add at least the following as a follow-up to #1804:

[preview]
strategy = auto (default) | static (max_of_all_cmds)   # was show_preview_auto

[happenslol]

No problem. If you tell me exactly what you'd like changed and we have the OK to get that version merged, I'll see if I can find the time, but I can't make any promises.

[tessus]

No, don't worry. I can easily do this myself. I'll just create a new PR.

[tessus]

I think this PR can be closed.