Platform-Specific Enum Variants

[daxpedda]

From #3056 (comment):

If we want to continue using the extension trait model for platform-specific features, I came up with the following idea that we could use for enums:

enum Event {
    EventA,
    EventB(String),
    EventC {
        data: u16,
    },
    Platform(PlatformEvent),
}

impl PlatformEventExtWayland for PlatformEvent {
    fn event(&self) -> WaylandEvent {
        self.0
    }
}

// Not really needed, just a neat idea.
impl PlatformEventExtWindows for PlatformEvent {
    fn event(&self) -> ! {
        match self.0 { }
    }
}

Alternatively we can always mark the enum #[non_exhausive] and add cfg-gated variants.

---

For consistency with the current API I would like to go with the first solution. The first thing I would like to apply this to is to re-introduce the ControlFlow enum after #3056.

[daxpedda]

From #3056 (comment):

I will say, I don't think Exit has a place in the enum, it's really a separate thing. If you want to have an enum, I think it should be something like enum NextEvent { Poll, Wait, WaitUntil(Instant) }.

So when we get to re-introduce the ControlFlow enum I would like to specifically tackle this as well.

[dhardy]

From my comment:

An alternative of course could be to simply mark the enum #[non_exhausive] and add cfg-gated variants.

Event should probably be #[non_exhaustive] anyway, just so that Winit v1.1 can add new events.

On the whole I am not a fan of the platform-extension-trait model. For an app targetting only one platform the model is fine, but that's mostly not why you'd use Winit anyway. A cross-platform API should require as little platform-specific code to use as possible. If, say, Event::PinchGesture is only generated on MacOS but exists everywhere, then removing unused handlers should be an optimisation problem only (i.e. the app should be able to just write a cross-platform handler for Event::PinchGesture). Adding #[cfg(macos_platform)] to this isn't a big deal if we must have a minimal API on each platform but is not necessary. (Further motivation for this is that if, later, PinchGesture were supported on Wayland, the only change needed for an app to enable it there too would be a one-line cfg, and only then if a cfg was used.)

This may not even affect Exit since I didn't see any motivation to keep it anyway.

[daxpedda]

Obviously there will always be APIs that should be behind a cfg guard because they don't make sense in a cross-platform API, e.g. EventLoopBuilder::with_msg_hook().

I agree with the overall sentiment, but this is a much bigger issue. Generally speaking I prefer keeping the API consistent over having half of the API follow a different pattern/rule. That said, if we can get consensus right here and now to remove the platform-extension-trait model I'm happy to go with that as well.

@dhardy if you like to start the discussion in a new issue that would be great. I'm happy to do the implementation, but it's a lot of work to collect, read and answer throughout the process.

[kchibisov]

I'm not sure why it has a separate issue, if enum is not a part of event, which a ControlFlow is, simply mark it as non_exhaustive and add make some variants cfg based.

For the event I can't say at the current time what the idea could be, but we have PRs/Issues discussing platform specific event callbacks and so on.

[daxpedda]

We already have a platform-specific event: ActivationTokenDone.

This issue isn't for discussing ControlFlow specifically, it's for the API to have a consistent way to add platform-specific enum variants. If you are happy with using cfgs on ControlFlow, I would argue we should do the same for Event or WindowState.

[kchibisov]

@daxpedda it's not if you look into it, it's not marked as wayland/x11 special.

[kchibisov]

I mean, you've linked to that issue from the ControlFlow PR, so I sort of replied that ControlFlow has nothing to do with the way this issue is being handled, because nothing stops us from having enum variants with cfg, it's something common.

[daxpedda]

@daxpedda it's not if you look into it, it's not marked as wayland/x11 special.

What I'm saying is that it is platform-specific, we just didn't mark it as such. My hope is that if we establish some guidelines how Winit should do this in the future we can determine if we want to guard it behind a cfg or not.

This issue isn't for discussing ControlFlow specifically, it's for the API to have a consistent way to add platform-specific enum variants. If you are happy with using cfgs on ControlFlow, I would argue we should do the same for Event or WindowState.

I mean, you've linked to that issue from the ControlFlow PR, so I sort of replied that ControlFlow has nothing to do with the way this issue is being handled, because nothing stops us from having enum variants with cfg, it's something common.

Are you trying to say that you don't care about the consistency part here? E.g. let's use cfgs on ControlFlow but not on other enums and just do it on a case-by-case-basis?

[kchibisov]

I do care, but Event is more complicated than any other enum and it might have different policy to what the rest is doing. I'm afraid to touch the way Event works, simply because we can't mark it as non_exhaustive yet and some events might depend on other events.

Simply user setters/getters could have a cfg based values, but when enum is passed as part of the API and when some of those things are close to must be handled it's complicated with Event...

Though, I have a goal to split event apart and have normal extension we have already... But regular simple enums could have a policy like cfg on the variant already.

[daxpedda]

Btw I have proposed a way to avoid non_exhaustive in the OP.

I honestly dislike slapping non_exhaustive on ControlFlow way more then on Event, I imagine there are extremely few applications out there who actually handle absolutely all events.

I'm happy to figure this out later if this is not the right time.

[madsmtm]

IIRC the reason Event is not #[non_exhaustive] is because adding an event is very likely to be a breaking change anyhow, a lot of the time it's about moving stuff around, and we can't just e.g. stop delivering an event. Also, I have seen someone express desire for exhaustive enums, so apparently some people are checking all the variants.

[madsmtm]

@daxpedda maybe you could explain the extra variant(s) you'd want to add to ControlFlow? That'd make it easier for me at least to figure out what the best way forward is.

[kchibisov]

The thing is thaw with trait based event dispatching you won't have enums like that at all, so I better not touch the way Event is working now, the reason to mark ControlFlow as non-exhaustive is because you can extend it without issue and the enum itself can't have breaking changes, really, it can only get new non-breaking stuff.

[daxpedda]

@daxpedda maybe you could explain the extra variant(s) you'd want to add to ControlFlow? That'd make it easier for me at least to figure out what the best way forward is.

Just different versions of Poll. Currently Poll is using setTimeout() in a pretty hacky way to avoid throttling or properly using the scheduling API which is currently only available in Chromium based browsers.

The first thing I want to add as soon as possible is requestIdleCallback(), which I consider pretty important for many use-cases. In the future adding access to the other priorities available in the scheduling API would be nice as well.

Upon further contemplation I could have avoided this whole mess by just adding a method called set_poll_type() for Web ...

[madsmtm]

This is also relevant for errors (see also #3067).

Currently we do MyError::Os(OsError), where OsError is platform-specific, but I think MyError::Platform would be nicer?

[kchibisov]

This is solved in the winit-next with the extension traits and vtable registrations. So scheduling it for 0.31.0 where the split design will likely land.