web-audio-api-rs icon indicating copy to clipboard operation
web-audio-api-rs copied to clipboard

Check non spec API surface

Open b-ma opened this issue 3 years ago • 3 comments
trafficstars

Maybe we should have a closer look on non-spec pub structs and traits and traits before v1 too, to have an API surface that is as lean as possible? Just a few questions I asked myself here and there:

  • can we make ConcreteBaseAudioContext pub(crate) only? we currently only the register method that is public, maybe we could move it to BaseAudioContext?
  • is RenderQuantum::force_mono() really needed as it's basically a shortcut for RenderQuantum::set_number_of_channels(1)?
  • etc.

b-ma avatar Apr 22 '22 08:04 b-ma

can we make ConcreteBaseAudioContext pub(crate) only?

I like the idea, but there is no simple solution, because in rust a function must return a concrete type. It is not allowed to return a raw trait object like this:

// compile error
impl BaseAudioContext for OfflineAudioContext {
    fn base(&self) -> dyn BaseAudioContext {
        &self.base
    }
}

The following also does not work:

  • return a Box<dyn BaseAudioContext>, because the trait is not 'object safe' due to the generic methods on it
  • return a impl BaseAudioContext (provided by ConcreteBaseAudioContext). This is not allowed (but will be in a later version of rust - https://rust-lang.github.io/rfcs/2071-impl-trait-existential-types.html )

The last resort would be to mark the ConcreteBaseAudioContext #[doc(hidden)] and add some clear documentation on the base() method, I'll try to wip up a PR to see how it looks

orottier avatar Jun 30 '22 15:06 orottier

Just a small note to be discussed, finding this not intuitive when creating the bindings (this is actually part of the spec but also of the deviations from the spec, so...)

Maybe having:

pub struct ChannelConfigOptions {
    pub count: usize,
    pub count_mode: ChannelCountMode,
    pub interpretation: ChannelInterpretation,
}

with count_mode instead of mode would be more logical ?

b-ma avatar Sep 13 '22 09:09 b-ma

Good point, but it's hard to find a middle ground

The IDL is

dictionary AudioNodeOptions {
  unsigned long channelCount;
  ChannelCountMode channelCountMode;
  ChannelInterpretation channelInterpretation;
};

We could go entirely with the IDL, or use your suggestion. Indeed having count_mode is more consistent (we just leave the channel prefix out of each parameter, because it is already in the struct name). Let's go with your suggestion

orottier avatar Sep 14 '22 15:09 orottier