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

PortSpec should be an enum, not a trait #73

Closed
Javyre opened this issue Oct 7, 2017 · 9 comments
Closed

PortSpec should be an enum, not a trait #73

Javyre opened this issue Oct 7, 2017 · 9 comments

Comments

@Javyre
Copy link
Contributor

Javyre commented Oct 7, 2017

It seems very unnecessary for PortSpec to be a trait.
Since i have to pass PortSpecs around in my code, it would be much easier passing around an enum instead of having to use generics and complicate code.

I dont think it would be too hard to do this:

pub enum PortSpec {
    AudioOut,
    AudioIn,
    MidiOut,
    MidiIn,
    Unowned,
}

impl PortSpec {
    fn jack_port_type(&self) -> &str { ... }

    fn jack_flags(&self) -> PortFlags { ... }

    fn jack_buffer_size(&self) -> libc::c_ulong { ... }
}

Although i understand this would break compatibility with previous versions of the library.
But i think this would be more idiomatic to rust and cleaner in the users code.

EDIT:
Also, something that would become possible to do with this that i need right now is to match PortSpecs:

match spec {
    j::PortSpec::AudioIn => {}
    j::PortSpec::AudioOut => {}
    ...
}
@wmedrano
Copy link
Member

wmedrano commented Oct 7, 2017

PortSpec is a trait so that other libraries can create their own types beyond the built-in "audio" and "midi". Though I see how supporting the built in types is more important.

Another convenience of having each be its own type is that each Port gets its own type as well.
This allows for this type safety:

AudioInSpec -> Port<AudioInSpec> -> AudioInPort wrapper
AudioOutSpec -> Port<AudioOutSpec> -> AudioOutPort wrapper
MidiInSpec -> Port<MidiInSpec> -> MidiInPort wrapper

In the above case, having the types allows for specific wrappers for each port.
AudioInPort is similar to a readable f32 slice
AudioOutPort is similar to a writeable f32 slice
MidiInPort is an iterable over midi signals.

If the Spec was no longer a different type for each port, then the conversions would not be as straight forward, it would be.

Spec::AudioIn -> Port -> AudioInPort
Spec::AudioOut -> Port -> AudioOutPort
Spec::MidiIn -> Port -> MidiInPort

// This last case isn't a valid transformation, so it should panic at
// runtime instead of being checked at compile time.
Spec::AudioIn -> Port -> AudioOutPort

@wmedrano
Copy link
Member

wmedrano commented Oct 7, 2017

In your use case, can you just use a custom Spec for your enum?

pub enum BuiltInSpecs {
    AudioIn,
    AudioOut,
    MidiIn,
    MidiOut,
}

impl PortSpec for BuiltInSpecs {
   ...
}

For convenience, the wrappers would have to take a port with any spec.

impl<'a> AudioOutPort<'a> {
    pub fn new_from_generic_port(port: &'a mut Port<T>, ps: &'a ProcessScope) -> Err<Self, err> {
        // check that port type is audio
        // check that direction is output
        // create wrapper
    }
}

@wmedrano
Copy link
Member

wmedrano commented Oct 7, 2017

Or, maybe easier:

pub enum BuiltInSpecs {
  AudioIn(AudioInSpec),
  AudioOut(AudioOutSpec),
  MidiIn(MidiInSpec),
  MidiOut(MidiOutSpec),
}
pub enum BuiltInPorts {
    AudioIn(Port<AudioInSpec>),
    AudioOut(Port<AudioOutSpec>),
    ...
}

@Javyre
Copy link
Contributor Author

Javyre commented Oct 7, 2017

Yeah I guess I understand the extensibility it brings.
I suppose i can make a wrapper enum.
I only need it for the specs themselves though
The ports being their own struct is fine by me.

My problem is that if I want to make a wrapper around Port, i need to mess with generics and it gets messy.

For example, this is not allowed since they are two different types:

let spec = if is_output { j::AudioOutSpec } else { j::AudioInSpec };
let spec = if is_output { j::Spec::AudioOut } else { j::Spec::AudioIn }; // this would work because theyre both the same type

@Javyre
Copy link
Contributor Author

Javyre commented Oct 7, 2017

Another probleam im having is how do I make a function that returns a port that can be of any spec?

fn register_port(&self, ...) -> j::Port</* has to be a type here at compile time */> {}

@Javyre
Copy link
Contributor Author

Javyre commented Oct 7, 2017

And another annoyance is that i cannot reuse the same spec twice in port registration:

let portl = cli.register_port(&pnl, spec).unwrap();
let portr = cli.register_port(&pnr, spec).unwrap(); // I don't own spec anymore so this doesn't compile!

This is probably a seperate issue though, spec should be passed by reference here...

@Javyre
Copy link
Contributor Author

Javyre commented Oct 8, 2017

About the last annoyance thing, maybe there should be a #[derive(Clone, Copy)] for the specs?

@Javyre
Copy link
Contributor Author

Javyre commented Oct 8, 2017

Ended up finding a way: Snowlabs/Jamyx#1 (comment)

Thanks for the help!

Should the issue be renamed to the #[derive(Copy, Clone)] thing?

@Javyre
Copy link
Contributor Author

Javyre commented Jan 13, 2018

This has been fully resolved now...
2628107

@Javyre Javyre closed this as completed Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants