Skip to content

[RFC] event: Unwrap user event in map_nonuser_event Err() - #1761

Closed
MarijnS95 wants to merge 1 commit into
rust-windowing:masterfrom
MarijnS95:map-nonuser-event
Closed

[RFC] event: Unwrap user event in map_nonuser_event Err()#1761
MarijnS95 wants to merge 1 commit into
rust-windowing:masterfrom
MarijnS95:map-nonuser-event

Conversation

@MarijnS95

Copy link
Copy Markdown
Member

⚠️ WARNING ⚠️ : This is a breaking API change!

Currently separating an Event<T> into a user event T and Event<()> is not ergonomic: when map_nonuser_event returns Err() the caller "knows" this can only contain UserEvent(T), yet has to write additional unpacking logic to extract the event, with an unnecessary unreachable!().

Solve this by returning T directly in the Err case, instead of Event<'a, T>.

  • Tested on all platforms changed
  • Compilation warnings were addressed (N/A: there were no compiler warnings in the first place?)
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
    • How about introducing a user event to multithreaded.rs? That is the exact use case we need this for: handle user events (from the thread) in the event loop, and send nonuser events into the thread.
  • Updated feature matrix, if new features were added or implemented

@chrisduerr chrisduerr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would anyone use map_nonuser_event to separate Event<T> into Event<()> and T? The purpose is to map Event<T> to Event<()>, why would that ever return the user event when you're clearly trying to get rid of it?

@MarijnS95

MarijnS95 commented Nov 10, 2020

Copy link
Copy Markdown
Member Author

when you're clearly trying to get rid of it?

@chrisduerr Who says that I'm trying to get rid of them? I want to separate these events out and Result is clearly used here to give me that, for lack of an Either type.

The purpose is to map Event<T> to Event<()>, why would that ever return the user event

It... already does? Just not in a very friendly way?

As explained my usecase is identical to what is going on in multithreaded.rs. Right now I have to write a panicking abomination like:

if let winit::event::Event::UserEvent(thread_event) = event {
    // Received message from thread
} else if let Some(event) = event.to_static() {
    match event.map_nonuser_event() {
        Ok(event) => {
            // Send event to thread
            let _ = event_send.send(event);
        }
        Err(_) => unreachable!(),
    }
}

Or

match event.map_nonuser_event() {
    Ok(event) => {
        // Send event to thread
        if let Some(event) = event.to_static() {
            let _ = event_send.send(event);
        }
    }
    Err(user_event) => match user_event {
        winit::event::Event::UserEvent(thread_event) => {
            // Received message from thread
        }
        _ => unreachable!(),
    }
}

Or any other variation of match/if let you can come up with.

With this change I can get rid of that unnecessary unreachable!() case and simply write:

match event.map_nonuser_event() {
    Ok(event) => {
        // Send event to thread
        if let Some(event) = event.to_static() {
            let _ = event_send.send(event);
        }
    }
    Err(thread_event) => {
        // Received message from thread
    }
}

EDIT: Sure, you can get rid of the unreachable!() by leaving the match arm empty or turning it into an if let without else but that is besides the point: I want to handle all cases without the possibility of accidentally loosing an event in case semantics around these change: this PR allows me to do that without copying the contents of map_nonuser_event (consider there's code handling the case where to_static returns None in these examples, too, but omitted for brevity).

@chrisduerr

Copy link
Copy Markdown
Contributor

As explained my usecase is identical to what is going on in multithreaded.rs

Which doesn't use map_nonuser_event event though?

It seems like your problem is fundamentally with the combination of to_static and map_nonuser_event? Can you provide a minimal working example?

@MarijnS95

Copy link
Copy Markdown
Member Author

Which doesn't use map_nonuser_event event though?

If it were to use user events, then it'd need it to send an Event<'static, ()> into the thread. Hence my request in the PR description to add such an event to this particular example.

It seems like your problem is fundamentally with the combination of to_static and map_nonuser_event? Can you provide a minimal working example?

There is not really a "problem" that this PR attempts to tackle, merely making the resulting Err() value from map_nonuser_event more readily usable. What do you expect to see in a minimal working example that is not covered by the code samples above?

Currently separating an Event<T> into a user event T and Event<()> is
not ergonomic: when map_nonuser_event returns Err() the caller "knows"
this can only contain UserEvent(T), yet has to write additional
unpacking logic to extract the event, with an unnecessary
unreachable!().

Solve this by returning T directly in the Err case, instead of
Event<'a, T>.
@MarijnS95
MarijnS95 force-pushed the map-nonuser-event branch from 4ec0b4a to 769e8b6 Compare July 8, 2021 08:55
@madsmtm

madsmtm commented Mar 17, 2025

Copy link
Copy Markdown
Member

This is redundant after #3694 (part of v0.31), as UserEvent has been removed entirely in favour of a wake-up mechanism instead.

@madsmtm madsmtm closed this Mar 17, 2025
@MarijnS95
MarijnS95 deleted the map-nonuser-event branch April 24, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S - api Design and usability

Development

Successfully merging this pull request may close these issues.

4 participants