jazzfool / reclutch

Rust UI Core
Other
157 stars 4 forks source link

introduce Advanced Event API #4

Closed fogti closed 5 years ago

fogti commented 5 years ago

This PR should solve some things brought up in previous PRs.

fogti commented 5 years ago

Currently the documentation of the reclutch_event crate is suboptimal. Missing pieces:

There are basically two different usages (supported as part of this PR):

And 2 support utilities to handle multiple event queues:

Additional notes:

fogti commented 5 years ago

@jazzfool is this ok? when this PR is mergable, I'll squash the commits together (the internal design changed in these commits sometimes back-and-forth, which isn't worth keeping in the git history).

fogti commented 5 years ago

Another usage example/use case for the cascading API: forward global mouse events to the appropriate Widget event queue.

jazzfool commented 5 years ago

Thanks for all the work. This looks good. We definitely need documentation on the different types of events, perhaps with example scenarios so that the user can pick the correct event type.

fogti commented 5 years ago

Another extension: a NonRcEvent, like RcEvent, but without reference counting and without Clone support.

fogti commented 5 years ago

I tried using NonRcEvent in the example, but I couldn't resolve the following error, so I skipped that for now.

error[E0597]: `window_q` does not live long enough
   --> reclutch/examples/counter/main.rs:195:36
    |
195 |     let mut counter = Counter::new(&window_q);
    |                       -------------^^^^^^^^^-
    |                       |            |
    |                       |            borrowed value does not live long enough
    |                       argument requires that `window_q` is borrowed for `'static`
...
241 | }
    | - `window_q` dropped here while still borrowed

error[E0505]: cannot move out of `window_q` because it is borrowed
   --> reclutch/examples/counter/main.rs:198:20
    |
195 |     let mut counter = Counter::new(&window_q);
    |                       -----------------------
    |                       |            |
    |                       |            borrow of `window_q` occurs here
    |                       argument requires that `window_q` is borrowed for `'static`
...
198 |     event_loop.run(move |event, _, control_flow| {
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `window_q` occurs here
...
216 |                 window_q.push(GlobalEvent::MouseMove(cursor));
    |                 -------- move occurs due to use in closure

But I fixed an issue in reclutch_derive: previously, it didn't handle lifetimes and resulted in the following API while testing for the other issue:

error[E0726]: implicit elided lifetime not allowed here
  --> reclutch/examples/counter/main.rs:35:8
   |
35 | struct Counter<'global> {
   |        ^^^^^^^- help: indicate the anonymous lifetime: `<'_>`

error[E0726]: implicit elided lifetime not allowed here
   --> reclutch/examples/counter/main.rs:107:8
    |
107 | struct Button<'global> {
    |        ^^^^^^- help: indicate the anonymous lifetime: `<'_>`

error[E0308]: mismatched types
  --> reclutch/examples/counter/main.rs:68:6
   |
68 | impl Widget for Counter<'_> {
   |      ^^^^^^ lifetime mismatch
   |
   = note: expected type `reclutch::WidgetChildren`
              found type `reclutch::WidgetChildren`
note: the lifetime `'_` as defined on the impl at 68:25...
  --> reclutch/examples/counter/main.rs:68:25
   |
68 | impl Widget for Counter<'_> {
   |                         ^^
   = note: ...does not necessarily outlive the static lifetime

Thus, I added support for generics and lifetimes to reclutch_derive.

fogti commented 5 years ago

I noticed that winit has a functionality to push events back into the event loop, and implemented the GenericEventInterface for it (winit is now an optional dependency). This makes cascading even more useful, because the main event loop is now a valid "cascade filter destination".

fogti commented 5 years ago

Another question regarding GenericEventInterface would be: Do we really want to expose listener_len and event_len to the users of the crate? They are not reliable for the more advanced use cases, the only method to reliable detect if a event queue has listeners is trying to push and checking the return value (which is false if no listeners are registered). It could be advisable to replace event_len with an is_empty method to check if any elements are buffered, which would return false if the Event itself doesn't expose this information. listener_len and event_len would still be needed in unit tests, but could be changed to pub(crate) (or something even more restricted) and moved outside of the trait. (done)

The only remaining concern is: should we keep ArcEvent or remove it from the implementation?

fogti commented 5 years ago

It is probably a good idea to drop reclutch_event::traits::private_::Sealed together with the dependent trait impl of GenericEventInterface. The only blocker for this is that ArcEvent is dropped to prevent 3x copies of the impl contents in the code-base.

fogti commented 5 years ago

Regarding bikeshedding: It might be a good idea to rename {Event -> ~Event~Queue} to avoid confusion.

Edit: EventQueue wouldn't be a good idea, because it would be a naming duplication between the crate name and the main types. Thus, I'll probably drop the Event-prefix everywhere it makes sense.

jazzfool commented 5 years ago

I assume the crossbeam-channel is a better fit for multi-threading than the ArcEvent interface, so yes, I would say that ArcEvent is no longer needed. I don't think there are a lot of use-cases with NonRcEvent, simply due to lifetime semantics. Also I agree, EventQueue is a better name.

fogti commented 5 years ago

NonRcEventQueue would be probably usable with pinning, e.g. the inner structures of Windows, if created on-demand (to avoid the issue with the jump into the main winit event loop), could use it. But often it is not worth it, because the performance benefit doesn't outweight the maintainance burden created by it. But it is usable as a temporary event queue.

fogti commented 5 years ago

ok, this PR is now considered feature-complete. I'll squash the commits soon and then mark this as ready for review.

fogti commented 5 years ago

ok, I'm done. Last thing added is a std::sync::mpsc-based Event Queue, which has the drawback that it doesn't support cascades and select.

fogti commented 5 years ago

@jazzfool ping

fogti commented 5 years ago

oh, I just noticed that I'm not completely done.

fogti commented 5 years ago

@jazzfool you should add a label event API or something like that (and probably another label for the reclutch main crate) to make it easier later to search (in the issue list) for bugs in the reclutch-event crate or event queue API.

fogti commented 5 years ago

P.S. Thanks that you introduced glutin+skia into the example code, I'm now finally able to run the example (before that I was unable to do it because my graphics card doesn't support the vulkan API and 'pixels' upstream crates don't fully support OpenGL yet).

jazzfool commented 5 years ago

Thanks again for all the work. I'm trying to maximize hardware compatibility with Skia, the only problem is build compatibility. I've added the labels you suggested too.