Repository navigation
gtk: improve split sizing - #13414
Conversation
| @@ -1269,10 +1249,60 @@ const SplitTreeSplit = extern struct { | |||
| }; | |||
| paned.setPosition(desired_pos); | |||
| } else { | |||
| // If only position changed, this is a manual human update and | |||
| // we need to write our update back to the tree. | |||
| // Update ratio of the split tree node. This is the most dangerous | |||
There was a problem hiding this comment.
Should we add some debug asserts here?
There was a problem hiding this comment.
Can't hurt, added assertions for all the assumptions in the comment.
| // Sync split ratio between the gtk.Paned widget and the split tree. | ||
| // If to_paned=true, update the gtk.Paned widget to match the ratio | ||
| // in the split tree and vice-versa for to_paned=false. | ||
| fn syncSplitRatio(self: *Self, to_paned: bool) void { |
There was a problem hiding this comment.
I don't love to_paned. Maybe a well named enum would be easier to understand when reading the code?
There was a problem hiding this comment.
Changed to use an enum, that's definitely better. Alternatively I also thought about just having a function that returns all the parameters (min, max, position, current_ratio etc.) and then doing the rest directly in propMaxPosition/onIdle, but that is also kind of awkward.
|
Can you fix the commit message formatting for the last two commits? |
8e9588c to
ec9ff3d
Compare
|
Done, sorry about that. |
|
Can you merge the commits where it makes sense? For instance if they are fixing up things in a previous commit. |
ec9ff3d to
5c535bf
Compare
|
Of course, that makes sense. Done. |
|
I added a milestone. I am not that familiar with the split tree code at the moment. It'll take me some time get at the bones of this change, but everything seems to make sense, and the video is definitely an improvement. |
jcollie
left a comment
jcollie
left a comment
There was a problem hiding this comment.
This LGTM but it should be rebased on the latest main so that it's past the Zig 0.16 changes (the rebase is trivial).
For widget resizes the split ratio is now synced directly from the propMaxPosition callback in the SplitTreeSplit widget, instead of an idle callback. With this change all surfaces will be sized correctly from the start, in a single round of size allocation in GTK. Previously, surfaces would initially be shown with the wrong size for a few frames until the idle callback ran, resulting in visible flickering. This was especially visible when resizing a split quickly by holding down the resize keybind. Moved logic to sync split ratio between gtk.Paned widget and split tree into new syncSplitRatio function in the SplitTreeSplit widget. Added debug assertions and log warnings to syncSplitRatio where we look up the split tree.
5c535bf to
90da3ab
Compare
jcollie
left a comment
jcollie
left a comment
There was a problem hiding this comment.
LGTM but let's see if @tristan957 has any further feedback.
No feedback. Feel free to merge! |
This PR improves the way splits/surfaces are sized in the GTK app, which eliminates flickering and slightly improves performance.
Fixes #13328, #12709, #11187.
Related #8208 (closed) but some later comments mention flickering issues persisting.
Builds on top of the changes in #12698.
Previously an idle callback was used to sync the split ratio between the GTK widget tree and the split tree data structure that represents the split layout. The widget tree contains a
SplitTreeSplitwidget, which wraps aGtkPanedwidget, for every split. During size allocation aGtkPanedwidget first computes the initial position of the divider and thereby the size for its two children. We get notified of that position (and the max possible position) via thepropPosition/propMaxPositioncallbacks inSplitTreeSplitand set up an idle callback (theonIdlefunction) to update the position if it does not match the desired split ratio. Since the initial position is often not correct, especially in nested layouts or if the ratio is not 0.5, a surface will first be shown with the wrong size for a few frames until the idle callback runs and corrects the sizing. In nested layouts it might take multiple rounds of size allocation and idle callbacks until every surface gets the correct size. This causes flickering as widgets eventually snap to another size, which is especially noticeable if the layout changes quickly e.g. when resizing a split using keybinds.To fix this, the divider position will now be corrected directly from the
propMaxPositioncallback, which runs during GTK size allocation, right after aGtkPanedcomputes the initial position and right before it uses the position to allocate sizes for its two children. With this change every surface will be sized correctly during the first round of size allocation.The idle callback is still used to update the ratio in the split tree when a split is resized by manually dragging the divider in the UI. The logic to sync the split ratio was moved to the new
syncSplitRatiofunction which is called from bothpropMaxPositionandonIdle.This is kind of hacky, but I reviewed the GTK source code in detail to verify that this is safe (see the various code comments for more details). I also tested extensively on both Hyprland and KDE Plasma: creating deeply nested layouts, resizing with both keybinds and dragging dividers by hand, with multiple tabs, resizing the entire window, resizing entire subtrees to 0 and back. Everything seems to work fine.
For performance testing I used sysprof which can also collect GTK stats. When creating/deleting/resizing splits I can measure a slight but consistent increase in GTK FPS (+5 to 10) on my system. Other than that CPU usage and FPS seem to be the same before and after. I guess this makes sense, while we added a bit of work to the GTK loop during size allocation, we avoid surfaces being resized.
For the flickering, here's a side-by-side comparison. Left is before the changes, right is after.
comparison.mp4
AI Disclosure: no AI was used.