-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
ID3v2: Support more frames #236
Conversation
The spec defines multiple frames that are only meant to appear in a tag once. They are the following: "MCDI", "ETCO", "MLLT", "SYTC", "RVRB", "PCNT", "RBUF", "POSS", "OWNE", "SEEK", and "ASPI". We now check for their presence in `Id3v2::insert` to prevent multiple appearing in a tag.
/// Represents an "RVA2" frame | ||
RelativeVolumeAdjustment(RelativeVolumeAdjustmentFrame), | ||
/// Unique file identifier | ||
UniqueFileIdentifier(UniqueFileIdentifierFrame), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. This is used by many MusicBrainz tags needed for http://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id21
}) | ||
} | ||
|
||
/// Used for errors in write::frame::verify_frame | ||
pub(super) fn name(&self) -> &'static str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use strum
for this purpose: https://docs.rs/strum/latest/strum/trait.VariantNames.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, looks like they have some useful derives. There are probably a few places they could be used. I might look into it eventually.
@@ -320,8 +331,18 @@ impl Id3v2Tag { | |||
} | |||
|
|||
/// Removes a [`Frame`] by id | |||
pub fn remove(&mut self, id: &str) { | |||
self.frames.retain(|f| f.id_str() != id) | |||
pub fn remove(&mut self, id: &str) -> impl Iterator<Item = Frame<'static>> + '_ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a FrameId<'_>
would be type safe and avoid the somehow unrelated eq_ignore_ascii_case()
. Otherwise we should extract a function that explcitly matches FrameId
with str
consistently instead of inlining this code here and probably elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll change that soon in a follow up PR. I don't know how many places it's used right now.
This adds support for the following frames: "RVA2", "OWNE", "ETCO", and "PRIV".
Additionally, this changes
Id3v2Tag::remove
to return the frame(s) removed, andId3v2Tag::insert
to check for frames that only meant to appear once in a file, replacing if necessary. Those frames are: "MCDI", "ETCO", "MLLT", "SYTC", "RVRB", "PCNT", "RBUF", "POSS", "OWNE", "SEEK", and "ASPI".Part of #189