gtk: fix invisible splits and focus being lost#12698
Open
dkinzler wants to merge 4 commits into
Open
Conversation
The cause of these bugs is that GTK can initially allocate a split/surface a width/height of 0 which causes it to get unmapped and lose focus. Additionally the split ratio is only set once but not accurately for tiny splits, which can keep a surface invisible even when the split gets resized later. To fix these problems the split ratio is always checked and possibly corrected when a split gets resized. Changes in a split ratio caused by the user dragging the divider are detected separately using an event controller. If a surface loses focus we restore it once the surface becomes mapped again.
max-position and position properties. Listening to drag events directly did not work that well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11193 where terminal surfaces might not appear and focus might be lost when creating multiple nested splits.
These bugs are caused by GTK initially allocating a tiny width/height to deeply nested splits. For a split with a tiny size, the split ratio will be set inaccurately e.g. to 1 which means that the right/bottom child of the split is invisible. If that child is the terminal surface that should have the focus, it will lose it. In the current implementation the split ratio can be set at most once, which means the inaccurate ratio never gets corrected and a surface (or an entire sub-tree of the layout) will stay invisible.
The following explains the current implementation and bug in more detail, it is a bit long, but I hope it will make it easier to review this PR.
Current Implementation
A split layout is a tree, in code represented by
datastruct/SplitTree, where inner nodes are splits and leafs are terminal surface. A split can be either horizontal or vertical, and has a ratio that defines how its space should be divided among the 2 children.The counterpart in the GTK UI is the
apprt/gtk/class/SplitTreewidget whoseonRebuild/buildTreefunctions build a widget tree that has the same structure as thedatastruct/SplitTree. The widget tree consists of aSplitTreeSplitwidget for every split and aSurfacewidget for every terminal surface.A
SplitTreeSplitwidget wraps agtk.Panedwidget, which displays its two children with a divider in between, either horizontally or vertically. How much space each child gets is determined by 3 properties.min_positionis always 0 in our case,max_positioncorresponds to the width/height (for horizontal/vertical splits) of the widget andpositionis where the divider should be. Sopositionis equivalent to the width/height of the left/top child and thereby also determines the width/height of the right/bottom child.SplitTreeSplitlistens for changes in the 3 properties. If there is one, thepropPosition,propMinPositionorpropMaxPositionfunction gets triggered and an idle callback for theonIdlefunction is added.We need to make sure that the widget tree and the
datastruct/SplitTreestay in sync.If we e.g. create a new split or close a surface, the structure of the split tree changes. In that case
gtk/class/SplitTree.onRebuildwill completely rebuild the widget tree (theSurfacewidgets are actually reused) to match the new tree structure. If we resize a split (i.e. change the split ratio) via action/keybind, we also completely rebuild the widget tree.Additionally we need to make sure that for every
SplitTreeSplit/gtk.Panedthepositiondivided bymax_positionmatches the ratio of the corresponding split node in ourdatastruct/SplitTree. There are two ways the current implementation keeps these ratios in sync, both are handled by theSplitTreeSplit.onIdlefunction.positionandmax_positionproperties of eachgtk.Panedwidget, which will trigger theSplitTreeSplit.onIdlefunction to run. GTK will not necessarily set position correctly, it is the task ofonIdleto make sure that the UI matches the layout defined by thedatastruct/SplitTree.onIdlechecks ifposition/max_positionmatches the ratio that the split should have and if not callsgtk.Paned.setPositionto update it. This can only happen once for each split sinceonIdlechecks if the position was set previously. The idea is that we should only ever need to set the position once, becausegtk.Panedwill automatically keep its current ratio whenever its size/max-positionchanges (if thesetPositionfunction has been called before). A size change can happen e.g. because the entire window was resized or because an ancestor split changed its split ratio.positionproperty ingtk.Panedchanges and eventually theSplitTreeSplit.onIdlefunction gets called. SincesetPositionshould have already been called when the widget was initially sized, we should fall through to the second case and write the current ratio back to thedatastructure/SplitTree.The problem with
SplitTreeSplit.onIdleis that sometimes the split ratio cannot be set accurately given the current size of thegtk.Panedwidget. BecauseonIdlecan only set the position/ratio once, any previous inaccuracy can never get corrected.For example with many nested vertical splits, GTK might initially allocate a
gtk.Panedwidget a height of 1. It will havemax_position=1andposition=1. WhenonIdleruns the current ratio ofposition/max_position = 1is different from the desired ratio of e.g. 0.5. But a ratio of 0.5 cannot be set, the position can only be 0 or 1 corresponding to a ratio of 0 or 1. The position will then get set as 1 and can't be changed later. Even when the split later gets a larger height, it will keep the ratio of 1 and the bottom child will stay invisible. When the surface that should be focused initially becomes invisible it loses focus and the focus will never be restored. That is exactly what happens in the first screencast in the issue description (#11193).Another problem with
onIdleis that thesetPositioncall inonIdlewill trigger another idle callback where the position change is sometimes wrongly interpreted as a manual update and written back to the tree. Also sometimes the initial ratio in agtk.Panedcan already be correct, in which case position will not get set. The next manual position update is then not detected as a manual update.Changes
SplitTreeSplit.onIdleis now able to set the split position every time the widget is resized, an inaccurate initial ratio will be corrected. To be able to distinguish a widget resize from a manual position update by the user, we keep track of whethermax-position,positionor both properties changed. If onlymax-positionor both properties changed, then the widget was resized. If justpositionchanged it is a manual update. This is kind of hacky but works. To verify I checked the source code forgtk.Paned, see the comment in the code ononIdle.SplitTreeSplitno longer listens to changes inmin-position, that should always be 0 (because we use the default resize/shrink properties forgtk.Paned) and there is already an assert inonIdlethat checks that.It can still happen that a surface initially gets allocated width/height 0 and loses focus. The only reliable way to detect when we can restore focus, is to listen to the
map/unmapsignals exposed bygtk.Widget. TheSurfacewidget now listens to these signals on itsGlAreachild (because that is where we want to put focus) and stores the current state in the newmappedproperty. TheSplitTreewidget listens to changes in that property: when surfaces become mapped, an idle callback for the newonRestoreFocusfunction is added, which will check whether the last focused surface is mapped and if so restore focus to it.Other possible solutions
Alternatively we could try to only set the split ratio once the split has its correct final size, but I think it's hard to detect that reliably. Or we could try to prevent the splits/surfaces from becoming invisible in the first place by e.g. setting a minimal widget size. But then you won't get the exactly correct layout and sometimes you do want a surface to be tiny or invisible e.g. you can drag the divider in a split all the way to one side.
The ideal solution would probably be to write a custom version of
gtk.Panedwhere you can provide the desired ratio on widget creation. Then everything will be sized correctly from the start, focus will not be lost. In terms of performance it would probably be better as well, because right now there can be multiple rounds of resizes until every split/surface has its correct size. I have played around with this a bit, it is definitely possible. But you would have to implement the divider widget, logic for layouting, handling gestures and co. That is a bigger undertaking.Testing
Tested by creating/modifying deeply nested layouts, dragging split dividers and resizing splits via keybind. Checked that ratios are maintained when the window is resized and tested that it works with zoom. Tested locally with hyprland and in a VM with KDE.
All the bugs described in the issue should be fixed.
AI Disclosure
Discovered the bug and wrote all code/comments by myself. Used AI in researching various internals of GTK.