Add is_* and as_* methods to the event enums

[joshka]

Often application code only cares about a small subset of possible
events. These methods make it simpler to write code which checks whether
an event is a particular event type or converts events into the specific
type (returning an Option).

A simple example of this is waiting for any key, which now becomes:

while !event::read()?.is_key_press() {}

This can help simplify some nested match blocks. E.g.:

match event {
    Event::Key(key) if key.kind == KeyEventKind::Press => { ... }
	_ => {}
}

becomes:

if let Some(key) = event.as_key_press() { ... }

Similar flexible methods are aded across all the event enums:

• Event::is_focus_gained()

• Event::is_focus_lost()

• Event::is_key()

• Event::is_mouse()

• Event::is_paste()

• Event::is_resize()

• Event::is_key_press()

• Event::as_key_press() -> Option<&KeyEvent>

• MouseEventKind::is_*()

• MouseButton::is_*()

• KeyEventKind::is_*()

• KeyEvent::is_press()

• KeyEvent::is_release()

• KeyEvent::is_repeat()

• KeyCode::is_*()

• KeyCode::is_function_key(n)

• KeyCode::is_char(c)

• KeyCode::as_char() -> Option<char>

• KeyCode::is_media_key(media)

• KeyCode::is_modifier(modifier)

[joshka]

There is an in-flight PR in derive_more that would be also be useful. It adds an AsVariant derive. This would make it easy to deal with e.g. if let Some(mouse) = event.as_mouse() { ... } etc. I'd like to consider waiting for that and adding it to this PR.

Obviously a proper match statement is much better when dealing with more than one event type, but there are many use cases that this change would make simpler.

[joshka]

The derive_more change seems stalled for now, so I added the as_* functions manually.

[TimonPost]

Nice! Helper functions always good to have :)

[archseer]

I'm against this change. It introduces heavy additional proc macro dependencies that pad out compile time to save a couple lines of code: dependencies like derive_more have no place in libraries. It'll increase compile time for us downstream on helix for functions we don't even intend on using.

These types of helpers were already possible with a few lines of user code:

fn as_key_press(event: &Event) {
  ...
}

if let Some(key) = as_key_press(&event) { ... }

At the very least it'd be nice if these methods were behind an optional feature flag (default to on) so we can turn them off.

[joshka]

Can you quantify the difference in compile times?

[the-mikedavis]

Some numbers running locally with a fresh project...

1. cargo new --bin example, cd example
2. cargo add --git https://github.com/crossterm-rs/crossterm crossterm --rev <rev>
   • before this change: 1bcfa9729c8a08b78e236271efe989e766da2926
   • after: e063091312d25c21095e69ef547abc00a90ebf7d
3. time cargo c

before:

real	0m1.851s
user	0m4.356s
sys	0m1.640s

after:

real	0m3.539s
user	0m4.650s
sys	0m0.996s

[joshka]

Quantifying the compile times on an Macbook M2:

Before this change

git switch --detach 1bcfa972 
HEAD is now at 1bcfa97 Fix build failure with event-stream, rustix, and use-dev-tty (#955)

cargo clean; time cargo build
...
cargo build  1.95s user 0.59s system 125% cpu 2.016 total

After

git switch master
Previous HEAD position was 1bcfa97 Fix build failure with event-stream, rustix, and use-dev-tty (#955)
Switched to branch 'master'
Your branch is up to date with 'upstream/master'.

cargo clean ; time cargo build
cargo build  2.51s user 0.75s system 121% cpu 2.688 total

572ms extra on a 2 year old laptop doesn't give me any concern about compile times. Help me understand the rationale for your concern here. The procmacro2/syn/quote libraries are already in your dependency tree for helix and the derive_more macro dependency only brings in the macros for the specific derives.

cd helix
cargo clean; time cargo build
cargo build  74.48s user 9.25s system 344% cpu 24.286 total

(that's on the second build after everything is downloaded).

[the-mikedavis]

Should we eventually remove serde and others that make proc macros necessary then we'd be stuck on tearing this out of crossterm as well.

It's just needless for a library to require these extra dependencies for a cosmetic improvement for UX, and IMO isn't justified (https://github.com/crossterm-rs/crossterm?tab=readme-ov-file#dependency-justification). Libraries should have a small dependency footprint. Adding functions like these sounds good to me but adding a dependency to make it happen isn't necessary.

[joshka]

To quantify doing this as functions, there are 32 variants where these functions would be added, each would be 4 lines of code including doc comment. That's 128 lines of code that this makes not necessary to read / bug check / maintain / ensure is consistent when adding new variants / ...

The tradeoff here is the cost to maintain boilerplate code. I don't generally think it's worth it to save < seconds of compile time to avoid using a library in comparison to the reduction of cognitive load and maintenance burden like the derive macros used here do. That's my specific tradeoff calculus, which I'd apply in general terms in most situations unless there's some compelling argument otherwise.

I'm not the main maintainer here, so I'll defer to Timon on making the final call, but I believe that there's reasonable justification here to use this approach.

[the-mikedavis]

We just have a different tradeoff calculus as you say - we can probably agree to disagree on whether this is justified. I hate to be preachy/normative but for libraries I think it's 'proper"/important to be judicious about dependencies because you're forcing your cost-benefit tradeoff on any consuming libraries and applications. An alternative to reverting or explicitly implementing these dependencies we'd be happy with is gating this behind a feature flag (ideally default=off). As you say though it's up to Timon.

[joshka]

One other part of dependency calculus (though more informative than normative) I generally pay attention to is the relative usages of each library. Derive_more has 6,469,406 downloads per month while crossterm has 1,857,699 downloads per month (and the crate usages have similar diffs). These numbers suggest there's a reasonable amount of safety that comes from choosing to use this library. It's well supported by users across the rust ecosystem and shouldn't generally be avoided.

The rationale for making these on by default is that these patterns are useful for pretty much all applications. As a maintainer of Ratatui, I've seen a heck of a lot of TUI applications that our users have built with the combination this library and ours. These changes would simplify the logic in many many cases. I understand that helix is a popular editor, but it's a single data point in comparison to thes.

Also, I helix is in a situation where the majority of your users install the product from a package manager where the compilation step has already been done. Would that be a correct assumption, or am I missing something? Where specifically are you trying to save compile time?

[the-mikedavis]

I can't speak for @archseer but I'm pretty sure what he's saying about compile time part is more of a general concern about taking dependencies. We already have the proc macro stuff in our tree so that isn't a concrete problem. We take issue with being forced to take unnecessary dependencies transitively. These functions might be useful for the out-of-the-box story for ratatui but the functions are unnecessary for us, and IMO it's unreasonable to pass on the cost of another dependency, no matter how common in the ecosystem, with no way to opt-out, when it's not actually important for core functionality.

[joshka]

Taking a step away from whether the added dependency is ok here and just focusing on the usefulness part of this...

My comments about usefulness in Ratatui apps is that these functions simplify any usage of the event functions of crossterm. I'm looking at that through the lens of evidence gleaned from being a library maintainer and viewing many example cases, but to be clear, the usages are in using Crossterm, and are not anything Ratatui specific.

The helix shaped lens you're viewing the usefulness of this seems a bit narrow to me. Most applications built on crossterm are not editors, and hence don't have or need a comprehensive key mapping layer like helix's built on top of the crossterm layer. The importance of this code for core functionality comes down to having code which is easy to use and reason about as a user of this crate in the general case. These sorts of affordances on enums are something that generally support that idea, and these affordances should be provided more often in libs than is current practice.

And back to the dependency part of this...

One of the main arguments about not adding extra deps is that it can make it difficult because things which appear in the external public api of a library tend to constrain usages of the library. Here the generated code does not expose any derive_more types externally. It's pure code generated in an implementation block of each enum. There's no extra trait or anything like that. Compile time dependencies like this are generally a good thing.

The other main argument against deps is that you put yourself in a situation much like the left-pad debacle from years ago, where the lib is yanked causing havoc, or in an xz situation where the code is compromised by a bad actor. While there's a small risk of both of this, I think the rust ecosystem seems to be fairly resilient to this sort of problem, so these don't really weigh the scales particularly negatively.

[archseer]

These types are mostly static and don't change often (they're public API after all), so it doesn't make sense to me to dynamically generate the methods on every compile time and have downstream pay the cost of it if we could just expand the macro once and paste them into the codebase. The weight to value ratio of derive_more here is different to something like mio which is critical for the library to function.

[joshka]

These types are mostly static and don't change often (they're public API after all), so it doesn't make sense to me to dynamically generate the methods on every compile time and have downstream pay the cost of it

This same logic could be said for any derive macro invocation (Debug, PartialEq, ...).

we could just expand the macro once and paste them into the codebase.

I value developer maintenance burden significantly higher than compute time where that compute time is not in a path of selling some product. Unless there's some other cost that you're referring to other than time that I'm missing. If you were replacing derive_more with your own code, I still I wouldn't suggest expanding the macro and copying the code, but instead would suggest writing a small macro rule that does the same thing.

The weight to value ratio of derive_more here is different to something like mio which is critical for the library to function.

The derive_more crate feature flag gates its macros so that it only compiles the specific ones selected (the IsVariant derive macro, and eventually the AsVariant macro). That's quite narrow in terms of an dependency use.