antoyo / relm

Idiomatic, GTK+-based, GUI library, inspired by Elm, written in Rust
MIT License
2.41k stars 79 forks source link

port to latest gtk-rs: help #308

Closed emmanueltouzery closed 9 months ago

emmanueltouzery commented 1 year ago

i'm trying to port relm to the latest gtkrs. my app which is built against gtkrs 0.15.x doesn't start anymore since I upgraded to fedora 38 (which does surprise me.. some assert failing about a glib source being null). I thought that upgrading to the latest grkrs should help.

i can't upgrade to gtkrs 0.16.x because I'm also depending on sourceview4, which supports either 0.15.x or 0.17.x.

So I'm trying to make relm build against gtkrs 0.17.x. One issue is that the TODO in gen_construct_widget "switch to builders." apparently must be implemented now, the old method was removed: https://gtk-rs.org/blog/2023/02/10/new-release.html#new-glibobject-constructors

I tried making this work, but the macro and quoting thing is over my head.

Here's what I did naively:

                     panic!("GTK has not been initialized. Call `gtk::init` first.");
                 }
             }
+
+            let obj_builder = glib::Object::builder::<#struct_name>();
             let parameters = [#(#parameters),*];
-            // TODO: switch to builders.
-            ::gtk::glib::object::Object::new::<#struct_name>(&parameters)
+            for (key, value) in parameters.iter() {
+                obj_builder.property(key, &value as &dyn ::gtk::glib::value::ToValue);
+            }
+
+            obj_builder.build()
         })

So, keep building the parameters out of the quote_spanned but instead of passing it directly, iterate on it. That doesn't work, the error in the generated code is...

error[E0277]: the size for values of type `str` cannot be known at compilation time
   --> projectpad/src/widgets/wintitlebar.rs:314:9
    |
314 |         gtk::HeaderBar {
    |         ^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `str`
    = note: only the last element of a tuple may have a dynamically sized type

Do you have any idea how to proceed? Thank you!

antoyo commented 1 year ago

I'm not exactly sure how to fix that particular issue, but the following would work. Replace parameters by two vectors:

    for (key, value) in gtk_widget.construct_properties.iter() {
        let key = key.to_string().replace("_", "-");
        let mut remover = Transformer::new(MODEL_IDENT);
        let value = remover.fold_expr(value.clone());
        keys.push(Ident::new(&key, struct_name.span()));
        values.push(quote_spanned! { struct_name.span() =>
            &#value as &dyn ::gtk::glib::value::ToValue
        });
    }

and use them as such instead of using a for loop:

            let mut obj_builder = glib::Object::builder::<#struct_name>();
            #(obj_builder = obj_builder.property(#keys, &#values as &dyn ::gtk::glib::value::ToValue);)*
            obj_builder.build()

By the way, when I wrote that TODO, I was referring to specific widget's builders like ButtonBuilder. The idea was that this would check that the property exists at compile-time, which doesn't happen when using Object as the properties are string. But your method works as well.

emmanueltouzery commented 1 year ago

thank you. that took me a little further, sadly I now hit another proc-macro related issue... :(

   |
56 | #[widget]
   | ^^^^^^^^^
   |
   = help: message: `"has-entry"` is not a valid identifier

For now i couldn't track down where that identifier is being declared...

antoyo commented 1 year ago

Sorry, there was a mistake in the code above. That's probably this issue. Here's the new version:

    for (key, value) in gtk_widget.construct_properties.iter() {
        let key = key.to_string().replace("_", "-");
        let mut remover = Transformer::new(MODEL_IDENT);
        let value = remover.fold_expr(value.clone());
        keys.push(key);
        values.push(quote_spanned! { struct_name.span() =>
            &#value as &dyn ::gtk::glib::value::ToValue
        });
    }
emmanueltouzery commented 1 year ago

thank you! that was it, yes...

so now my app builds and starts against gtkrs 0.17.. and sadly the original issue that I was trying to fix is still present. So it was not about the slightly older gtkrs...

Here's the error that I'm getting at startup:

(projectpad:40642): GLib-CRITICAL **: 14:26:53.441: g_source_attach: assertion 'source->context == NULL' failed
thread 'main' panicked at 'assertion failed: `(left != right)`
  left: `0`,
 right: `0`', /home/emmanuel/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.17.9/src/source.rs:50:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

and with the backtrace:


(projectpad:41508): GLib-CRITICAL **: 14:28:24.202: g_source_attach: assertion 'source->context == NULL' failed
thread 'main' panicked at 'assertion failed: `(left != right)`
  left: `0`,
 right: `0`', /home/emmanuel/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.17.9/src/source.rs:50:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:212:5
   4: <glib::source::SourceId as glib::translate::FromGlib<u32>>::from_glib
             at /home/emmanuel/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.17.9/src/source.rs:50:9
   5: glib::translate::from_glib
             at /home/emmanuel/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.17.9/src/translate.rs:1414:5
   6: glib::source::<impl glib::auto::source::Source>::attach
             at /home/emmanuel/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.17.9/src/source.rs:1082:13
   7: relm::core::Channel<MSG>::new
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/core/mod.rs:179:9
   8: <projectpad::widgets::project_list::ProjectList as relm::state::Update>::model
             at ./projectpad/src/widgets/project_list.rs:50:33
   9: relm::create_widget
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/lib.rs:204:17
  10: <W as relm::container::ContainerWidget>::add_widget
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/container.rs:172:47
  11: <projectpad::widgets::win::Win as relm::widget::Widget>::view
             at ./projectpad/src/widgets/win.rs:747:32
  12: relm::create_widget
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/lib.rs:205:22
  13: relm::init
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/lib.rs:288:37
  14: relm::run
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/lib.rs:348:22
  15: relm::widget::Widget::run
             at /home/emmanuel/.cargo/git/checkouts/relm-8ad96e43440f833f/6603842/src/widget.rs:66:9
  16: projectpad::main
             at ./projectpad/src/main.rs:53:5
  17: core::ops::function::FnOnce::call_once
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The line at my main.rs:53 is the initialization of the first widget:

    widgets::win::Win::run((sql_channel, !db_preexisted)).unwrap();

Last line of my main.rs.

So that worked fine on fedora 37 and is now blowing up on fedora 38. And my app still works in flatpak, since the flatpak ships the runtime/libraries together with the app. I'm guessing something changed in glib::Source recently...

antoyo commented 1 year ago

This assert comes from the glib C library:

(projectpad:41508): GLib-CRITICAL **: 14:28:24.202: g_source_attach: assertion 'source->context == NULL' failed

That seems to be this line. Which we call from here. I have a feeling that we should set a context in the source created here. You could try setting the source context to the main context.

emmanueltouzery commented 1 year ago

for now I tried this:

        let mut funcs = Box::new(funcs);
        let source = g_source_new(&mut *funcs, mem::size_of::<SourceData<T>>() as u32);
+       let context: *mut GMainContext = glib::MainContext::default().to_glib_none().0;
+       g_source_attach(source, context);
        ptr::write(&mut (*(source as *mut SourceData<T>)).data, data);
        ptr::write(&mut (*(source as *mut SourceData<T>)).funcs, funcs);
        from_glib_full(source)

But that didn't help.

emmanueltouzery commented 1 year ago

I do see in the apidocs: https://docs.gtk.org/glib/ctor.Source.new.html

The source will not initially be associated with any GMainContext and must be added to one with g_source_attach() before it will be executed.

However the apidocs also say: https://docs.gtk.org/glib/method.Source.attach.html

A GMainContext (if NULL, the global-default main context will be used) The argument can be NULL.

So I tried with NULL too, but that didn't help:

        g_source_attach(source, 0 as *mut GMainContext);
antoyo commented 1 year ago

I think the assert is actually the opposite of what I thought. It looks like we try to attach a context multiple times on the source. You could try removing that line. If that doesn't work, you'll need to check if we add it at other places.

emmanueltouzery commented 1 year ago

the current status is that something else breaks if i just comment that line, which led me to think that it's at least sometimes needed. So I did that:

        let main_context = MainContext::default();
        if unsafe {glib_sys::g_source_get_context(source.as_ptr()) == std::ptr::null_mut()} {
            eprintln!("adding src context");
            source.attach(Some(&main_context));
        } else {
            eprintln!("NOT adding src context");
        }

and I got that output:

adding src context
adding src context
adding src context
adding src context
NOT adding src context
adding src context
adding src context
adding src context
NOT adding src context
adding src context
adding src context
adding src context
adding src context

(projectpad:164851): GLib-CRITICAL **: 19:34:10.460: source_remove_from_context: assertion 'source_list != NULL' failed

(projectpad:164851): GLib-CRITICAL **: 19:34:10.460: g_hash_table_remove_internal: assertion 'hash_table != NULL' failed
[1]    164851 segmentation fault (core dumped)  cargo run --bin projectpad

now the app actually starts, but it crashes when exiting.

antoyo commented 1 year ago

At that point, I'm not sure.

It looks like the GSource is not initialized correctly, somehow.

If you cannot figure out why it goes in the else, I suggest you ask people that knows the GLib C library.

emmanueltouzery commented 1 year ago

unfortunately, to be completely honest, at this point it's probably more likely that i'll attempt to port my application to relm4/gtk4 :(

antoyo commented 1 year ago

I'm now getting segfaults on basic relm examples. The segfault seems related to GSource stuff, so it might be related to your issues.

Still not sure what is going on…

antoyo commented 1 year ago

I found the problem. This struct needed a #[repr(C)]. I released a new version with it; you can try rebasing on it to see if this fixes your problem.

You should not need the conditional stuff of the last comments.

antoyo commented 9 months ago

Closing. Please reopen if you still have an issue.