Make some dialogs compatible with GTK4

[bhennion]

This is an attempt to adapt our handling of dialogs, and make it compatible with Gtk4 (different from @Febbe's in the gtk4 branch).

As mentionned in #2722, the main problem is that dialogs in Gtk4 are not blocking. The philosophy is: noone should ever block the main loop, if you're waiting for something, make your response a callback.

I propose here a wrapper around Dialogs: it behaves like a smart pointer, but transfers the Dialog's destruction to the associated window's closing signal once the dialog is shown.

I also change GtkDialog's into GtkWindow's, as GtkDialog's are deprecated in Gtk4.

For now, I implemented the changes to AboutDialog, FillOpacityDialog, ExportDialog, GotoDialog and FormatDialog.
Except for the first one, the commits are independent and can be reviewed one by one.

Those dialogs should compile fine with GTK4, but some properties set in the UI files will need to be replaced when the switch actually happens (e.g. a widget-based <property name="has-default">true</property> will need to be replaced by a window-wide <property name="default-widget">widgetID</property>).

I'll work on more dialogs in a follow up PR.

Edit: updated PR description

[Febbe]

I must look into it deeper tomorrow. But a general question, why do we still need inheritance here. I would love to get rid of the GladeGui class, and the dialog class should also be overkill. Instead I would prefer a general dialog generator, which only depends on the handler and optionally the glade file.

Also, how do you want to handle functions, which return a value from the dialog, after blocking on it? We must basically rewrite all those functions to some sort of state machine, keeping track of the dialogs state.

With c++20 we could use coroutines instead. They wouldn't touch our control flow.

[bhennion]

I must look into it deeper tomorrow. But a general question, why do we still need inheritance here. I would love to get rid of the GladeGui class, and the dialog class should also be overkill. Instead I would prefer a general dialog generator, which only depends on the handler and optionally the glade file.

Yes, GladeGui seems redundant. The Dialog base class is used for its destructor:

Dialog::~Dialog() {
    std::cout << "deleting Dialog" << std::endl;
    gtk_window_destroy(GTK_WINDOW(this->window));
}

The actual window only gets destroyed if the Dialog gets destroyed, so any dev could spot if there were a data leak (there'd still be a window...).

Also, how do you want to handle functions, which return a value from the dialog, after blocking on it? We must basically rewrite all those functions to some sort of state machine, keeping track of the dialogs state.

The callback should rely on two different set of variables:

1. the ones we use to get from the dialog itself (e.g. the state of a spin button and whatnot)
2. the ones we get from the original caller's local variables.

Dummy example

void caller() {
    int status = 34;
    SomeDialog dlg(...);
    dlg.show(...);
    double value = dlg.getValue();

    whatever_should(status);
    become_part_of_the_callback(value);
}

So here, value is of type 1. and status is of type 2.

Type 1. is no issue, we already have the getters for those, and dlg itself can easily be passed on to the callback (it outlives the callback).
Type 2. is more complicated. For now, I only implemented the case where the Type 2. variables were actually global app variables (e.g. a ref to Control or so). Then, no lifetime issues. In general, those Type 2. variables should be passed on to the callback in a state structure: using a coroutine's state for that seems a good solution.

With c++20 we could use coroutines instead. They wouldn't touch our control flow.

I am not sure this changes the structure of Dialog and descendents. The point with using coroutines here is, I guess, that we can give a coroutine's resume() as a callback function for our dialogs.
Or did you imagine something else?

[bhennion]

As a side question: are we ready to use coroutines in terms of compiler support?
GCC and MSVC seem ok, but Clang seems to have a problem on Windows (see here)

[Febbe]

As a side question: are we ready to use coroutines in terms of compiler support?
GCC and MSVC seem ok, but Clang seems to have a problem on Windows (see here)

Regarding windows, I can tell that it works, we are using clang-cl as compiler and asio::awaitable at my company. Seems like we did not hit those Abi missmatches yet. Also the question is, if our target systems support those new compilers.
Microsoft Win 11 is not a problem and Mac OS should also support it. But those many Linux derivates are problematic. Debian Buster for example does not support it natively. But we could install a own libc++ or libstdc++.

[Febbe]

I must look into it deeper tomorrow. But a general question, why do we still need inheritance here. I would love to get rid of the GladeGui class, and the dialog class should also be overkill. Instead I would prefer a general dialog generator, which only depends on the handler and optionally the glade file.

Yes, GladeGui seems redundant. The Dialog base class is used for its destructor:

Dialog::~Dialog() {
    std::cout << "deleting Dialog" << std::endl;
    gtk_window_destroy(GTK_WINDOW(this->window));
}

The actual window only gets destroyed if the Dialog gets destroyed, so any dev could spot if there were a data leak (there'd still be a window...).

Ok, but those handlers could also know, that it has to handle the superclass, therefore the base lass could be completely protected.

Also, how do you want to handle functions, which return a value from the dialog, after blocking on it? We must basically rewrite all those functions to some sort of state machine, keeping track of the dialogs state.

The callback should rely on two different set of variables:

1. the ones we use to get from the dialog itself (e.g. the state of a spin button and whatnot)
2. the ones we get from the original caller's local variables.

Dummy example

void caller() {
    int status = 34;
    SomeDialog dlg(...);
    dlg.show(...);
    double value = dlg.getValue();

    whatever_should(status);
    become_part_of_the_callback(value);
}

Value is not initialized yet. The dlg may not even spawned at that point. And how do we want to handle the case, where caller wants to return a value determined by the Dialog?

So here, value is of type 1. and status is of type 2.

Type 1. is no issue, we already have the getters for those, and dlg itself can easily be passed on to the callback (it outlives the callback).
Type 2. is more complicated. For now, I only implemented the case where the Type 2. variables were actually global app variables (e.g. a ref to Control or so). Then, no lifetime issues. In general, those Type 2. variables should be passed on to the callback in a state structure: using a coroutine's state for that seems a good solution.

Without coroutines, those values must be moved into the dialogs own state or must live in the surrounding newly written state Maschine. Which itself should be managed via the lifetime of the dialog(to prevent a global state for the state machine@ ).

With c++20 we could use coroutines instead. They wouldn't touch our control flow.

I am not sure this changes the structure of Dialog and descendents. The point with using coroutines here is, I guess, that we can give a coroutine's resume() as a callback function for our dialogs.
Or did you imagine something else?

The code will look like this for a caller with an value return:

xoj::task<double> caller() {
    int status = 34;
    SomeDialog dlg(...);
    double value = co_await dlg.async_show(...);
    whatever_should(status);
    co_return value;
}

xoj::task must implement the coroutine interface of cause.

[bhennion]

Dummy example

void caller() {
    int status = 34;
    SomeDialog dlg(...);
    dlg.show(...);
    double value = dlg.getValue();

    whatever_should(status);
    become_part_of_the_callback(value);
}

Value is not initialized yet. The dlg may not even spawned at that point. And how do we want to handle the case, where caller wants to return a value determined by the Dialog?

I wasn't clear here: this is an example as in master: dlg.show() is blocking (so value is initialized). We want to adapt this.

[Febbe]

Dummy example

void caller() {
    int status = 34;
    SomeDialog dlg(...);
    dlg.show(...);
    double value = dlg.getValue();

    whatever_should(status);
    become_part_of_the_callback(value);
}

Value is not initialized yet. The dlg may not even spawned at that point. And how do we want to handle the case, where caller wants to return a value determined by the Dialog?

I wasn't clear here: this is an example as in master: dlg.show() is blocking (so value is initialized). We want to adapt this.

Ah ok, this example is not problematic I think. Because we are here in the C domain and those gtk calls only support pointers, not small types, you must create either a data/handler class, which you pass as pointer, allocated on the heap, or you allocate the callback (a lambda with captures) directly in dynamic storage.

The more problematic cases are all those, where the result is used after the return of the control flow.
Something like that:

class A{
void get_foo(){
  FooDialog f{member_foo};
  f.show;
  member_foo = f.getFoo()
}

void do_task_with_double_member_foo(){
  getFoo();
  control.foouse(member_foo)
}
  double member_foo;
};

Or simple calls, which return the result of the Dialog

Foo getFoo(){
  FooDialog d;
  d.show();
  return d.getFoo();  
}

[bhennion]

I think I got to a structure that feels right and fits all cases I've encountered so far.
I have not encountered situations like the ones @Febbe described until now, but I suspect we will have issues in places where we rely on a GtkFileChooserDialog.

[Febbe]

I think I got to a structure that feels right and fits all cases I've encountered so far.
I have not encountered situations like the ones @Febbe described until now, but I suspect we will have issues in places where we rely on a GtkFileChooserDialog.

Yes, your code looked also relatively good so far. Cases like the ones GtkFileChooserDialog can still be done later I think.

[bhennion]

I rebased on master, integrated @Febbe's comment, removed the debug outputs and updated the PR description.

I think this is ready for a full review (and some testing). I'll keep working on new dialogs in a follow-up PR (to keep things reviewable).

[Febbe]

@rolandlo do you have time to test this in production? I don't have much time because I'm writing my thesis right now. But I can still do code reviews.

[rolandlo]

@rolandlo do you have time to test this in production? I don't have much time because I'm writing my thesis right now. But I can still do code reviews.

Yes, I can test it thoroughly.

[lb90]

Hello! Showing dialogs without nested loops is definitely the best way forward. Anyway, as a porting aid, gtk_dialog_run() can be reintroduced as in https://gitlab.com/inkscape/inkscape/-/merge_requests/5373

https://gitlab.com/inkscape/inkscape/-/blob/827e13285f/src/ui/dialog-run.cpp#L10

[bhennion]

I rebased onto the latest master and tested a little bit.
Here is a testing checklist for generic dialogs, should anyone test this:

1. Check that the dialog's specific functionalities work (duh)
2. If applicable: check that the default active widget is the right one (e.g. the only text field of the dialog, or the only button).
3. Check that the window is modal (always on top)
4. Check that pressing Esc, clicking the cross-button or the cancel button closes the dialog without doing anything unwanted.
5. Check that pressing Enter validates the dialog if it feels like it should (e.g. not if the dialog contains a multiline text box).

[bhennion]

Alright, I think this is stable enough and up for review.

[rolandlo]

Why are there minimize and maximize buttons (which do not work)?

[rolandlo]

There are some rounding errors for the opacity value in the fill opacity dialog. When I close and reopen the dialog, I would expect to get the same value displayed again.

[rolandlo]

Other than the (superfluous and non-working) minimize and maximize buttons I didn't find a regression from this PR. The missing accuracy in the fill opacity dialog was already present before (although slightly differently it seems).

On a positive note: My stylus interacts with the dialogs (in particular with the buttons) just fine, which was mostly not the case with the old dialogs.

I have also noticed that the new dialogs can be moved and the main window can be resized while the dialogs are open. That looks advantageous to me, but I don't know if it was intended.

[bhennion]

Why are there minimize and maximize buttons (which do not work)?

Strangly enough, I do not get the same thing: I have a minimize button, which minimizes the entire app (dialog+main window), but no maximize button

I'm on KDE - X11, and this behaviour is exactly the same as what I have on master.
To be clear, those buttons did not appear for you with the master branch?

[bhennion]

The missing accuracy in the fill opacity dialog was already present before (although slightly differently it seems).

This comes from the fact that the dialog displays a value /100, while the stored data is /255. There is a rounding error there.
Before this PR, the conversion was done by direct casting, and now it uses std::round. This explain the tiny evolution.

We could replace the displayed 100 by 255 if you think this'd look cleaner.

[bhennion]

Did some more tests:

• KDE - Wayland: minimize button minimizes the dialog only, no maximize button -- same as master
• KDE - X11: minimize button minimizes the entire app, no maximize button -- same as master
• Gnome - Wayland: no minimize button, no maximize button -- same as master
• Gnome - X11: minimize and maximize buttons, with no effects -- same as master

[rolandlo]

Did some more tests:

* KDE - Wayland: minimize button minimizes the dialog only, no maximize button -- same as master

* KDE - X11: minimize button minimizes the entire app, no maximize button -- same as master

* Gnome - Wayland: no minimize button, no maximize button -- same as master

* Gnome - X11: minimize and maximize buttons, with no effects -- same as master

Sorry for the confusion. I started your PR from VS code console, which uses GDK_BACKEND=x11 whereas I started master from a wayland session.

On x11 I see whatever Gnome Tweak tools has set

On wayland I only have the close button regardless of the settings in the Gnome tweaks tool.

[rolandlo]

The relevant settings in the Gnome tweaks tool can also be obtained via

gsettings get org.gnome.desktop.wm.preferences button-layout

[bhennion]

Rebased onto master. Merging in 48 hours unless an objection is raised.
@rolandlo I don't think there is anything I can do about how the DE decides to implement modal windows.

[rolandlo]

Rebased onto master. Merging in 48 hours unless an objection is raised. @rolandlo I don't think there is anything I can do about how the DE decides to implement modal windows.

Ok, I'm fine with that.