search

bodil / vgtk

A declarative desktop UI framework for Rust built on GTK and Gtk-rs
http://bodil.lol/vgtk/
Other
1.05k stars 37 forks source link

Don't crash when app is actived twice #73

Closed meteficha closed 3 years ago

meteficha commented 3 years ago

Gtk will call the activate signal again if the binary is executed while the app is already running. Ideally one would handle it in a better way, such as by bringing the main application window forward. This commit is a first step in that direction, at least making sure that the app doesn't crash.

pepijndevos commented 3 years ago

Thanks for the PR!

I think the build failures are not your fault, but caused by gtk.rs choking on its own documentation. I had this problem on my local machine, and could only build gtk.rs by disabling the docs. Not sure if disabling docs on CI is a good move.

The clippy error needs fixing though, not sure where that came from. It does not obviously come from your changes as far as I can tell, but I also can't tell where it did come from, besides "inside the macro".

meteficha commented 3 years ago

Thanks for taking a look, @pepijndevos. I don't think the clippy warning comes from my PR either, but I've "fixed" it in commit 3054df9.

pepijndevos commented 3 years ago

Heh, that's quite a crude fix indeed.

Bodil is not giving a lot of attention to this package at the moment, so I'm trying to maintain it a bit, although I barely know what I'm doing. I'll leave this open for a bit to see if anyone has anything to add, and otherwise just merge it.

If you have any idea how to properly fix clippy or the build system, any help is greatly appreciated. I think maybe the gtk.rs bug just needs to be reported upstream. No idea if the clippy error is sensible for macro-generated code, and how to properly ignore it. With your "fix" any user of the library who uses clippy would have to manually ignore the warning.

I found there is https://github.com/dtolnay/cargo-expand which might help debug the clippy error, but this macro stuff is a bit magic to me at the moment. I was trying to fix some other issue originating in the macro, and failing https://github.com/bodil/vgtk/issues/39#issuecomment-755240258

meteficha commented 3 years ago

Agreed that the fix is not great. OTOH, neither of these two clippy issues were caused by cd6bc70, the main commit of this PR. If you're happy with cd6bc70, we can either merge this PR as is, and open issues to remind us to get a better fix for the clippy issue, or I can remove the "fix" and we merge that. What do you think?

pepijndevos commented 3 years ago

I would say undo the clippy "fix" and make an issue to fix the problems for real.

I think the core commit is fine. It just made me wonder why https://doc.rust-lang.org/std/sync/struct.Once.html was not used here. I guess it doesn't offer the panic behaviour, so if it's important that it panics if called twice, this change makes sense.

meteficha commented 3 years ago

@pepijndevos: I've reverted the "fix" and opened #75.

I actually didn't know about std::sync::Once, thanks! It's probably not too important to panic in run_dialog_props. However, one would still need to have some code to work around the fact that in start the closure is FnOnce, so std::sync::Once is not a drop-in replacement. I'd guess that's why the custom once function was written in the first place.

pepijndevos commented 3 years ago

Thanks! Help always welcome to fix the other problems of course. But not crashing is a good start :)