midly icon indicating copy to clipboard operation
midly copied to clipboard

Add Display trait to MetaMessage enum

Open gbrochar opened this issue 10 months ago • 1 comments

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 :joy:

Thank you for your work :pray:

gbrochar avatar Apr 01 '24 18:04 gbrochar

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.

kovaxis avatar Jun 15 '24 21:06 kovaxis