Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
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

Copy link
Contributor

@chrisduerr chrisduerr left a comment

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
Copy link
Member Author

MarijnS95 commented Nov 10, 2020

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
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
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>.
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.

3 participants