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

Add Display trait to MetaMessage enum #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gbrochar
Copy link

@gbrochar gbrochar commented Apr 1, 2024

Hello again :) I hope this isn't considered spam, I'd rather be spamming than non-exhaustive hence the amount of messages !

SequencerSpecific and Unknown are in wildcard match case because it doesn't seemed like it should be text, please tell me your views.

Before

println!("{:?}", meta_message);
SequencerSpecific([5, 15, 18, 0, 0, 127, 65, 0])
SequencerSpecific([5, 15, 28, 50, 48, 48, 51, 46, 49, 49, 46, 49, 50])
SequencerSpecific([5, 15, 9, 0, 64])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
TrackName([68, 97, 114, 117, 100, 101, 32, 45, 32, 83, 97, 110, 100, 115, 116, 111, 114, 109])
Text([50, 48, 48, 48, 32, 40, 67, 41, 32, 98, 121, 32, 73, 122, 122, 101, 116, 32, 83, 101, 108, 97, 110, 105, 107, 32, 102, 114, 111, 109, 32, 84, 111, 112, 108, 105, 115, 116, 45, 84, 101, 97, 109, 46, 99, 111, 109, 10])
Copyright([50, 48, 48, 48, 32, 40, 67, 41, 32, 98, 121, 32, 73, 122, 122, 101, 116, 32, 83, 101, 108, 97, 110, 105, 107, 32, 97, 110, 100, 32, 65, 114, 110, 101, 32, 77, 117, 108, 100, 101, 114, 32, 102, 114, 111, 109, 32, 84, 111, 112, 108, 105, 115, 116, 45, 84, 101, 97, 109, 46, 99, 111, 109])
MidiChannel(u4(0))
Tempo(u24(600000))
EndOfTrack
SequencerSpecific([5, 15, 9, 64, 72])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
MidiChannel(u4(0))
EndOfTrack
SequencerSpecific([5, 15, 9, 0, 64])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
TrackName([])
EndOfTrack

After

println!("{meta_message}");
SequencerSpecific([5, 15, 18, 0, 0, 127, 65, 0])
SequencerSpecific([5, 15, 28, 50, 48, 48, 51, 46, 49, 49, 46, 49, 50])
SequencerSpecific([5, 15, 9, 0, 64])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
TrackName("Darude - Sandstorm")
Text("2000 (C) by Izzet Selanik from Toplist-Team.com
")
Copyright("2000 (C) by Izzet Selanik and Arne Mulder from Toplist-Team.com")
MidiChannel(u4(0))
Tempo(u24(600000))
EndOfTrack
SequencerSpecific([5, 15, 9, 64, 72])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
MidiChannel(u4(0))
EndOfTrack
SequencerSpecific([5, 15, 9, 0, 64])
SequencerSpecific([5, 15, 6, 71, 101, 110, 101, 114, 97, 108, 32, 77, 73, 68, 73])
TrackName("")
EndOfTrack

Please note the ugly newline is in the before version as well (ascii code 10).
Please note the TrackName([]) vs TrackName("") as that could be a subject of discussion.
Please note I struggled to print what could be Japanese characters from old (PC98) game, but I lack the knowledge in encoding to solve that. I didn't even tested with more classic UTF-8 anyway, it's not my use case ATM.
Please note I am not experienced in open source contributing and I apologies if this creates technical debt, I'm not used to propre crate coding and maybe there is a "standard" way for the case of enum with &[u8] variants.

I can implement the rest of the Display in one pull request only or many if you prefer that but I'd rather see if you're done for it else I'll just implement for my needs for now. I can limit the pull requests if they're annoying I don't know the good practices or your preferences. I'd like to work on my project as well at some point 😂

Thank you for your work 🙏

@kovaxis
Copy link
Owner

kovaxis commented Jun 15, 2024

Hello! Sorry for the late reply.

This looks good, although it belongs more as an fmt::Debug implementation than an fmt::Display implementation. Display is intended for when a type has a "canonical" representation that an end-user could understand. I believe MIDI messages are not really end-user representable like strings or numbers are.

Another detail that could be improved is using the "{:?}" representation of strings. For example, it would print \n instead of the "ugly newline".

I'd be happy to merge this after those two details are implemented, if you'd like to work on this.

About the SequencerSpecific and Unknown variants not showing text, it looks good to me. They really don't look like they are meant to hold text anyway.

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

Successfully merging this pull request may close these issues.

2 participants