gtk: use Adwaita TabView when possible #2051

Pangoraw · 2024-08-06T09:40:00Z

👻 Adwaita - Gnome 46.3

👻 Adwaita - Gnome 42.9

This moves the logic related to gdk_notebook to a new struct Notebook which is an enum dispatching to either the gtk_notebook or adw_tabview.

One improvement to make before merging is that there should no runtime cost to Notebook when gtk-libadwaita = false at build time.

I am opening as draft because quoting Mitchell on discord:

I've said in the past Im' open to using the adwaita APIs for more elements but want to see them introduced in a way that doesn't overburden the GTK backend too much. I'm pretty sure its possible.

as I am not sure my current approach is the best for limiting changes in the GTK backend.

With regards to #1122, the AdwTabview handles dropping tab on a tab from another window to move the tab to the new window.

kareigu · 2024-08-06T10:07:13Z

src/apprt/gtk/notebook.zig

+
+pub const Notebook = union(enum) {
+    adw_tab_view: *AdwTabView,
+    gtk_notebook: *c.GtkNotebook,


You could turn this into a struct with a single field that gets its type defined at comptime by checking build_options.libadwaita.

Then you can do the same comptime check for each of the functions rather than switching on an union at runtime.

That sounds like a good plan for the case when build_options.libadwaita is false but when it's true there still needs to be dispatch in the case because the config option gtk-adwaita can be false.

Ah right, we had that runtime option as well.

Then the union makes the most sense but we can still do the switching based on build_options.libadwaita.
I think having something like:

if (!build_options.libadwaita or self == .gtk_notebook) { /// gtk code return; } /// adwaita code

Should be enough for the compiler to discard all of the adwaita logic while avoiding having to duplicate the normal gtk logic for 2 cases.

rockorager · 2024-08-06T13:58:33Z

cc @jcollie @tristan957

jcollie · 2024-08-06T14:54:39Z

I haven't looked at the code yet, but this might be a good time to decide if we want to fully commit to libadwaita and remove the conditionals, or if conditional compilation/runtime is still needed.

tristan957 · 2024-08-07T22:45:16Z

I see 4 resolutions:

Remove the GTK apprt and go all in on libadwaita
Remove the libadwaita apprt and go all in on GTK
Support both a GTK apprt and a libadwaita apprt
This PR

I personally think maintaining abstractions is a maintenance burden.

rockorager · 2024-08-09T14:54:02Z

- Remove the GTK apprt and go all in on libadwaita

I don't think we should do this. I can imagine packagers will prefer having both options.

- Remove the libadwaita apprt and go all in on GTK

Also don't think we want this. Libadwaita is just too nice.

- Support both a GTK apprt and a libadwaita apprt

Are there *other* libadwaita features that would require having a separate apprt? It seems like maintaining two apprts is a lot more work than abstracting a portion of the widgets.

- This PR

Of these options, this one seems to be the best path. I'm testing it out today...those libadwaita tabs are just :chefs-kiss:

tristan957 · 2024-08-09T14:59:12Z

Are there other libadwaita features that would require having a separate
apprt? It seems like maintaining two apprts is a lot more work than abstracting
a portion of the widgets.

Other widgets we probably want to use:

AdwBanner instead of GtkInfoBar
AdwAlertDialog instead of GtkMessageDialog
AdwAboutDialog instead of GtkAboutDialog
AdwPreferencesDialog (if we want native widgetry for configuring ghostty via GUI)
AdwHeaderBar instead of GtkHeaderBar
AdwApplicationWindow instead of GtkApplicationWindow
AdwToast/AdwToastOverlay (thanks jcollie!)

tristan957 · 2024-08-09T15:25:51Z

Also we may want to use libpanel for managing splits in libadwaita.

jcollie · 2024-08-09T15:38:06Z

It would be nice to have AdwToast/AdwToastOverlay to pop up a little notification that shows the size of the screen as you are dynamically resizing the window.

tristan957 · 2024-08-09T16:06:15Z

@jcollie I think what ptyxis does is a bit better. Take a look for yourself. We could probably just lift the code straight from ptyxis, but someone would need to think about the license differences. Or we could just reuse the machinery of the url thingies that Mitchell added.

Toasts would be better for clipboard interaction, I think. It would be similar to what Android (and I believe iOS) does.

jcollie · 2024-08-09T16:47:33Z

Remove the GTK apprt and go all in on libadwaita
I don't think we should do this. I can imagine packagers will prefer having both options.

I'm not convinced that the support burden is worth it, especially the more usage of libadwaita we add. Basically, we've got to implement everything twice, once for libadwaita, and once for non-libadwaita and do it in a way that can be switched at runtime.

jcollie · 2024-08-09T16:49:47Z

I think what ptyxis does is a bit better.

Seems as if it's the same as Gnome Console, so perhaps it's actually a libvte thing. It's fine except that I almost missed it because it's in the lower right corner. I'd prefer centered.

Pangoraw · 2024-08-16T08:10:59Z

AdwHeaderBar instead of GtkHeaderBar
AdwApplicationWindow instead of GtkApplicationWindow

FYI, this pr already uses the adwaita version for both of these in order to have the tabview blend in the header bar.

Pangoraw · 2024-08-20T14:10:47Z

Remove the GTK apprt and go all in on libadwaita

From my experience using this PR day-to-day on multiple computers, the libadwaita version available on my Ubuntu system is older and require a partial integration of libadwaita since for example an AdwWindow cannot be used:

ghostty/src/apprt/gtk/Window.zig

Lines 65 to 69 in 2c3033b

    
           const adw_window = adwaita and app.config.@"gtk-titlebar" and c.ADW_MINOR_VERSION >= 4; 
        
           const window: *c.GtkWidget = if (adw_window) 
        
               c.adw_application_window_new(app.app) 
        
           else 
        
               c.gtk_application_window_new(app.app);

Therefore, going all in on libadwaita may potentially exclude users running on systems where libadwaita version is older. If people could test this PR and report if things works for them it would be very nice as I could only test on two systems with different libadwaita versions. I added screenshots of ghostty with the two adwaita versions in the main comment of this PR.

jcollie · 2024-08-20T14:21:20Z

Remove the GTK apprt and go all in on libadwaita

From my experience using this PR day-to-day on multiple computers, the libadwaita version available on my Ubuntu system is older and require a partial integration of libadwaita since for example an AdwWindow cannot be used:

Which version of Ubuntu and what's the full version of libadwaita shipped with that version? Are there any other major distros that are still supported that ship that version? (Looking at you RHEL and clones.)

Pangoraw · 2024-08-20T14:45:50Z

Which version of Ubuntu and what's the full version of libadwaita shipped with that version?

Ubuntu 22.04 which has libadwaita 1.1. My fedora and arch systems both have at least libadwaita 1.4 (AdwWindow support).

add tab bar view

update

gate toolbar view on ADWAITA_MINOR >= 4 use the right destructor on window creation errors

this falls back to top when using either right or left.

mitchellh · 2024-09-11T16:39:32Z

I'm starting to work on this, applying some more complicated review comments I would've had.

Just noting that for me, when I do zig build run and exit, I get a segfault. Something to look at.

mitchellh · 2024-09-11T16:47:56Z

Explaining the changes I made so far:

I moved as much common functions for adw into adwaita.zig. In this, I did the following: there is an enabled() and versionAtLeast() function. Importantly, both of these functions can be called in a comptime and non-comptime context.

The comptime context will check enabled/version with build-time information, i.e. the build config, headers, etc. The non-comptime context will check enabled version with runtime information, i.e. the actual Ghostty config and the linked libadw functions (not the headers).

Somewhat annoyingly, this makes the adw checks a bit verbose, because you have to do all of them. Let's take a look at a basic example:

if ((comptime adwaita.versionAtLeast(1, 3, 0)) and
     adwaita.enabled(&app.config) and
     adwaita.versionAtLeast(1, 3, 0)) {}

The comptime check prevents the basic block from being compiled if we don't have libadw at all or if the header versions are too low, because the definitions won't be available.

The runtime checks prevent the logic from running if someone configured Ghostty at runtime to disable adw or the systems dynamically linked libadw is too old.

mitchellh · 2024-09-11T16:59:26Z

Okay, I'm pretty happy/fine with this PR. Thanks for the good work @Pangoraw. The last issue we need to fix is that sometimes when exit we get a segfault. It appears that for some reason the AdwTabView pointer is undefined (0xAA...). I haven't figured out why yet.

Can you try to look into that @Pangoraw ?

mitchellh · 2024-09-11T17:06:05Z

One improvement to make before merging is that there should no runtime cost to Notebook when gtk-libadwaita = false at build time.

I think we can punt this to the future if we do it at all. The runtime overhead of the switching is so low compared to everything else going on with a UI that I wouldn't sweat this detail.

tristan957 · 2024-09-11T17:16:07Z

Doing a zig build run on this PR:

$ zig build run
run
└─ run ghostty
   └─ zig build-exe ghostty Debug native 1 errors
src/apprt/gtk/notebook.zig:68:17: error: unable to evaluate comptime expression
            !app.config.@"gtk-titlebar")
             ~~~^~~~~~~
referenced by:
    create: src/apprt/gtk/notebook.zig:21:50
    init: src/apprt/gtk/Window.zig:145:29

mitchellh · 2024-09-11T17:17:02Z

The issue seems to be that adw_tab_view_close_page doesn't unref the container until some event loop cycles later and we depend on that all happening in the current loop run. So we're referencing bad memory later.

mitchellh · 2024-09-11T17:17:51Z

Doing a zig build run on this PR:

$ zig build run
run
└─ run ghostty
   └─ zig build-exe ghostty Debug native 1 errors
src/apprt/gtk/notebook.zig:68:17: error: unable to evaluate comptime expression
            !app.config.@"gtk-titlebar")
             ~~~^~~~~~~
referenced by:
    create: src/apprt/gtk/notebook.zig:21:50
    init: src/apprt/gtk/Window.zig:145:29

Fixed.

tristan957 · 2024-09-11T17:22:55Z

I tried looking into the corrupted pointer, but I can't reproduce.

mitchellh · 2024-09-11T17:27:51Z

I tried looking into the corrupted pointer, but I can't reproduce.

I wonder if this is libadwaita bug in prior versions that's now fixed. My libadw version is 1.3.2. Let me see if I can get a newer one and try.

Pangoraw · 2024-09-11T17:28:45Z

The issue seems to be that adw_tab_view_close_page doesn't unref the container until some event loop cycles later and we depend on that all happening in the current loop run. So we're referencing bad memory later.

Thanks for pointing the error location. I never encountered this error but maybe we need to move these two lines to a close-page event handler or something:

ghostty/src/apprt/gtk/notebook.zig

Lines 277 to 279 in 8186a88

    
           // If we have no more tabs we close the window 
        
           if (self.nPages() == 0) 
        
               c.gtk_window_destroy(tab.window.window);

tristan957 · 2024-09-11T17:29:02Z

I have 1.5.3 for reference.

mitchellh · 2024-09-11T17:31:14Z

It is a libadw bug. Updating to 1.5.1 fixes it.

For details, I was able to reproduce the bug with minimal Ghostty interaction: adw_tab_view_close_page doesn't release the reference to its contents so it doesn't trigger cleanup. Later versions do.

Let me think if its worth having a workaround or not.

mitchellh · 2024-09-11T17:44:57Z

Fixed on libadw 1.3.x.

Fixes #2023 Fixes regression from #2051 #2051 introduced a regression where `window-decoration=false` had no effect when libadwaita was in use. Further, the `toggle_window_decorations` keybinding also had no effect whatsoever. This commit fixes this and #2023 by explicitly hiding the header bar when window decorations are disabled. We hide the header bar rather than the full top bar because we still want the tab bar to show up with window decorations disabled.

kareigu reviewed Aug 6, 2024

View reviewed changes

Pangoraw and others added 9 commits September 11, 2024 15:35

gtk: use adwaita tab view when possible

0e22695

add tab bar view

adw: move notebook to its own file

42c93d8

adw: respect gtk-wide-tabs

9c8a9f3

update

adw: implement next/previous

5327daa

gtk: respect wide-tabs option

4399889

gtk: add devel class to window in debug builds

05e7bf7

adw: update window title on selected page change

618a7a3

gtk: adw toolbar view

bdf618d

gate toolbar view on ADWAITA_MINOR >= 4 use the right destructor on window creation errors

gtk: add adwaita banner

0c286a0

Pangoraw force-pushed the adw_tab_view branch from 4671719 to 43ba748 Compare September 11, 2024 13:43

Pangoraw changed the title ~~[DRAFT] gtk: use Adwaita TabView when possible~~ gtk: use Adwaita TabView when possible Sep 11, 2024

adw: add support for gtk-tabs-location = bottom

7aa6b00

this falls back to top when using either right or left.

Pangoraw force-pushed the adw_tab_view branch from 43ba748 to 7aa6b00 Compare September 11, 2024 15:29

mitchellh added 2 commits September 11, 2024 09:21

apprt/gtk: move adw enabling and version checks into shared file

1ab850f

apprt/gtk: use adwaita helpers, move out some complicated logic

afc95fe

apprt/gtk: the comptimeEnabled check is redundant

c27f427

apprt/gtk: scope comptime to only one

8186a88

apprt/gtk: log the libadwaita version at startup if we use it

2b5d436

apprt/gtk: cleaup final notebook page on libadw 1.3.x

37ba052

mitchellh merged commit 050602a into ghostty-org:main Sep 11, 2024
17 checks passed

mitchellh deleted the adw_tab_view branch September 11, 2024 17:52

Pangoraw mentioned this pull request Sep 11, 2024

adw: add option to choose toolbar style #2219

Merged

mitchellh mentioned this pull request Sep 12, 2024

apprt/gtk: hide header bar when window decorations are disabled #2222

Merged

Pangoraw mentioned this pull request Sep 13, 2024

apprt/gtk: respect window-new-tab-position again #2240

Merged

mitchellh mentioned this pull request Sep 22, 2024

ability to combine multiple windows into one #1122

Open

Pangoraw mentioned this pull request Sep 23, 2024

apprt/gtk: set tabs reorderable/detachable on GtkNotebook #2288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gtk: use Adwaita TabView when possible #2051

gtk: use Adwaita TabView when possible #2051

Pangoraw commented Aug 6, 2024 •

edited

Loading

kareigu Aug 6, 2024

Pangoraw Aug 6, 2024 •

edited

Loading

kareigu Aug 6, 2024 •

edited

Loading

rockorager commented Aug 6, 2024

jcollie commented Aug 6, 2024

tristan957 commented Aug 7, 2024

rockorager commented Aug 9, 2024 via email

tristan957 commented Aug 9, 2024 •

edited

Loading

tristan957 commented Aug 9, 2024

jcollie commented Aug 9, 2024

tristan957 commented Aug 9, 2024 •

edited

Loading

jcollie commented Aug 9, 2024

jcollie commented Aug 9, 2024

Pangoraw commented Aug 16, 2024

Pangoraw commented Aug 20, 2024

jcollie commented Aug 20, 2024

Pangoraw commented Aug 20, 2024 •

edited

Loading

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024

Pangoraw commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024 •

edited

Loading

mitchellh commented Sep 11, 2024

gtk: use Adwaita TabView when possible #2051

gtk: use Adwaita TabView when possible #2051

Conversation

Pangoraw commented Aug 6, 2024 • edited Loading

kareigu Aug 6, 2024

Choose a reason for hiding this comment

Pangoraw Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

kareigu Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

rockorager commented Aug 6, 2024

jcollie commented Aug 6, 2024

tristan957 commented Aug 7, 2024

rockorager commented Aug 9, 2024 via email

tristan957 commented Aug 9, 2024 • edited Loading

tristan957 commented Aug 9, 2024

jcollie commented Aug 9, 2024

tristan957 commented Aug 9, 2024 • edited Loading

jcollie commented Aug 9, 2024

jcollie commented Aug 9, 2024

Pangoraw commented Aug 16, 2024

Pangoraw commented Aug 20, 2024

jcollie commented Aug 20, 2024

Pangoraw commented Aug 20, 2024 • edited Loading

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024

mitchellh commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024

Pangoraw commented Sep 11, 2024

tristan957 commented Sep 11, 2024

mitchellh commented Sep 11, 2024 • edited Loading

mitchellh commented Sep 11, 2024

Pangoraw commented Aug 6, 2024 •

edited

Loading

Pangoraw Aug 6, 2024 •

edited

Loading

kareigu Aug 6, 2024 •

edited

Loading

tristan957 commented Aug 9, 2024 •

edited

Loading

tristan957 commented Aug 9, 2024 •

edited

Loading

Pangoraw commented Aug 20, 2024 •

edited

Loading

mitchellh commented Sep 11, 2024 •

edited

Loading