[vim/vim] GTK4: Improve menus (PR #20593)

Foxe Chen

unread,

Jun 20, 2026, 9:24:26 PM (3 days ago) Jun 20

to vim/vim, Subscribed

Currently the menu system for the GTK4 GUI does not display accelerator keys and does not show accelerators. Additionally it does not work with mnemonics (winaltkeys option).

The current menu system relies on GMenuModel + GtkPopoverMenu, which separated the data from the view. However Vim already implements the "data" side in menu.c, which makes using GMenuModel awkward to use. Create a custom widget system for menu which works similarly to GtkMenuBar from GTK3. I tried making it as similar looking to the existing menu.

I experimented with using GtkPopoverMenu's custom widget feature, however I encountered a pretty big bug with it, so I didn't go with it.

Before:
image.png (view on web)

After:
image.png (view on web)

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/20593

Commit Summary

06a908d improve menu system for gtk4 gui

File Changes

(5 files)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Jun 21, 2026, 9:12:15 AM (3 days ago) Jun 21

to vim/vim, Subscribed

chrisbra left a comment (vim/vim#20593)

Thanks, but this adds quite some code and I am not sure this is what we want to do.
Before this goes in, though, I'd like to talk about the size, and there's one
real bug plus a couple of things to confirm.

On the size: this adds about 865 lines that build three new GTK widgets
(VimMenuBar, VimMenuItem, VimMenu) by hand, including all the hover, keyboard,
and open/close logic. That's basically reimplementing what GtkPopoverMenuBar
and GtkPopoverMenu already do. The GMenu code it replaces was much smaller
because GTK did that work for us. But it's a lot of new code that we now have to maintain ourselves: every future GTK
change in this area becomes our problem, not GTK's. Two requests:

Please add a sentence to the commit message saying why GMenu wasn't
enough, so the reason for the rewrite is on record.
Can you confirm there isn't a smaller fix for the actual compositor /
blank-drawing-area bugs, short of rebuilding the whole menu system? If
GMenu genuinely can't do per-item enable / mnemonics / accelerators the
way Vim needs, then this size is justified and that's fine.

Now the bug:

gui_mch_add_menu(), popup branch:

menu->submenu_id = vim_menu_new();
parent_widget = gtk_widget_get_parent(gui.drawarea);
gtk_widget_set_parent(menu->submenu_id, parent_widget);

Here submenu_id is left floating and the parent takes ownership of it, so Vim
only has a borrowed pointer -- unlike the menubar branch, which does
g_object_ref_sink(). But gui_mch_destroy_menu() treats them the same: it calls
gtk_widget_unparent(submenu_id), which drops the last ref and frees the
popover, and then g_clear_object(&submenu_id), which unrefs it a second time --
a use-after-free. The menubar path only survives because it took that extra
ref. Please ref_sink in the popup branch too, so destroy frees it exactly once.

Two more to check:

gui_mch_destroy_menu() always g_clear_object()s menu->id, but not every place
that creates a menu took a ref for Vim -- the toolbar separator
(vim_toolbar_insert_separator, marked TODO) and the toolbar button path don't.
Please confirm all five creation sites take exactly the reference that destroy
releases.

vim_menu_bar_item_menu_closed_cb() does
gtk_widget_unset_state_flags(GTK_WIDGET(menubar->active_item), ...), but
active_item can already be NULL by the time "closed" fires (popping a menu down
emits "closed"). That can pass NULL to GTK or clear the wrong item when
switching straight from one top-level menu to another. Add an active_item !=
NULL check and double-check the popdown/closed order.

Minor:

assert() isn't used elsewhere in Vim and disappears under -DNDEBUG; the
set_submenu / set_accel ones guard real invariants that would silently
corrupt aux_widget in a release build. Either drop them or use an always-on
check. Also confirm <assert.h> is actually included, not just pulled in by
accident.
gui_mch_menu_grey() and _hidden() now call gui_mch_update() every time, so
building a menu redraws once per item; hidden used to redraw only when
visibility actually changed.
Cosmetic: double blank lines in vim_menu_set_active_item and vim_menu_init;
typos "changeed" and "Don't think the check if necessary ... anyways"; the
assert(item->menu == NULL) right after g_object_new does nothing.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.

Foxe Chen

unread,

Jun 21, 2026, 1:37:23 PM (2 days ago) Jun 21

to vim/vim, Subscribed

64-bitman left a comment (vim/vim#20593)

My preferred approach would be to use GMenu with custom widgets (should be much less code). However I found an issue that prevented me from doing that. I have posted it in the GNOME development forums: https://discourse.gnome.org/t/custom-widget-for-gtkpopovermenubar-does-not-apply-for-nested-submenus/35629. If it turns out its not a bug and something I was doing wrong, I will likely close this PR and open another one on a separate branch that I already made with the preferred approach.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.

Foxe Chen

unread,

Jun 21, 2026, 2:49:41 PM (2 days ago) Jun 21

to vim/vim, Subscribed

64-bitman left a comment (vim/vim#20593)

Thanks, but this adds quite some code and I am not sure this is what we want to do. Before this goes in, though, I'd like to talk about the size, and there's one real bug plus a couple of things to confirm.

On the size: this adds about 865 lines that build three new GTK widgets (VimMenuBar, VimMenuItem, VimMenu) by hand, including all the hover, keyboard, and open/close logic. That's basically reimplementing what GtkPopoverMenuBar and GtkPopoverMenu already do. The GMenu code it replaces was much smaller because GTK did that work for us. But it's a lot of new code that we now have to maintain ourselves: every future GTK change in this area becomes our problem, not GTK's. Two requests:

The new custom menu implementation is slightly better in some ways. E.g. keyboard navigation works properly (GtkMenuPopover seems to be really finicky regarding that), also hjkl keys also work.

Please add a sentence to the commit message saying why GMenu wasn't
enough, so the reason for the rewrite is on record.

Should I squash all commits?

Can you confirm there isn't a smaller fix for the actual compositor /
blank-drawing-area bugs, short of rebuilding the whole menu system? If
GMenu genuinely can't do per-item enable / mnemonics / accelerators the
way Vim needs, then this size is justified and that's fine.

I'm not sure what this means

assert() isn't used elsewhere in Vim and disappears under -DNDEBUG; the
set_submenu / set_accel ones guard real invariants that would silently
corrupt aux_widget in a release build. Either drop them or use an always-on
check. Also confirm <assert.h> is actually included, not just pulled in by
accident.

Not an issue

gui_mch_menu_grey() and _hidden() now call gui_mch_update() every time, so
building a menu redraws once per item; hidden used to redraw only when
visibility actually changed.

GTK3 code does this so I added it back again

Cosmetic: double blank lines in vim_menu_set_active_item and vim_menu_init;
typos "changeed" and "Don't think the check if necessary ... anyways"; the
assert(item->menu == NULL) right after g_object_new does nothing.

Fixed

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.

Foxe Chen

unread,

12:35 AM (22 hours ago) 12:35 AM

to vim/vim, Subscribed

64-bitman left a comment (vim/vim#20593)

I think this is ready. No one has responded to my forum post and Claude says the GMenuModel custom widget thing is a bug with GTK. Also there are some things I'm pretty can't be implemented with custom widgets using GMenu.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward