Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

destroy event never called #266

Open
sanpii opened this issue Feb 17, 2021 · 11 comments · May be fixed by #267
Open

destroy event never called #266

sanpii opened this issue Feb 17, 2021 · 11 comments · May be fixed by #267

Comments

@sanpii
Copy link
Contributor

sanpii commented Feb 17, 2021

use gtk::WidgetExt;
use relm::Widget;

#[derive(relm_derive::Msg, Debug)]
pub enum Msg {
    Destroy,
    DeleteEvent,
}

#[relm_derive::widget]
impl Widget for Win {
    fn model() -> () {
    }

    fn update(&mut self, event: Msg) {
        dbg!(&event);

        if matches!(event, Msg::DeleteEvent) {
            gtk::main_quit();
        }
    }

    view! {
        gtk::Window {
            destroy(_) => Msg::Destroy,
            delete_event(_, _) => (Msg::DeleteEvent, gtk::Inhibit(false)),
        }
    }
}

fn main() {
    Win::run(()).expect("Win::run failed");
}
$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/rust`
[src/main.rs:17] &event = DeleteEvent

The same program without relm:

use gtk::prelude::*;

fn main() {
    gtk::init().unwrap();

    let window = gtk::Window::new(gtk::WindowType::Toplevel);

    window.connect_destroy(|_| {
        dbg!("destroy");
    });
    window.connect_delete_event(|_, _| {
        dbg!("delete event");
        gtk::main_quit();
        gtk::Inhibit(false)
    });

    window.show_all();
    gtk::main();
}
$ cargo run
   Compiling rust v0.1.0 (/home/sanpi/test/rust)
    Finished dev [unoptimized + debuginfo] target(s) in 3.37s
     Running `target/debug/rust`
[src/main.rs:21] "delete event" = "delete event"
[src/main.rs:18] "destroy" = "destroy"
@antoyo antoyo linked a pull request Feb 17, 2021 that will close this issue
@antoyo
Copy link
Owner

antoyo commented Feb 17, 2021

After some investigation, here's what is happening:

  1. gtk::main_quit() ends the main loop.
  2. This triggers the destruction of the EventStream's.
  3. There is still the Destroy message in the EventStream.
  4. This message is never sent to the component.

I opened #267 with a fix. Can you test it please?

@sanpii
Copy link
Contributor Author

sanpii commented Feb 17, 2021

It works, it doesn't fix my real problem, but it's a good start 😃

@antoyo
Copy link
Owner

antoyo commented Feb 17, 2021

Err, is your real problem related to relm?
If so, what is it?

@sanpii
Copy link
Contributor Author

sanpii commented Feb 17, 2021

Err, is your real problem related to relm?
If so, what is it?

I don’t know. The bugs appeared with the latest version.

The first one is due to an unowned widget: sanpii/effitask@9fef741

The second is still under investigation. This signal panics (Trying to call emit() on a dropped EventStream) when I quit the application.

@antoyo
Copy link
Owner

antoyo commented Feb 17, 2021

Yeah, those are a different issue and indeed caused by a bug that was fixed in relm.

This is discussed in the blog post announcing this new version.

Basically, before version 0.21, the EventStream's were leaked and thus, never dropped.
So, now you have to save the components somewhere, like you did in sanpii/effitask@9fef741 , so that they don't get dropped early.

It might not always be obvious how to fix it, but if you need more details about this change, please ask me.

By the way, I made this a panic!() so that messages don't get silently dropped, but if you can think of a better solution, I'm all ears!

@sanpii
Copy link
Contributor Author

sanpii commented Feb 17, 2021

I rewrite the widget with multiple widgets view: https://github.com/sanpii/effitask/blob/logger/src/logger.rs

It’s cleaner! But still panic… I am going to write an simple application to reproduce this bug.

By the way, I made this a panic!() so that messages don't get silently dropped, but if you can think of a better solution, I'm all ears!

Logging an error, like gtk does with critical errors?

@antoyo
Copy link
Owner

antoyo commented Feb 17, 2021

I thought about logging, but the log crate won't log anything if the user don't use a logger, so that would should be logged with eprintln!().

Any reason why you prefer logging than a panic!()? The panic gives you a stacktrace of where the error might come from, so I find it convenient.

@sanpii
Copy link
Contributor Author

sanpii commented Feb 17, 2021

A minimalist example to reproduce my bug:

use gtk::prelude::*;
use relm::Widget;

#[derive(relm_derive::Msg)]
pub enum Msg {
    Destroy,
    Quit,
}

#[relm_derive::widget]
impl Widget for Win {
    fn model() -> () {
    }

    fn update(&mut self, _: Msg) {
        gtk::main_quit();
    }

    view! {
        #[name="window"]
        gtk::Window {
            Logger {
            },
            delete_event(_, _) => (Msg::Quit, gtk::Inhibit(false)),
        }
    }
}

fn main() {
    Win::run(()).expect("Win::run failed");
}

#[relm_derive::widget]
impl relm::Widget for Logger {
    fn init_view(&mut self) {
        let label = gtk::Label::new(None);
        self.widgets.list_box.add(&label);
    }

    fn model() -> () {
    }

    fn update(&mut self, _: Msg) {
    }

    view! {
        #[name="toggle"]
        gtk::ToggleButton {
        }

        #[name="popover"]
        gtk::Popover {
            relative_to: Some(&toggle),
            #[name="list_box"]
            gtk::ListBox {
                destroy(_) => Msg::Destroy,
            },
        }
    }
}

Any reason why you prefer logging than a panic!()?

I don’t know if it’s a good idea to crash an application for a non-critical error. Ok, the corresponding action doesn’t work, but you expose users to corrupted data.

The panic gives you a stacktrace of where the error might come from, so I find it convenient.

Do both: panic in debug mode and log (with eprintln if you prefer) in release mode?

In reality, it's just a problem the time it takes to update the application for relm 0.21.

@antoyo
Copy link
Owner

antoyo commented Feb 17, 2021

Note to myself: it could be because of the handling of orphaned widgets.

@sanpii
Copy link
Contributor Author

sanpii commented Feb 18, 2021

The problem persist is a own the label:

diff --git i/src/main.rs w/src/main.rs
index 7019d65..0d8a592 100644
--- i/src/main.rs
+++ w/src/main.rs
@@ -33,11 +33,11 @@ fn main() {
 #[relm_derive::widget]
 impl relm::Widget for Logger {
     fn init_view(&mut self) {
-        let label = gtk::Label::new(None);
-        self.widgets.list_box.add(&label);
+        self.widgets.list_box.add(&self.model);
     }

-    fn model() -> () {
+    fn model() -> gtk::Label {
+        gtk::Label::new(None)
     }

     fn update(&mut self, _: Msg) {

But disappear if I don’t use a popover widget:

diff --git i/src/main.rs w/src/main.rs
index 7019d65..1179b0c 100644
--- i/src/main.rs
+++ w/src/main.rs
@@ -44,17 +44,9 @@ impl relm::Widget for Logger {
     }

     view! {
-        #[name="toggle"]
-        gtk::ToggleButton {
-        }
-
-        #[name="popover"]
-        gtk::Popover {
-            relative_to: Some(&toggle),
-            #[name="list_box"]
-            gtk::ListBox {
-                destroy(_) => Msg::Destroy,
-            },
-        }
+        #[name="list_box"]
+        gtk::ListBox {
+            destroy(_) => Msg::Destroy,
+        },
     }
 }

or simply don’t set its relative to:

diff --git i/src/main.rs w/src/main.rs
index 7019d65..c59f0b2 100644
--- i/src/main.rs
+++ w/src/main.rs
@@ -50,7 +50,6 @@ impl relm::Widget for Logger {

         #[name="popover"]
         gtk::Popover {
-            relative_to: Some(&toggle),
             #[name="list_box"]
             gtk::ListBox {
                 destroy(_) => Msg::Destroy,

@antoyo
Copy link
Owner

antoyo commented Feb 18, 2021

The Label is actually not required to trigger this issue. If I remove the init_view() function from the original code, it still panics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants