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

Refactor Constraint System #4021

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheLostLambda
Copy link
Member

@TheLostLambda TheLostLambda commented Feb 24, 2025

Just a draft PR so @imsnif can keep on eye on the trouble I'm causing.

  • Look into taffy — will that make this a lot easier? Like the web, is tree-based, so doesn't fit our needs
  • Wrap something like good_lp / microlp? (and call it something fantastic like rectesselate)
  • Refactor stuff so that the stages are clearer and have less technical jargon (eg. spans, discretisations, etc.)
  • Make it so that the constraint system (now known as PaneResizer) doesn't operate on the panes themselves, but rather receives a list of PaneGeoms tied to pane ids and then sends back a list of those changed geoms to be mutated by whoever called it. This would be great because then it'll allow me to recover from whichever errors this system might have in whichever place in the code I place it.

@TheLostLambda
Copy link
Member Author

Might have to rule out taffy since it's tree-based, though honestly I'll need to remind myself how I got around that in the first place...

I'll look a little more, since it would be nice to not be stuck on super low-level code that's gone untouched / unmaintained for 7 years, but it might be the only thing reasonable for now.

@TheLostLambda
Copy link
Member Author

TheLostLambda commented Feb 24, 2025

Either way, it's probably worth adding a blog-style visual explanation of the algorithm to the docs — especially if it's tree-free, since that's rather unusual.

@TheLostLambda
Copy link
Member Author

TheLostLambda commented Feb 24, 2025

Or maybe good_lp (or even the microlp solver itself)? Certainly the other direction from taffy (it's very low level like cassowary), but with solvers that can work on integers — that would entirely eliminate that "discretisation" step and make rounding-based off-by-one errors impossible.

For @imsnif , is my ancient understanding here correct? We wanted to avoid anything tree-based so that you can take this:
image
And resize to either this:
image
Or this:
image

Without issue? But as I understood back then, layouts were still written as trees / nested panes? Either way, if we want the above, I think you're right we need to do things without trees!

@TheLostLambda
Copy link
Member Author

If I wrap good_lp or microlp, I could come up with a nice API and separate some of that lower-level stuff from zellij. I could toss together something a bit like cassowary, but that operates on integers and is focused on tree-free tiling. That would:

  1. Eliminate the need for any rounding, discretisation, etc — getting rid of the most confusing and error-prone code
  2. Mean that resizing code in zellij would be simplified by a much higher-level API that knows things need to tile into a gapless grid

And as a note to myself, I can resurrect that old graphical testing program I used to develop this resizer initially — that way I can quickly and graphically play with the algorithm, then drop it into zellij as a stand-alone module that takes PaneGeoms in, and then spits them back out. There could be worse excuses for playing with Bevy!

For @imsnif: Do 100% of resizes, new panes, pane removals, etc go through this code currently? I think the best way to prevent bugs would be only ever changing those Constraints, then having just one code path for getting from those to actual sizes! Ideally we'd make it impossible to manually set any non-constraint pane sizes?

@TheLostLambda
Copy link
Member Author

And finally, just so I can just have everything documented here for future me, does this seem like a bug related to this pane-resizing code?

Pane.Layout.Bug.mp4

It looks like it's moving from "Vertical" to "Stacked" mode, which is indeed new to me, so perhaps this is intended behavior?

@imsnif
Copy link
Member

imsnif commented Feb 25, 2025

You are right about the trees or lack thereof!

It's important to note that a lot has changed in the code (both code-wise and feature-wise). We now have swap layouts (https://zellij.dev/documentation/swap-layouts.html) which dictate how new panes are added and what happens when they are removed. After setting them we still go through the constraint system for reasons that escape me at the moment (I think some state gets borked otherwise). One can "break out" of a swap layout by resizing a pane (in that case Zellij figures out you want to do your own thing and lets you).

The PaneResizer is very much not the only place that changes pane sizes. To name a few, we have the layout interpreter (the part in charge of translating a layout into panes) which generally lives in zellij-utils/src/input/layout.rs, everything that has to do with StackedPanes and then there are floating panes which never go through this system at all.

I think the path of least resistance would be simplifying PaneResizer as you mentioned, if possible to get rid of all the percentage stuff and make it work on fixed numbers (but still behave the same!) that would be amazing. Then the part of getting the rest of the code base to behave the same would be technical debt but considerably easier than having it dance around the percentages as it does now.

Makes sense?

@imsnif
Copy link
Member

imsnif commented Feb 25, 2025

Also, as an addition - I think most of the resizing errors will go away if the system does a "snap to grid" sort of thing. Like, when resizing a pane up, the system looks at its neighboring panes to the right/left and tries to get it to align if possible (even if it means resizing less - but not more - than the requested amount).

By observation, most of the issues happen when a previous resize left the panes in a state where one is slightly protruding. If this makes sense?

@TheLostLambda
Copy link
Member Author

I'll respond to more properly later, but I do think it's important, if anything, to go the other direction with percentages and make more things (everything non-fixed?) use them? I think that's the only way to crunch the terminal down to a smaller size, have it look sane, then blow it back up to the original size and have it look like it originally did?

Because the floating panes don't seem to go through the resizer like you said, they (after being resized, to break out of the layout stuff, I think?) break things a bit on smaller terminal sizes:

Floating.Resize.mp4

A realistic context for this sort of issue is moving from a larger docked monitor, back to a smaller laptop screen when traveling.

But, looking at the rounding code you linked me (https://github.com/zellij-org/zellij/blob/main/zellij-server/src/panes/tiled_panes/mod.rs#L1464), I can see how just resizing by a given percentage isn't a great API for your end. If this new PaneResizer system provided an API where you could specify a desired resize increment in either cells or percentages, but it internally it converted everything to percentages (so that things always scale and resize sensibly), would that fix your issues you have with percentages?

It would at least mean that that rounding code is no longer needed, I think, since you could be sure that every resize actually changes the size of the pane, and if all resizing was done through this code-path, it could also implement that snapping you've mentioned — if you're about to resize a pane so it passes that snapping point, it just shortens the resize to snap to that grid point instead?

@TheLostLambda
Copy link
Member Author

And for my own reference, some other odd bugs showing up after resizing the terminal a bunch:

Bottom bar being overlapped?
image

A bunch of panes just disappearing? (Alt-n until stack, then resize to minimum, then return to original size):

Vanishing.Trick.mp4

@TheLostLambda
Copy link
Member Author

And final question for @imsnif for now: just so I understand the scope, are tiled, stacked, and floating panes all of the types currently? Are there any other types that you'd want to add in the near future?

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.

2 participants