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

Make overlays (e.g. pickers) fullscreen #12311

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

salman-farooq-sh
Copy link
Contributor

@salman-farooq-sh salman-farooq-sh commented Dec 21, 2024

I couldn't open a PR before for some reason so I made #11780 but here it is now. Open to feedback. Here's how it looks:

image

Thanks!

helix-term/src/ui/overlay.rs Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@nik-rev
Copy link
Contributor

nik-rev commented Dec 21, 2024

The status line should not be hidden. It contains important information like which file I am currently in

@salman-farooq-sh
Copy link
Contributor Author

Do you need it while a picker is open? What about just closing the picker to look at the status line and opening back the last picker? Picker/preview gets an extra line this way but I can see the value of status line.

@nik-rev
Copy link
Contributor

nik-rev commented Dec 21, 2024

Do you need it while a picker is open? What about just closing the picker to look at the status line and opening back the last picker? Picker/preview gets an extra line this way but I can see the value of status line.

Yes, why would you want to close the picker just to see the file name?

I often glance at the statusline to know where I am currently

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 22, 2024

Yes, why would you want to close the picker just to see the file name?

Because it allows an extra line for previews and/or pickers.

Edit: I admittedly don't look at the status line much so I do have a bias, let's let the maintainers decide.

@TornaxO7
Copy link
Contributor

@TornaxO7 Will you like to add anything further?

peepoMaybe

I think, I personally would prefer Option B but I don't mind C.

@RoloEdits
Copy link
Contributor

I'd go for Option C

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 29, 2024

It probably should not be a "this versus that", but something that:

1. Works for large screens, and the current implementation is great

2. Works for small screens, where current implementation lacks

3. Works for _weird_ screens, like vertical monitors, or horizontal splitted in half/thirds, and the current implementation is also not great for that imo

4. Does not sacrifice important information (debatable if tabs and/or statusline is considered important, but for me personally it is, as I can easily forget which file is open atm :))

First of all, nice roundup.

Few things that I think should be mentioned if not already known: Control + T hides preview. Under a certain width (I think width is the trigger?), previews auto hide too.

1, 2, and 3 are addressed by having this config-able. Or are you saying this should be intelligent dynamic behavior?

4 is also personal opinion. And I think using 2 or 3 lines for status line and tabs is not that big of a problem. The problem is, how to config this? Because status line is always visible and hardcoding some space underneath the overlays is easy, but tabs are dynamic.

@4ndv
Copy link

4ndv commented Dec 30, 2024

1, 2, and 3 are addressed by having this config-able. Or are you saying this should be intelligent dynamic behavior?

Probably dynamic, it's not like anyone would change paddings in config every time terminal window is resized

@darshanCommits
Copy link
Contributor

Probably dynamic, it's not like anyone would change paddings in config every time terminal window is resized

That sounds way beyond the scope of the original issue.

Starting small would actually get the pr merged.

Plus I doubt you'll need to change it that often.

But if ended on that path, something like css clamp() internally calculated on the padding values in user config could workout. This would both keep it simple to the user and have that dynamic behaviour when resizing.

But I think that should be left for future work.

@4ndv
Copy link

4ndv commented Dec 30, 2024

Starting small would actually get the pr merged.

It's not like this PR makes it any easier to do in the future, but will make experience worse on big screens imo

Plus I doubt you'll need to change it that often.

Terminal sizes aren't fixed, so paddings also shouldn't be? Unless it's a configurable ratio, I guess

@salman-farooq-sh
Copy link
Contributor Author

It looks like some form of option C seems to be the popular choice at least in this thread: overlay size shouldn't be a nuclear option.

Option A and B are always effectively achievable with option C, and we will do it such that option Z people won't see any difference unless they explicitly go ahead and add the new config. The debate hasn't been this simple obviously but we have to conclude this eventually.

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 30, 2024

I have been thinking about the tabs / status line visibility issue, maybe individual config for 4-side padding isn't that bad after all?

If any side's padding is config'ed between 0.0 to 1.0 as a float, then the number works as the ratio of full terminal height/width.

If it is given as a positive integer or 0, then it is taken as the number of rows/columns.

If for some or all sides padding is not specified, old defaults are used.

Following config will leave 1 and 2 lines for tabs and status line respectively being fullscreen otherwise:

[editor.picker.padding]
top = 1
right = 0
bottom = 2
left = 0

Old default will be: (Not exactly, as master clips 2 lines from bottom before calculating 90% width/height. This is 90% of full-width/full-height. Edit: mayyybe ratios should be after clipping 2 lines from bottom and 1 from top?)

[editor.picker.padding]
top = 0.05
right = 0.05
bottom = 0.05
left = 0.05

Weird combinations are possible whether it is good or bad:

[editor.picker.padding]
top = 3
right = 0.4
bottom = 0
left = 20

Of course, we can do away with the concept of floats vs. integers and have only one or the other.

This was just an idea, comments appreciated. Problem to discuss is: tabs / status line visibility.

@salman-farooq-sh
Copy link
Contributor Author

Personally, for my workflow only, I am most okay top to bottom with the following:

  1. Fullscreen with no tabs / status line. No config. (current state of this PR)
  2. Fullscreen with 2 line padding hardcoded for status line. No config. (a previous commit in this branch)
  3. Fullscreen with 2 line padding hardcoded for status line and 1 for tabs. No config.
  4. editor.picker.padding.x/y config as ratio.
  5. editor.picker.padding.top/right/bottom/left config as ratio / absolute.
  6. Only 1, 2, and 3 as selectable options.
  7. Responsive / dynamic

Did I miss anything?

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 30, 2024

Picker/preview ratio will be another PR as @darshanCommits suggested.

@salman-farooq-sh
Copy link
Contributor Author

This was just an idea, comments appreciated. Problem to discuss is: tabs / status line visibility.

Ratio can be of full-height / full-height minus status line / full-height minus tabs / full-height minus status line minus tabs based on a separate flag.

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 30, 2024

@the-mikedavis Replying to relevant part of #12369 (comment) here, this PR definitely provides usability improvements.

We can make this a simple toggle between:

  • fullscreen + [picker:preview = 3:5] + [no status line or tabs]
  • master

I am okay with this^ too if that makes the merge fast.

Edit:

I am most okay top to bottom with the following:

  1. Fullscreen with no tabs / status line. No config. (current state of this PR)

@the-mikedavis
Copy link
Member

I'm not interested in adding config for this in any form. Can we instead set small constants that expand the overlay to cover the full width and/or height (minus statusline) when the terminal is small?

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 30, 2024

Can we instead set small constants that expand the overlay to cover the full width and/or height (minus statusline) when the terminal is small?

Yes.

What terminal width/height values should trigger this behavior?

I also want picker/preview ratio to change to 3:5 on this trigger. Will it be okay?

@the-mikedavis

@the-mikedavis
Copy link
Member

The picker/preview ratio part I'm unsure of - some pickers have a few columns and the table can take up quite a bit of horizontal space depending on the current results. #9647 left improvement of the way the columns are laid out as a follow-up and maybe we can improve the way we share space with the preview as well. There's also an idea like #7783 to split the results and preview in a vertical layout that might compete with this.

For the overlay part - what size are you at that you'd like a full screen? stty size for my terminal in my usual layout reports 80 170 which is fairly large and there the overlay fits nicely - I don't usually use a smaller layout.

@univerz
Copy link

univerz commented Dec 30, 2024

80 170 which is fairly large and there the overlay fits nicely

165 196 and yet i think it would be great to use this wasted space to display a larger preview (the days of 80 columns are long gone)

@salman-farooq-sh
Copy link
Contributor Author

The picker/preview ratio part I'm unsure of - some pickers have a few columns and the table can take up quite a bit of horizontal space depending on the current results. #9647 left improvement of the way the columns are laid out as a follow-up and maybe we can improve the way we share space with the preview as well. There's also an idea like #7783 to split the results and preview in a vertical layout that might compete with this.

Very fair points. We can skip changing picker/preview ratio in this PR then.

For the overlay part - what size are you at that you'd like a full screen? stty size for my terminal in my usual layout reports 80 170 which is fairly large and there the overlay fits nicely - I don't usually use a smaller layout.

stty size gives me 57 182 in my usual layout and I don't usually go below it when using helix with overlays. I would like if this (and a little bigger e.g. up to 195 columns) would have fullscreen overlays.

I suppose we can target anything below ~200-250 columns. I am suggesting a higher value because we should go as high as possible until the point made in #12311 (comment) becomes a problem if there isn't anything else stopping us. This can essentially make fullscreen the default for a majority of the people though.

@salman-farooq-sh
Copy link
Contributor Author

salman-farooq-sh commented Dec 30, 2024

(Updated) Switching on 240 columns, previews of how it looks:

image
image
image
image

nik-rev added a commit to nik-rev/helix that referenced this pull request Jan 4, 2025
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests.

Check it out here: https://github.com/NikitaRevenco/patchy
uncenter added a commit to uncenter/helix that referenced this pull request Jan 6, 2025
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests.

Check it out here: https://github.com/NikitaRevenco/patchy
nik-rev added a commit to nik-rev/helix that referenced this pull request Jan 6, 2025
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests.

Check it out here: https://github.com/NikitaRevenco/patchy
nik-rev added a commit to nik-rev/helix that referenced this pull request Jan 7, 2025
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests.

Check it out here: https://github.com/NikitaRevenco/patchy
nik-rev added a commit to nik-rev/helix that referenced this pull request Jan 7, 2025
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests.

Check it out here: https://github.com/NikitaRevenco/patchy
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.