allow stacking the picker preview vertically #7783
Conversation
There was a problem hiding this comment.
With your changes the picker dissapears fi the window gets too small since the else branch is only executed if there is no preview (but not anymore if there is not enough space) I added suggestions how to fix that.
I like this in general its pretty nice to have tall screens and the picker wasn't taking much advantage of those 👍 I think the cetoffs could be further tweaked tough
helix-term/src/ui/picker.rs
Outdated
| if self.show_preview { | ||
| if area.width > MIN_AREA_WIDTH_FOR_PREVIEW { |
There was a problem hiding this comment.
| if self.show_preview { | |
| if area.width > MIN_AREA_WIDTH_FOR_PREVIEW { | |
| if self.show_preview && area.width > MIN_AREA_WIDTH_FOR_PREVIEW { |
helix-term/src/ui/picker.rs
Outdated
|
|
||
| self.render_picker(area.with_width(picker_width), surface, cx); | ||
| self.render_preview(area.clip_left(picker_width), surface, cx); | ||
| } else if area.height > MIN_AREA_HEIGHT_FOR_PREVIEW { |
There was a problem hiding this comment.
| } else if area.height > MIN_AREA_HEIGHT_FOR_PREVIEW { | |
| } else if self.show_preview && area.height > MIN_AREA_HEIGHT_FOR_PREVIEW { |
|
|
||
| self.render_picker(area.with_height(picker_height), surface, cx); | ||
| self.render_preview(area.clip_top(picker_height), surface, cx); | ||
| } |
helix-term/src/ui/picker.rs
Outdated
| pub const MIN_AREA_WIDTH_FOR_PREVIEW: u16 = 72; | ||
| pub const MIN_AREA_HEIGHT_FOR_PREVIEW: u16 = 18; |
There was a problem hiding this comment.
I think the a simple cutoff like this isn't quite ideal. For example:
This looks very readable:
but making the picker slightly wider switches to a critical split which feels worse to me:
I think it might make more sense to have 2 different thresholds here. For wide windows (lets say more than 120 wide) we always show the current layout, if that's not the case and we are higher that a fairly large height (lets say 40) always shows the vertical layout and if that's not true you try the current fallback. I think we would get slightly nicer layouts that way (there could also be other implementation that work better but its what came to mind)
pascalkuthe
added
C-enhancement
|
i tried something a bit different - instead of multiple thresholds, i gave it a target ratio of width to height to try to maintain, and let it pick an orientation based on which one got closer to that ratio. i think this seems to work more smoothly with a wide range of terminal sizes, but let me know if you see anything else that seems obviously bad. i also added a max width and height, because it seemed more useful to display more of the file picker for extra wide or extra high sizes than to always devote half of the screen to the preview, but i'm not sure exactly what those values make the most sense to be. |
| @@ -40,7 +40,10 @@ use helix_view::{ | |||
|
|
|||
| use super::{menu::Item, overlay::Overlay}; | |||
|
|
|||
| pub const MIN_AREA_WIDTH_FOR_PREVIEW: u16 = 72; | |||
| pub const MIN_AREA_WIDTH_FOR_PREVIEW: u16 = 40; | |||
| pub const MIN_AREA_HEIGHT_FOR_PREVIEW: u16 = 9; | |||
There was a problem hiding this comment.
9 seems a little low, I don't think we should not below 15 here
There was a problem hiding this comment.
one of the main goals here is to have it able to show the preview in an 80x24 terminal. would it be better to drop the min width down to 36? (it's worth noting that the interpretation of these constants is different in the pr than previously - the existing code tests against the full width/height, but in this pr it is testing against the width that would be used by just the preview)
There was a problem hiding this comment.
ah I see I don't have time to test this out at the moment and din\t look at the new code in detail zet so I thaught it had the same meaning as before. I think the old bound was fine so if the definition just changed then its probably ok, I will take a look at that one I have more time.
I just meanr rhar 9 rows for picker and preview would have been a little low (that would have been like 1 or two matches shown at most)
pascalkuthe
added
S-waiting-on-review
doy
force-pushed
the
vertical-picker
branch
from
4c60c58
to
0a17d5a
Compare
a lot of pickers don't actually make sense without the preview (global search, for instance), so forcibly hiding the preview on 80x24 terminal windows (the default in most situations) is not ideal.
|
I need to test this out, but just wanted to let you know that I am still interested in this but waiting until #9647 lands with any changes to the picker. |
a lot of pickers don't actually make sense without the preview (global search, for instance), so forcibly hiding the preview on 80x24 terminal windows (the default in most situations) is not ideal.