Replace Granite.DynamicNotebook with Hdy TabView and TabBar

[jeremypw]

Fixes #766

This adapts the DocumentView in Code which is already based on the Hdy widgets.

This mostly reproduces current behaviour except:

• dropping files onto the tab bar now works.
• Duplicating a tab creates the new tab next to the target tab (like in Browser)
• Restorable tabs are handled by TerminalView which only stores the current working directory not the terminal widget (which caused undiagnosable problems when restored to a TabPage)

Not sure whether dropping on tab labels is correctly implemented (or even whether it should be implemented at all). At present the tab dropped on resets and navigates to the dropped location (provided there is no foreground process running). Feedback required.

Note that a lot of terminal noise is produced when a tab is closed involving multiple messages of the type:

*** BUG ***
In pixman_region32_init_rect: Invalid rectangle passed
Set a breakpoint on '_pixman_log_error' to debug


(io.elementary.terminal:63111): Gtk-CRITICAL **: 18:23:47.237: gtk_widget_queue_draw_area: assertion 'width >= 0' failed

As far as I can tell this is an issue with the Hdy widgets themselves - it occurs even if all the handlers of signals relating closing a tab are disconnected. It also occurs in Code.

[jeremypw]

Outstanding issue is a transparent gap between the tab bar and the header bar through which the desktop shows:

This occurs on X and on Wayland, but does not occur in Files and Code which have very similar widgets.

@danirabbit Is this a styling issue or a difference in coding that I can't see?

[jeremypw]

@danirabbit The problem seems to be due to the "terminal-window" styleclass previously applied to the grid containing the header and view. I have removed that class for now. I am not sure what it did or how to fix it.

[danirabbit]

It looks like zoom controls are broken with this branch:

[danirabbit]

@jeremypw proposed a fix here :) #761

[leolost2605]

I think this will cause a memory leak since the lambda keeps a reference on us and the terminal widget so neither of them will be freed :(

See https://gitlab.gnome.org/GNOME/vala/-/issues/1548
And elementary/appcenter#2193 (comment)

[jeremypw]

@leolost2605 Yes, you are right - TerminalWidgets are not being destroyed when tabs are closed. I had similar issues a long time ago with Files widgets and ended up just disconnecting the signals before closing. I'll see if I can find a better way here.

[leolost2605]

@jeremypw for the terminal_widget you can use the instance that's always passed in signal handlers (so something like terminal_widget.child_exited.connect ((instance, any_other_variables_that_are_passed_to_the_signal_handler, ...) => { which might be enough to break the cycle? Atleast if the terminal widgets lifetime doesn't depend on this. If it does then the program local variable is still an issue so maybe add a property in the terminal widget for it? I'm not at all in codebase though so I'm not sure whether that makes any sense here :)

[leolost2605]

I'm currently trying to write the logics behind this down maybe for inclusion in docs.vala.dev and for me to better understand it since it seems to be a pretty big problem especially in widget hierarchies so I'll leave it here might be completely wrong though :)

A word on vala memory management with callbacks.

With signals:
If local variables are used inside signals vala will ref the current this (see glib.signal.connect_object, which is used if no local variables are used vs. glib.signal.connect_data_full which is used if local variables are used). This can cause memory leaks because of reference cycles if the signal is not manually disconnected and the lifetime of the object that the signal was connected to depends on the this lifetime. This is for example the case if the this is a widget and we add another widget to it and connect to a signal of that widget. This is also the case if the object where the signal was connected to itself is a local variable and used within the lambda. The latter case can be easily mitigated by using the instance that will always be given as an (optional) first argument to the signal handler instead of the local variable.

TLDR: If you don't know what you are doing try to avoid using local variables in lambdas.

For other callbacks individual cases have to be looked at. Here it might be worth looking at the c code and what code vala generates. For example

public class Immortal : Object {
    public GTK.SortListModel sorted_items;

    construct {
        var items = new ListStore (typeof(SomeItemClass));
	var sorter = new GTK.CustomSorter (sort_func);
        sorted_items = new GTK.SortListModel (items, sorter);
    }

    private int sort_func (Object? obj1, Object? obj2) {
	//Sort this stuff
        return 0;
    }
}

If you use this class in you GTK App for example to display a list and then the user navigates somewhere else and the list should be destroyed and freed. Then this class will never be freed even if e.g. the widget that holds a reference on this will be freed. This is because the sorter "holds" a reference on this and we hold a reference on it (via the sorted_items model). To break this cycle we can add a method

    public void cleanup () {
        sorted_items = null;
    }

which should be called by the destructor of the class that uses this class (e.g. the listview that displays our items).

[jeremypw]

@Marukesu Just noticed that the test pass in two of the builds but not in the development-target build. I have not idea why (It pass locally on OS8)

[Marukesu]

The tests look alright, the last fail is not consistent, and i believe there's no way to get the CI's coredumps to understand what happened there, sadily.

fixed

outdated

[danirabbit]

LGTM 🎉

[jeremypw]

@Marukesu Is this OK to merge now, do you think?

[ryonakano]

@jeremypw Thank you for this! Could you add src/Widgets/TerminalView.vala to POTFILES so that we can translate menu items?

[jeremypw]

@ryonakano Done! Sorry - I always forget to do that ☹️