mirrors/ghostty
1
0
Fork 0
mirror of https://github.com/ghostty-org/ghostty.git synced 2026-05-23 21:30:19 +00:00
Code Issues Packages Projects Releases Wiki Activity

gtk: fix invisible splits and focus being lost (#12698)

Browse Source 
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/SplitTree` widget
whose `onRebuild/buildTree` functions build a widget tree that has the
same structure as the `datastruct/SplitTree`. The widget tree consists
of a `SplitTreeSplit` widget for every split and a `Surface` widget for
every terminal surface.

A `SplitTreeSplit` widget wraps a `gtk.Paned` widget, 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_position` is always 0 in our case, `max_position`
corresponds to the width/height (for horizontal/vertical splits) of the
widget and `position` is where the divider should be. So `position` is
equivalent to the width/height of the left/top child and thereby also
determines the width/height of the right/bottom child. `SplitTreeSplit`
listens for changes in the 3 properties. If there is one, the
`propPosition`, `propMinPosition` or `propMaxPosition` function gets
triggered and an idle callback for the `onIdle` function is added.

We need to make sure that the widget tree and the `datastruct/SplitTree`
stay 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.onRebuild` will
completely rebuild the widget tree (the `Surface` widgets 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.Paned` the `position` divided by `max_position`
matches the ratio of the corresponding split node in our
`datastruct/SplitTree`. There are two ways the current implementation
keeps these ratios in sync, both are handled by the
`SplitTreeSplit.onIdle` function.
1. Initially when the widget tree is built, GTK allocates each widget a
size. Specifically it also sets the `position` and `max_position`
properties of each `gtk.Paned` widget, which will trigger the
`SplitTreeSplit.onIdle` function to run. GTK will not necessarily set
position correctly, it is the task of `onIdle` to make sure that the UI
matches the layout defined by the `datastruct/SplitTree`. `onIdle`
checks if `position/max_position` matches the ratio that the split
should have and if not calls `gtk.Paned.setPosition` to update it. This
can only happen once for each split since `onIdle` checks if the
position was set previously. The idea is that we should only ever need
to set the position once, because `gtk.Paned` will automatically keep
its current ratio whenever its size/`max-position` changes (if the
`setPosition` function 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.
2. The user can manually change the ratio between the two children of a
split by dragging the divider between them in the UI. When that happens
the `position` property in `gtk.Paned` changes and eventually the
`SplitTreeSplit.onIdle` function gets called. Since `setPosition` should
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 the
`datastructure/SplitTree`.

The problem with `SplitTreeSplit.onIdle` is that sometimes the split
ratio cannot be set accurately given the current size of the `gtk.Paned`
widget. Because `onIdle` can 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.Paned` widget a height of 1. It will have
`max_position=1` and `position=1`. When `onIdle` runs the current ratio
of `position/max_position = 1` is 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 `onIdle` is that the `setPosition` call in `onIdle`
will 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 a `gtk.Paned` can 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.onIdle` is 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 whether `max-position`, `position` or both
properties changed. If only `max-position` or both properties changed,
then the widget was resized. If just `position` changed it is a manual
update. This is kind of hacky but works. To verify I checked the source
code for `gtk.Paned`, see the comment in the code on `onIdle`.

`SplitTreeSplit` no longer listens to changes in `min-position`, that
should always be 0 (because we use the default resize/shrink properties
for `gtk.Paned`) and there is already an assert in `onIdle` that 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/unmap` signals exposed by `gtk.Widget`.
The `Surface` widget now listens to these signals on its `GlArea` child
(because that is where we want to put focus) and stores the current
state in the new `mapped` property. The `SplitTree` widget listens to
changes in that property: when surfaces become mapped, an idle callback
for the new `onRestoreFocus` function 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.Paned` where 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.
This commit is contained in:
Jeffrey C. Ollie
2026-05-17 20:20:01 -05:00
committed by GitHub
parent e90b7c9fad 9f72eb9d7c
commit 4814aee44e
4 changed files with 174 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

165
src/apprt/gtk/class/split_tree.zig
View File

@@ -158,6 +158,13 @@ pub const SplitTree = extern struct {
/// used to debounce updates.
rebuild_source: ?c_uint = null,
/// The source that we use to restore focus. With enough nested
/// splits, some surfaces might initially be allocated a width or
/// height of 0 which causes them to get unmapped and lose focus.
/// We can reliably restore focus to the last focused surface only
/// once it is mapped again.
restore_focus_source: ?c_uint = null,
/// Used to store state about a pending surface close for the
/// close dialog.
pending_close: ?Surface.Tree.Node.Handle,
@@ -415,6 +422,13 @@ pub const SplitTree = extern struct {
self,
.{ .detail = "focused" },
);
_ = gobject.Object.signals.notify.connect(
surface,
*Self,
propSurfaceMapped,
self,
.{ .detail = "mapped" },
);
}
}
@@ -571,6 +585,12 @@ pub const SplitTree = extern struct {
}
priv.rebuild_source = null;
}
if (priv.restore_focus_source) |v| {
if (glib.Source.remove(v) == 0) {
log.warn("unable to remove restore_focus source", .{});
}
priv.restore_focus_source = null;
}
gtk.Widget.disposeTemplate(
self.as(gtk.Widget),
@@ -766,6 +786,24 @@ pub const SplitTree = extern struct {
self.as(gobject.Object).notifyByPspec(properties.@"active-surface".impl.param_spec);
}
fn propSurfaceMapped(
surface: *Surface,
_: *gobject.ParamSpec,
self: *Self,
) callconv(.c) void {
if (!surface.getMapped()) return;
// We could add the idle callback only if this is actually the last
// focused surface. But we can avoid that check because usually all
// the surfaces get mapped at once, so the idle callback will run
// only once anyway.
const priv = self.private();
if (priv.restore_focus_source == null) priv.restore_focus_source = glib.idleAdd(
onRestoreFocus,
self,
);
}
fn propTree(
self: *Self,
_: *gobject.ParamSpec,
@@ -779,14 +817,20 @@ pub const SplitTree = extern struct {
self.as(gobject.Object).notifyByPspec(properties.@"has-surfaces".impl.param_spec);
self.as(gobject.Object).notifyByPspec(properties.@"is-zoomed".impl.param_spec);
// If we were planning a rebuild, always remove that so we can
// start from a clean slate.
// If we were planning a rebuild or focus restore, always remove
// that so we can start from a clean slate.
if (priv.rebuild_source) |v| {
if (glib.Source.remove(v) == 0) {
log.warn("unable to remove rebuild source", .{});
}
priv.rebuild_source = null;
}
if (priv.restore_focus_source) |v| {
if (glib.Source.remove(v) == 0) {
log.warn("unable to remove restore_focus source", .{});
}
priv.restore_focus_source = null;
}
// If we transitioned to an empty tree, clear immediately instead of
// waiting for an idle callback. Delaying teardown can keep the last
@@ -842,6 +886,26 @@ pub const SplitTree = extern struct {
return 0;
}
fn onRestoreFocus(ud: ?*anyopaque) callconv(.c) c_int {
const self: *Self = @ptrCast(@alignCast(ud orelse return 0));
// Always mark our source as null since we're done.
const priv = self.private();
priv.restore_focus_source = null;
// If we have a last-focused surface and it is mapped, restore focus
// to it. Depending on the available size, the surface might already
// have focus because it never got unmapped. In that case grabbing
// focus will have no effect.
if (priv.last_focused.get()) |v| {
defer v.unref();
if (v.getMapped()) {
v.grabFocus();
}
}
return 0;
}
/// Builds the widget tree associated with a surface split tree.
///
/// Returned widgets are expected to be attached to a parent by the caller.
@@ -1044,6 +1108,12 @@ const SplitTreeSplit = extern struct {
/// Source to handle repositioning the split when properties change.
idle: ?c_uint = null,
/// Whether the max-position/position property of the gtk.Paned widget
/// changed. We use these to distinguish between a resize and the user
/// manually moving the split divider. See the "on-idle" function.
max_changed: bool = false,
pos_changed: bool = false,
// Template bindings
paned: *gtk.Paned,
@@ -1083,21 +1153,37 @@ const SplitTreeSplit = extern struct {
gtk.Widget.initTemplate(self.as(gtk.Widget));
}
fn refresh(self: *Self) void {
const priv = self.private();
if (priv.idle == null) priv.idle = glib.idleAdd(
onIdle,
self,
);
}
// We need to keep the split ratios from the tree datastructure and
// widget tree in sync. Using the max-position and position properties
// of the gtk.Paned widget, we can distinguish a resize from a manual
// update (e.g. the user dragging the divider).If max-position changes,
// we always have a widget resize. Usually position will change as well
// but it might not if the size change is small enough. If only position
// changes, we have a manual human update.
//
// This is a hack, it relies on the timing of property notifcations.
// From looking at the GTK source code, it should not be possible that
// we interpret a position change from a resize as a manual update.
// When a gtk.Paned is resized, internally the gtk_paned_calc_position
// function will change both max-position and position and synchronously
// call our propMaxPosition and propPosition functions. I.e. when the
// widget is resized, it should not be possible for onIdle to run before
// we have been notified of both property changes.
fn onIdle(ud: ?*anyopaque) callconv(.c) c_int {
const self: *Self = @ptrCast(@alignCast(ud orelse return 0));
const priv = self.private();
const paned = priv.paned;
// Our idle source is always over
priv.idle = null;
// Clear source and fields at the end. Otherwise if setPosition is
// called below, propPosition is triggered and would add another
// idle callback before this one is finished.
defer priv.idle = null;
defer priv.max_changed = false;
defer priv.pos_changed = false;
if (!priv.max_changed and !priv.pos_changed) {
return 0;
}
// Get our split. This is the most dangerous part of this entire
// widget. We assume that this widget is always a child of a
@@ -1132,16 +1218,6 @@ const SplitTreeSplit = extern struct {
);
break :max gobject.ext.Value.get(&val, c_int);
};
const pos_set: bool = max: {
var val = gobject.ext.Value.new(c_int);
defer val.unset();
gobject.Object.getProperty(
paned.as(gobject.Object),
"position-set",
&val,
);
break :max gobject.ext.Value.get(&val, c_int) != 0;
};
// We don't actually use min, but we don't expect this to ever
// be non-zero, so let's add an assert to ensure that.
@@ -1172,51 +1248,51 @@ const SplitTreeSplit = extern struct {
return 0;
}
// If we're out of bounds, then we need to either set the position
// to what we expect OR update our expected ratio.
// If we've never set the position, then we set it to the desired.
if (!pos_set) {
if (priv.max_changed) {
// Widget got resized, update position to match desired ratio.
// Note that if max-position is small, it might not be possible
// to accurately set the desired ratio. E.g. with max-position=2
// you can only have ratios 0, 0.5 and 1.
const desired_pos: c_int = desired_pos: {
const max_f64: f64 = @floatFromInt(max);
break :desired_pos @intFromFloat(@round(max_f64 * desired_ratio));
};
paned.setPosition(desired_pos);
return 0;
} else {
// If only position changed, this is a manual human update and
// we need to write our update back to the tree.
tree.resizeInPlace(priv.handle, @floatCast(current_ratio));
}
// If we've set the position, then this is a manual human update
// and we need to write our update back to the tree.
tree.resizeInPlace(priv.handle, @floatCast(current_ratio));
return 0;
}
//---------------------------------------------------------------
// Signal handlers
fn propPosition(
_: *gtk.Paned,
_: *gobject.ParamSpec,
self: *Self,
) callconv(.c) void {
self.refresh();
}
fn propMaxPosition(
_: *gtk.Paned,
_: *gobject.ParamSpec,
self: *Self,
) callconv(.c) void {
self.refresh();
const priv = self.private();
priv.max_changed = true;
if (priv.idle == null) priv.idle = glib.idleAdd(
onIdle,
self,
);
}
fn propMinPosition(
fn propPosition(
_: *gtk.Paned,
_: *gobject.ParamSpec,
self: *Self,
) callconv(.c) void {
self.refresh();
const priv = self.private();
priv.pos_changed = true;
if (priv.idle == null) priv.idle = glib.idleAdd(
onIdle,
self,
);
}
//---------------------------------------------------------------
@@ -1275,7 +1351,6 @@ const SplitTreeSplit = extern struct {
// Template Callbacks
class.bindTemplateCallback("notify_max_position", &propMaxPosition);
class.bindTemplateCallback("notify_min_position", &propMinPosition);
class.bindTemplateCallback("notify_position", &propPosition);
// Virtual methods

53
src/apprt/gtk/class/surface.zig
View File

@@ -169,6 +169,24 @@ pub const Surface = extern struct {
);
};
pub const mapped = struct {
pub const name = "mapped";
const impl = gobject.ext.defineProperty(
name,
Self,
bool,
.{
.default = false,
.accessor = gobject.ext.privateFieldAccessor(
Self,
Private,
&Private.offset,
"mapped",
),
},
);
};
pub const @"min-size" = struct {
pub const name = "min-size";
const impl = gobject.ext.defineProperty(
@@ -592,11 +610,15 @@ pub const Surface = extern struct {
/// focus events.
focused: bool = true,
/// Whether the GLArea widget is mapped. Some operations like grabbing
/// focus only work if a widget is mapped.
mapped: bool = false,
/// Whether this surface is "zoomed" or not. A zoomed surface
/// shows up taking the full bounds of a split view.
zoom: bool = false,
/// The GLAarea that renders the actual surface. This is a binding
/// The GLArea that renders the actual surface. This is a binding
/// to the template so it doesn't have to be unrefed manually.
gl_area: *gtk.GLArea,
@@ -1768,6 +1790,7 @@ pub const Surface = extern struct {
priv.mouse_shape = .text;
priv.mouse_hidden = false;
priv.focused = true;
priv.mapped = false;
priv.size = .{ .width = 0, .height = 0 };
priv.vadj_signal_group = null;
@@ -2019,6 +2042,11 @@ pub const Surface = extern struct {
return self.private().focused;
}
/// Returns true if the GLArea of this surface is mapped.
pub fn getMapped(self: *Self) bool {
return self.private().mapped;
}
/// Change the configuration for this surface.
pub fn setConfig(self: *Self, config: *Config) void {
const priv = self.private();
@@ -3250,6 +3278,26 @@ pub const Surface = extern struct {
priv.im_context.as(gtk.IMContext).setClientWidget(null);
}
fn glareaMap(
_: *gtk.GLArea,
self: *Self,
) callconv(.c) void {
self.updateMapped(true);
}
fn glareaUnmap(
_: *gtk.GLArea,
self: *Self,
) callconv(.c) void {
self.updateMapped(false);
}
fn updateMapped(self: *Self, mapped: bool) void {
const priv = self.private();
priv.mapped = mapped;
self.as(gobject.Object).notifyByPspec(properties.mapped.impl.param_spec);
}
fn glareaRender(
_: *gtk.GLArea,
_: *gdk.GLContext,
@@ -3560,6 +3608,8 @@ pub const Surface = extern struct {
class.bindTemplateCallback("drop", &dtDrop);
class.bindTemplateCallback("gl_realize", &glareaRealize);
class.bindTemplateCallback("gl_unrealize", &glareaUnrealize);
class.bindTemplateCallback("gl_map", &glareaMap);
class.bindTemplateCallback("gl_unmap", &glareaUnmap);
class.bindTemplateCallback("gl_render", &glareaRender);
class.bindTemplateCallback("gl_resize", &glareaResize);
class.bindTemplateCallback("im_preedit_start", &imPreeditStart);
@@ -3592,6 +3642,7 @@ pub const Surface = extern struct {
properties.@"error".impl,
properties.@"font-size-request".impl,
properties.focused.impl,
properties.mapped.impl,
properties.@"key-sequence".impl,
properties.@"key-table".impl,
properties.@"min-size".impl,

2
src/apprt/gtk/ui/1.2/surface.blp
View File

@@ -23,6 +23,8 @@ Overlay terminal_page {
GLArea gl_area {
realize => $gl_realize();
unrealize => $gl_unrealize();
map => $gl_map();
unmap => $gl_unmap();
render => $gl_render();
resize => $gl_resize();
hexpand: true;

1
src/apprt/gtk/ui/1.5/split-tree-split.blp
View File

@@ -13,7 +13,6 @@ template $GhosttySplitTreeSplit: Adw.Bin {
Adw.Bin {
Paned paned {
notify::max-position => $notify_max_position();
notify::min-position => $notify_min_position();
notify::position => $notify_position();
}
}