`clone!` macro doesn't execute closure

[haecker-felix]

I have a weird case, where the callback closure doesn't gets executed. After playing a bit with the code, I was able to found out that the clone! macro is the issue.

This is the code which doesn't work. No clicked message.

        // save_button
        get_widget!(self.builder, gtk::Button, save_button);
        save_button.connect_clicked(
            clone!(@weak self.widget as widget, @weak self.builder as builder, @strong self.song as song, @strong self.sender as sender => move |_| {
                debug!("clicked");
            }),
        );

After removing @weak self.builder as builder, the closure works, and the message gets printed.

        // save_button
        get_widget!(self.builder, gtk::Button, save_button);
        save_button.connect_clicked(
            clone!(@weak self.widget as widget, @strong self.song as song, @strong self.sender as sender => move |_| {
                debug!("clicked");
            }),
        );

Affected source code: https://gitlab.gnome.org/World/Shortwave/blob/master/src/ui/song_row.rs#L61

[GuillaumeGomez]

The origin of the problem is very likely because self.builder is destroyed which makes it impossible to get retrieved and therefore it is returned before even calling your closure.

I think we should print messages in such cases in debug builds by default. Like I proposed in https://github.com/gtk-rs/glib/pull/596

cc @sdroege @EPashkin

[sdroege]

Yes this is a case of using weak references wrong. You need to ensure that your objects stay alive or otherwise they would be None inside the closure and it directly returns.

I think we should print messages in such cases in debug builds by default.

I disagree with that. This is the expected behaviour of weak references. If there's anything printed I'm ok with having a separate, non-default feature for that.

[haecker-felix]

This doesn't make much sense for me. self.builder shouldn't get destroyed.

https://gitlab.gnome.org/World/Shortwave/blob/master/src/ui/song_row.rs

Can anyone help me to understand it? :thinking:

[GuillaumeGomez]

@sdroege: And that's where I don't agree with you: for the clone macro users, it won't be obvious at all where the issue comes from since it's difficult to find it by just reading the source code since they very likely won't be aware of how it actually works (well, that's not great but it should definitely be taken into account).

Therefore, when running in debug mode, I think it'd be better to enable the feature to have those error messages.

[haecker-felix]

I fixed the issue with this commit:

https://gitlab.gnome.org/World/Shortwave/-/commit/f19eb7b0226c0d223b8127452679c2c84301734b

Though I'm still not sure what caused issue.

[sdroege]

@GuillaumeGomez In that case that either means that the clone macro is not to be recommended in this form, or our documentation for weak references has to be improved, or people don't read documentation :)

We could maybe make use of the log crate for this, then it can be enabled/disabled in downstream code as needed? I don't know what the overhead of that is if it's unused, so that needs to be investigated.

[sdroege]

Can anyone help me to understand it? :thinking:

Taking a look after dinner :)

[EPashkin]

@haecker-felix Can you check expansion of wrong version? Ex. with cargo expand

[EPashkin]

also seems you can just move button_stack without adding it to clone!

[sdroege]

@haecker-felix The reason is that the SongRow you create in SongListBox::add_song() is directly dropped at the end of the function. Nothing is keeping it alive. So that then also drops the builder, of which the only strong reference is stored in the SongRow.

I think that hints at a bigger conceptual problem here.

Also, moving a strong reference to the builder into that closure will create a reference cycle between the builder, the button, the closure and again the builder.

[GuillaumeGomez]

We could maybe make use of the log crate for this, then it can be enabled/disabled in downstream code as needed? I don't know what the overhead of that is if it's unused, so that needs to be investigated.

I don't see the point of adding the log crate for that. Also, you can just disable the feature if you don't want it on debug mode. :)

[sdroege]

I don't see the point of adding the log crate for that. Also, you can just disable the feature if you don't want it on debug mode. :)

If you make it a non-default feature then I'm fine with that

[GuillaumeGomez]

I never wanted it as a default feature (except on debug).

[sdroege]

I don't want it as default feature, at all :) That would make the clone macro useless for me as it's going to print stuff in perfectly valid situations.

[haecker-felix]

@haecker-felix The reason is that the SongRow you create in SongListBox::add_song() is directly dropped at the end of the function. Nothing is keeping it alive. So that then also drops the builder, of which the only strong reference is stored in the SongRow.

Ooh... Damn. That makes sense. Thanks for the help. A feature (doesn't matter if default or not) which prints warning messages in those cases would be helpful to prevent this in future.

[GuillaumeGomez]

I don't want it as default feature, at all :) That would make the clone macro useless for me as it's going to print stuff in perfectly valid situations.

What prevents you from disabling it if it's enabled by default on debug? :)

[sdroege]

What prevents you from disabling it if it's enabled by default on debug? :)

Every crate that depends on glib would have to have default-features = false in its Cargo.toml for that to work, otherwise the feature would be enabled.

So if I have a crate that depends on gio, and gio does not have default-features = false in its own Cargo.toml then I wouldn't be able to disable the feature.

[GuillaumeGomez]

But if it's only on debug, who cares? :-/

Anyway, at the minimum, we should add the feature and discuss about making it default later on (and of course make it obvious in the documentation).

[sdroege]

But if it's only on debug, who cares? :-/

It will produce a lot of noise when I'm developing, so I care :)

[GuillaumeGomez]

Ok... I guess it won't be a default feature then. I need to think about how making it obvious not to miss it...

[EPashkin]

Maybe use "env!(GTK_RS_DISABLE)" to disable ?

[sdroege]

As discussed on IRC we would go with using the GLib logging system: https://github.com/gtk-rs/glib/issues/610

That can be enabled/disabled with environment variables btw.