Concerns about usage of a ControlFlow type instead of Inhibit.

[andy128k]

GLib/Gtk isn't that consistent when it comes to control flow values. Sometimes FALSE means stop (e.g. g_timeout_add* family) and sometimes it's opposite (e.g. key-pressed signal of GtkEventControllerKey).

There was a change gtk-rs/gtk-rs-core#1126 made recently which unifies these values. A usage of a ControlFlow instead of Inhibit seems unfortunate.

Inhibit(true) became Continue and Inhibit(false) became Break, while it seems more natural to be opposite.

[bilelmoussaoui]

I think the easiest solution without breaking API and also keeping one single enum to handle both Inhibit and Continue, is to make the generated code that used to have Inhibit do a ControlFlow::from(!value).

[awused]

Yeah I just ran into this and was looking for the documentation on ControlFlow vs Inhibit, because I thought I had it right but wanted to be sure. I would have been entirely wrong if I didn't find this bug. The current mapping is backwards and the documentation for ControlFlow does nothing to clear up the confusion.

But I'm not sure that swapping the values after you've already made a release is going to be a good idea, though I think the well has already been poisoned. Unlike with the breaking change to 0.7 where the build broke, going to 0.8 to reverse the meaning of control flow just seems like it's going to make for some really subtle bugs.

I'm personally confident the two separate ideas of "keep calling this closure or not" and "prevent or allow other event handlers from handling the event I have handled" should not even be the same type.

[andy128k]

0.7.0 may be yanked and 0.7.1 released with a fix (with the change reverted or a control flow value negated).

[bilelmoussaoui]

I'm personally confident the two separate ideas of "keep calling this closure or not" and "prevent or allow other event handlers from handling the event I have handled" should not even be the same type.

Maybe, the thing is that in the past it was just a wrapper around a boolean with a unlogical semantics. I still believe having one type for both does make sense to some extent.

0.7.0 may be yanked and 0.7.1 released with a fix (with the change reverted or a control flow value negated).

That sadly won't be enough, as the change affects also https://github.com/gtk-rs/gtk-rs-core/blob/751e1dde2f816add5577cbf597eee81da3aa2531/gio/src/auto/settings.rs#L517 (it is the only use case in gtk-rs-core, thankfully)

[awused]

Personally I hope you opt for a revert instead. It doesn't make sense for me to "continue" or "break" other event handlers so whether it's negated or not there will be more confusion with the new names. I was confused even before I realized the meanings were the opposite of what I thought.

[bilelmoussaoui]

Personally I hope you opt for a revert instead. It doesn't make sense for me to "continue" or "break" other event handlers so whether it's negated or not there will be more confusion with the new names. I was confused even before I realized the meanings were the opposite of what I thought.

A boolean is not any better, and no. I am really not fan of reverting that change

[bilelmoussaoui]

These are the following options we have:

• Revert the changes -> Requires releasing a bunch of crates which were not affected -> a lot of work
• re-introduce Inhibit type again but as an enum instead
• switch back to plain boolean and stop trying to make a messy api any better
• negate the value

[awused]

Maybe, the thing is that in the past it was just a wrapper around a boolean with a unlogical semantics. I still believe having one type for both does make sense to some extent.

A boolean is not any better, and no.

I'd make the case that a boolean is better in this case; true/false doesn't mean much on its own in a program and you have to figure out what it means from context and reading the documentation. "Continue" and "Break" mean something, but their meanings are at a pretty oblique angle to how they're being applied to two different concepts here. Depending on how you twist your perspective, you can make either option make sense for either behaviour in this context.

Edit: Of those options, I think reintroducing Inhibit is the least bad. It cleanly breaks the build, instead of changing runtime behaviour, and the wording better matches what it's doing.

[bilelmoussaoui]

Right... well, we can re-introduce Inhibit but as an enum this time and have something like

pub enum Inhibit {
    Handled,
    NotHandled
}

Not a fan of the variants names though.

[andy128k]

pub enum Inhibit {
    Inhibit,
    Propagate,
}

[andy128k]

This or that I like enums. Inhibit(false) and Continue(false) were very confusing.

[awused]

To be honest when you said "revert" earlier I assumed that meant to revert removing Inhibit from gtk4-rs, not necessarily revert adding ControlFlow to gtk-rs-core. ControlFlow with Continue/Break makes enough sense for a closure that is being repeatedly called once like a loop (from idle or timeout callbacks) and that is the only case the documentation covers.

I'm not sure inhibit needs to be the name of the enum though. Maybe EventPropagation or Propagation as the enum and inhibit as a value.

pub enum EventPropagation {
    Inhibit, (or Stop?)
    Proceed
}

[bilelmoussaoui]

Opinions on gtk-rs/gtk-rs-core#1144 would be appreciated. Note I won't be able to do all the releases today, but will try to reduce the impact as much as possible.

[bilelmoussaoui]

I went ahead and yanked gio (the other crates are not affected by this), the gtk4-rs crates, libadwaita/sourceview5/libshumate