helgoboss / reaper-rs

Rust bindings for the REAPER C++ API
MIT License
82 stars 8 forks source link

Medium-level API: Improve forward compatibility of enums #41

Closed helgoboss closed 3 years ago

helgoboss commented 3 years ago

Medium-level API: Some of the defined enums are designed in a way that is open to future additions of variants in the REAPER API: By using a Custom(...) variant, e.g. this one:

/// A modifier key.
#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
pub enum ModKey{
    /// SHIFT key.
    Shift,
    /// CTRL key.
    Control,
    /// ALT key.
    Menu,
    /// Custom modifier key according to
    /// [this list](https://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes).
    Custom(i32),
}

Shift variant is translated to the number 16 when passed to REAPER API. It shouldn't make any difference equality-wise if we use Custom(16) instead. At the moment it does. Just compare and hash to_raw() instead of self!

helgoboss commented 3 years ago

Actually that's not so useful. This is most relevant for enums that are passed as parameters of callback functions (functions that are called by REAPER and implemented by the plug-in) because in this case the parameter value usually needs to be compared. In the other direction usually not that often. The problem is that this comparison can also be made by matching - and matching doesn't take Hash or PartialEq into account.

helgoboss commented 3 years ago

A more consistent solution would probably be a newtype along with a few known constants.

helgoboss commented 3 years ago

Made ModKey and TouchedParameterType newtypes.

Further candidates used in control surface API (which are also used for the opposite direction, that is, plug-in to REAPER):

Plus, there are many enums returned by functions.

At the moment many of them don't even have Custom variant. So if the REAPER devs decide to add a new mode, a non-upgraded reaper-rs-based plug-in would panic. The good side is that this is a kind of "hey, there's something new we must support". But a panic in medium API level is too opinionated, it should be left to the consumer how to handle this situation.

As an overview, I can think of the following approaches to handle an unknown variant coming from REAPER:

  1. Panic
    • Not good. We couldn't even avoid that when always keeping up-to-date with the latest REAPER API because plug-ins that are already built would panic, too.
  2. Custom(...) variant
    • Okay on first sight but would result in the aforementioned issue: Let's assume REAPER gets a new Foo variant and a consumer employs this new variant by matching with Custom(...). At some point reaper-rs gets updated to support this new variant. Everything fine and working correctly until now, better than panic from a user perspective. However, as soon as the consumer upgrades to the latest version of reaper-rs, Foo will be returned, so it won't match anymore with Custom(...). Worst thing: This will not be a compile-time error!
    • Perfectly okay for the other direction though (if the consumer creates the variant and passes it to REAPER).
  3. Unknown variant
    • Like Custom but not exposing the actual value.
    • The good thing about this is that the problem of point 2 can't happen - because the consumer can't rely on any unsupported variant in the first place.
    • This also means that the consumer is forced to contribute the new variant to reaper-rs. Although slightly inconvenient, I consider this as good because it tends to keeps reaper-rs more up-to-date and brings the benefit of more readable code to all other consumers at the same time. Also, if consumer is in a hurry, using a customized reaper-rs version is super easy thanks to Cargo.
  4. Newtype
    • This is probably the way to go for value ranges that are too fluctuating or large, or if there are undefined variants and the underlying raw value (e.g. integer) feels kind of natural to use (e.g. ControllerNumber type in helgoboss-midi).
helgoboss commented 3 years ago

I really like the Unknown variant approach and will apply it to each enum that potentially suffers this problem (each enum that is returned by REAPER or a callback parameter). Sorry, it's an API change but this one seems important. Much more panic-proof everything. Can remove lots of expect() calls in the medium-level API.