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

Why is send always overwriting MIDIMessage.channel? #56

Open
konstantint opened this issue Feb 9, 2024 · 4 comments
Open

Why is send always overwriting MIDIMessage.channel? #56

konstantint opened this issue Feb 9, 2024 · 4 comments

Comments

@konstantint
Copy link

konstantint commented Feb 9, 2024

Just curious, but why is send forcing a channel on a message (here) even if the message had its channel field set explicitly? Isn't the main point of the message having a "channel" field in being able to prepare a set of messages assigned to different channels and have the MIDI interpreter send them to those channels, exactly as given?

Sure, I could always make sure to write midi.send(message, message.channel), but this now seems to defeat the purpose of the send method's channel argument (and the MIDI object out_channel field) in being able to configure an out_channel as some sort of a default when necessary.

A more meaningful design, IMO, could be:

  • Whenever a parameter is passed to send, use it.
  • Otherwise, use MIDIMessage.channel, unless it is None.
  • ... in which case fall back to MIDI.out_channel.
@tannewt
Copy link
Member

tannewt commented Feb 9, 2024

It originates from this conversation I think: #9 (comment)

I agree it is a bit weird to overwrite it.

@kevinjwalters may have thoughts too.

@kevinjwalters
Copy link

Not much to add as there's a lot covered in aforementioned conversation which is better than my memory here as this was quite a while back! I don't have an opinion on discussed change other than it may affect existing application code.

There might have been some influence here from the original implementation too.

@todbot
Copy link

todbot commented Feb 11, 2024

I've been bitten by this too. Having so many useful places to set channel makes the logic a bit tricky. I would expect the precedence to be: (send(), msg, MIDI) as @konstantint suggests. I think swapping the channel assignment in the isinstance() conditional would set that precedence. E.g. from this:

        if isinstance(msg, MIDIMessage):
            msg.channel = channel

to this:

        if isinstance(msg, MIDIMessage):
            channel = msg.channel or channel  # guard against msg channel not set

But I've not tested this.

@tannewt
Copy link
Member

tannewt commented Feb 12, 2024

I wouldn't use or because 0 may be considered false. But this logic with an if statement would be fine.

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

4 participants