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

Add support for stretching tab bar to fill width of tab bar #4403

Closed
wants to merge 9 commits into from

Conversation

dberlin
Copy link
Contributor

@dberlin dberlin commented Oct 6, 2023

This PR is meant to resolve issue #1914

It implements support for filling letting tabs expand to fill the title bar.

At least, about as well as was obvious to me how to do :)

The main issues with the current implementation are:

  1. When computing layout, most people would expect (IMHO) the real tabs to take up all available space except for the width of the left and right status items.
    Unfortunately, it was not obvious to me how to do this without computing layout twice.

This is exacerbated by the fact that we give the max width to all of the computation functions, and they often try to do reasonable truncation (see #2).

Additionally, as far as the window is current, a right status item always exists (AFAICT), so the max title fill of a single tab is really half the space.

The best i think makes sense at the moment (not implemented), unless i'm missing something, would be to add options for max width of left/right status items,
and use them to account for the items without wasting a full tab width on them.

If there is a way to compute the layout of the right/left status items first, you could get it exactly right.

(This was fixed later)

  1. The max width to give to the format-tab-title function is computed twice, and we give a max_width.
    On the second pass, the (previous to this patch) semantic was to pass in first_pass_title_len.min(max_tab_width)
    The problem is that this semantic is, IMHO, broken.

Lots of format-tab-title functions truncate, especially if they involve the current working directory.
They then also leave a little space at the end.
Or left truncate and then right truncate.

So something like

  if tab.active_pane.current_working_dir then
  local path_string = tab.active_pane.current_working_dir.path
  local current_width = wezterm.column_width(title_with_icon)
  local working_dir_width = wezterm.column_width(path_string)
  title_with_icon = title_with_icon .. ":" .. wezterm.truncate_left(path_string, max_width - current_width - 10)
  end
  if wezterm.truncate_right(title_with_icon, max_width - 6) ~= title_with_icon then
      title = " " .. wezterm.truncate_right(title_with_icon, max_width - 8) .. " .."
  else
      title = " " .. title_with_icon .. " "
  end

When the title runs over during the first pass, we will return a title that is smaller than it needs to be (the - 8).
The second pass will now hand us the old title width instead of the actual max amount we could expand to, because that's the min of (first_pass_title_len, max_tab_width)
We will now truncate even further, despite having plenty of space left in our tab title.

I have changed this second pass to instead use max(title_len, max_tab_width). This will allow functions like the above to work.

Looking at github, most functions act like the above.
On top of this, if you don't do this, there is no way to truncate to anything <= max_width without it giving you a smaller size during pass 2.
This change seemed the better option.

@dberlin dberlin force-pushed the tab-fill branch 2 times, most recently from 3e3f2dc to 81ace83 Compare October 7, 2023 06:23
@dberlin
Copy link
Contributor Author

dberlin commented Oct 7, 2023

I have modified the implementation to implement fill directly in the box model.

We recompute the width of the fill items as a group, after calculating the available non-static space.

The effect is much nicer - tab titles grow to the size needed, up to <width/number of tabs> size.

This also lets us avoid trying to pre-calculate the layout max width in the fancy tab bar code itself.

It's still possible to overrun the status item if you try real hard. I think it is becuase of the order of computation and floating order.

Honestly, i'm likely to take a whack at converting the box model to use cassowary constraints after this patch. I suspect it would be easier to get correct, and faster (because it can incrementally update very quickly for things like window resizes, single item changes, etc)

@dberlin
Copy link
Contributor Author

dberlin commented Oct 7, 2023

I fixed having it run into the right status by doing the following:

  1. We ensure the right elements and left elements of the tab bar end up in the same children list.
    Before, they were in different children elements with floats set on the child container.
    Float is now directly set on them

  2. The floating algorithm did not seem to work right - it did not float things all the way to the right edge even when it could. This is now fixed by all float right elements being laid out right to left from the right edge.

  3. We process the filling after the floating, and compute the available space as the space between the leftmost starting point, and the first float right element, minus all non-filled elements.

I have a branch that converts this into constraints that i'm using to test against.

@jacksongoode
Copy link

This looks good from my build!

@dberlin
Copy link
Contributor Author

dberlin commented Oct 9, 2023

If you want to see a complicated tab formatting setup so you can see how it fills or doesn't, here's one:
https://gist.github.com/dberlin/1b3bed90e414d1339cb870f16562997b

@zmc
Copy link

zmc commented Dec 2, 2023

Hi, I discovered this after partially implementing a similar feature, but this is quite a bit better than what I'd gotten done. I wonder, though, if it would be possible to tweak this so that instead of forcing the tabs to fill the bar, it would instead allow them to individually expand up to config.tab_max_width, filling the bar if enough tabs were present. Setting tab_max_width to 0 could trigger the force-fill behavior.

@dberlin
Copy link
Contributor Author

dberlin commented Dec 7, 2023

Hi, I discovered this after partially implementing a similar feature, but this is quite a bit better than what I'd gotten done. I wonder, though, if it would be possible to tweak this so that instead of forcing the tabs to fill the bar, it would instead allow them to individually expand up to config.tab_max_width, filling the bar if enough tabs were present. Setting tab_max_width to 0 could trigger the force-fill behavior.

Short answer: It's possible, but it's not super-easy and I don't plan on doing it myself, but you are welcome to.

Long answer:
Currently, wezterm's layout (including tabs) is a CSS box model implementation. But it's a bit buggy, and somewhat complicated. Simply because it's hard to handle the edge cases right in all cases, and for something like wezterm, which is using it for tab layout, if it's not perfect nobody notices unless you are doing something like tab fill :) That's actually fine - it's important to know what good enough looks like, and i would argue that, prior to tab-fill, what's there is more than good enough (IMHO)

However, for something like tab fill, it's like trying to make something work in CSS, which is hard enough, but then it's like trying to make it work in CSS when the underlying CSS engine works only right 80% of the time ;)

Originally, early versions of the patch did what you ask, mostly . But because of interesting box model bugs, it would run into/overwrite the right status icon, because of how filling is implemented. across two different children of the same parent. I also believe it would never have worked with bottom tab-bar.

You are basically asking for it to float left instead of float right. But if you implement float left, you will also discover it wil not draw the X/etc next to tabs properly in all cases.

Fixing this is not easy with the way the layout algorithm works right now. Which is why it now puts them in the same tree and floats them all right.
But doing so means that they always expand.

The algorithm used to do layout here only works in the degenerate case that they are all float right. It would not work if they are a mix. To be fair - it didn't work right in this case before i did this either :)

Fixing all ofthis is possible, and the algorithms are not theoretically hard, but you will probably spend a lot of time debugging lots of edge cases that exist and trying to figure out why they aren't doing what they are supposed to.

Quite honestly, my suggestion is that as this becomes more complicated, time would be better spent moving the box model to use something like taffy, which does exactly what all the box model code is trying to do, but is complete, well tested, and very fast at it. It is almost a mechanical port.

I have a port of wezterm to taffy somewhere, that does layout properly in all cases, and enables what you are talking about.

It is faster than the current layout engine (even with 100 tabs that can't possibly fit it takes <1ms to do layout), and enables what you are asking for to just work by setting the right layout attributes. In also lets you remove hundreds of lines of complicated layout code. The only slightly difficult part was handling the glyph bounding boxes, which taffy now easily supports with it's measurement function.

But i would not go down this path without the overall maintainer being willing to see it happen - i would instead live with always-expanding tabs ;)

@dberlin
Copy link
Contributor Author

dberlin commented Jun 24, 2024

Closing this because i use the branch in my repo a lot and it's going to generate noise here.

If we decide we want to look at this, happy to reopen a cleaner version.

@dberlin dberlin closed this Jun 24, 2024
@wez
Copy link
Owner

wez commented Jun 24, 2024

Quite honestly, my suggestion is that as this becomes more complicated, time would be better spent moving the box model to use something like taffy, which does exactly what all the box model code is trying to do, but is complete, well tested, and very fast at it. It is almost a mechanical port.

I have a port of wezterm to taffy somewhere, that does layout properly in all cases, and enables what you are talking about.

I didn't get a chance to fully review this, but this comment about taffy caught my eye.
I think I'd like to see first a port of the box_model stuff to taffy, then I think a bunch of UI-related feature requests will become a bit simpler to implement as follow-on PRs.

@dberlin
Copy link
Contributor Author

dberlin commented Jun 24, 2024

Sounds reasonable to me.
I'll send a PR to do that when i get a chance.

This particular PR became complicated/noisy in part because

  1. I sort of added comments to the PR as I went along the journey and thought about it . I usually don't.
  2. At some point I forgot this branch was being used for this PR and so it now has unrelated commits on it ;). Easy enough to fix.
  3. The code itself can't work perfectly in all cases because of how the box model is solved right now. So for example, there are interactions between floating and auto-filling. The tab filling assumes that all float-right elements are not also set to fill, and that they are all in a row. Otherwise, it doesn't know how much to fill vs float. These things can be fixed,
    but t some point fixing these would require rewriting how the solving happens anyway, which is why i went the taffy route - it's not obvious to me it's worth trying to write our own complete box model solver just to fill tabs and place some buttons/bars :)

So it was either "accept some limitations will likely last for a long time" or "see if i could outsource box model solving".

The latter turned out to be easier than expected - the representation we use and they use are very close already because everyone seems to have tried to hew to the naming/semantics of CSS.

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.

4 participants