Version: 1.4.3 (using KDE KDE 3.5.4) Installed from: Compiled From Sources Both iTunes 7.0 and Windows Media Player 10 expose the ID3v2 field TPE2 as Album Artist. This allows the album to be grouped by the album artist, even when individual songs are performed (TPE1) by different artists (the so-called "various artists albums"). Even though the ID3v2 specification designates TPE2 as "Band/orchestra/accompaniment", I think it'll be useful to adopt this popular interpretation of the TPE2 tag.
Created attachment 18142 [details] Album Artist support in ID3v2 (TPE2) Here's a proposed patch. It merely extends the tag editor dialog / columns and causes the Album Artist tag to be read/written (as TPE2) into ID3v2 tags. It doesn't do anything smart with the new Album Artist information; that'll be left for the next patches. I also have similar patches for WMA and MP4 but let's first get this one in.
Can anyone please review this patch, or at the very least mark this bug as NEW? This is delaying me in the further contributions I want to make to Amarok.
Hi Ilya, we had a discussion on the 'album artist' topic recently on irc. We concluded that it does not make sense to just include an additional column in the tags table and to include the possibility to show it in the tag editor, add it as a column to the playlist, ... However, this does not at all mean that we are not interested in supporting this tag. I'm sure that you are aware of Amarok's handling of compilations: in its collection browser, those are filed under "Various Artists". In our opinion, the album artist is a logical extension to this concept and thus should be unified with the already available compilation handling. And in any way, unified handling for 'album artist' across file formats in MetaBundle is a good thing. Sorry for slowing down your work, but as the 1.4.4 release is very close - only a few days away. So we all feel very reluctant about changing database tables, ... as this tends to break things very easily. However, in a week or so we will apply your patch - so please keep the patches coming! You might also want to join us on IRC at #amarok in order to discuss this a little bit more.
Obviously there are smart things to do with the Album Artist information: it shouldn't just be another piece of metadata, but rather something we make decisions upon, as to whether consider an album a compilation, how to place it in the browser tree etc. My patch starts this by providing the infrastructure: parsing the TPE2 tag and storing it in yet another column. I don't believe that editing should be done any different than the way I've done it; however, a unifying display column should be made, which would be something to be done in another patch. If it's a tough time, I'll wait. However, if there's a discussion about this subject and it's held on IRC while I'm not there, please post a summary here so I'd know what's going on and how I can help.
Release 1.4.4 is already out and about a month has passed. Could anyone please check this patch in?
SVN commit 609411 by aumuell: commit parts of the album artist patch - i omitted the tag editor part for two reasons, though: - the patch didn't apply any longer - don't make the album artists user visible until it serves a purpose - thanks to Ilya Konstantinov <kde-bugzilla@future.shiny.co.il> for the patch (and please continue your work, making it actually useful)! CCBUG: 134202 M +21 -2 metabundle.cpp M +12 -7 metabundle.h M +2 -0 xmlloader.cpp --- trunk/extragear/multimedia/amarok/src/metabundle.cpp #609410:609411 @@ -110,7 +110,7 @@ { // construct static qstrings to avoid constructing them all the time static QString columns[] = { - "Filename", "Title", "Artist", "Composer", "Year", "Album", "DiscNumber", "Track", "BPM", "Genre", "Comment", + "Filename", "Title", "Artist", "AlbumArtist", "Composer", "Year", "Album", "DiscNumber", "Track", "BPM", "Genre", "Comment", "Directory", "Type", "Length", "Bitrate", "SampleRate", "Score", "Rating", "PlayCount", "LastPlayed", "Mood", "Filesize" }; static QString error( "ERROR" ); @@ -128,6 +128,7 @@ case Filename: return i18n( "Filename" ); case Title: return i18n( "Title" ); case Artist: return i18n( "Artist" ); + case AlbumArtist:return i18n( "Album Artist"); case Composer: return i18n( "Composer" ); case Year: return i18n( "Year" ); case Album: return i18n( "Album" ); @@ -312,6 +313,7 @@ m_url = bundle.m_url; m_title = bundle.m_title; m_artist = bundle.m_artist; + m_albumArtist = bundle.m_albumArtist; m_composer = bundle.m_composer; m_album = bundle.m_album; m_comment = bundle.m_comment; @@ -387,6 +389,7 @@ { return uniqueId() == bundle.uniqueId() && //first, since if using IDs will return faster artist() == bundle.artist() && + albumArtist() == bundle.albumArtist() && title() == bundle.title() && composer() == bundle.composer() && album() == bundle.album() && @@ -539,9 +542,12 @@ if ( !file->ID3v2Tag()->frameListMap()["TCOM"].isEmpty() ) setComposer( TStringToQString( file->ID3v2Tag()->frameListMap()["TCOM"].front()->toString() ).stripWhiteSpace() ); + if ( !file->ID3v2Tag()->frameListMap()["TPE2"].isEmpty() ) // non-standard: Apple, Microsoft + setAlbumArtist( TStringToQString( file->ID3v2Tag()->frameListMap()["TPE2"].front()->toString() ).stripWhiteSpace() ); + if ( !file->ID3v2Tag()->frameListMap()["TCMP"].isEmpty() ) compilation = TStringToQString( file->ID3v2Tag()->frameListMap()["TCMP"].front()->toString() ).stripWhiteSpace(); - + if(images) { loadImagesFromTag( *file->ID3v2Tag(), *images ); } @@ -679,6 +685,7 @@ { setTitle( bundle.title() ); setArtist( bundle.artist() ); + setAlbumArtist( bundle.albumArtist() ); setComposer( bundle.composer() ); setAlbum( bundle.album() ); setYear( bundle.year() ); @@ -733,6 +740,7 @@ { case Title: setTitle( newText ); break; case Artist: setArtist( newText ); break; + case AlbumArtist: setAlbumArtist( newText ); break; case Composer: setComposer( newText ); break; case Year: setYear( newText.toInt() ); break; case Album: setAlbum( newText ); break; @@ -761,6 +769,7 @@ case Filename: return filename(); case Title: return title(); case Artist: return artist(); + case AlbumArtist: return albumArtist(); case Composer: return composer(); case Year: return QString::number( year() ); case Album: return album(); @@ -794,6 +803,7 @@ case Filename: text = isFile() ? MetaBundle::prettyTitle(filename()) : url().prettyURL(); break; case Title: text = title().isEmpty() ? MetaBundle::prettyTitle( filename() ) : title(); break; case Artist: text = artist(); break; + case AlbumArtist: text = albumArtist(); break; case Composer: text = composer(); break; case Year: text = year() ? QString::number( year() ) : QString::null; break; case Album: text = album(); break; @@ -1271,7 +1281,9 @@ case ( discNumberTag ): id = "TPOS"; break; case ( bpmTag ): id = "TBPM"; break; case ( compilationTag ): id = "TCMP"; break; + case ( albumArtistTag ): id = "TPE2"; break; // non-standard: Apple, Microsoft } + fprintf(stderr, "Setting extended tag %s to %s\n", id, value.utf8().data()); TagLib::MPEG::File *mpegFile = dynamic_cast<TagLib::MPEG::File *>( file ); if ( mpegFile && mpegFile->ID3v2Tag() ) { @@ -1298,6 +1310,7 @@ case ( discNumberTag ): id = "DISCNUMBER"; break; case ( bpmTag ): id = "BPM"; break; case ( compilationTag ): id = "COMPILATION"; break; + case ( albumArtistTag ): id = "ALBUMARTIST"; break; // non-standard: Amarok } TagLib::Ogg::Vorbis::File *oggFile = dynamic_cast<TagLib::Ogg::Vorbis::File *>( file ); if ( oggFile && oggFile->tag() ) @@ -1315,6 +1328,7 @@ case ( discNumberTag ): id = "DISCNUMBER"; break; case ( bpmTag ): id = "BPM"; break; case ( compilationTag ): id = "COMPILATION"; break; + case ( albumArtistTag ): id = "ALBUMARTIST"; break; // non-standard: Amarok } TagLib::FLAC::File *flacFile = dynamic_cast<TagLib::FLAC::File *>( file ); if ( flacFile && flacFile->xiphComment() ) @@ -1447,6 +1461,7 @@ if ( hasExtendedMetaInformation() ) { + setExtendedTag( f->file(), albumArtistTag, albumArtist() ); setExtendedTag( f->file(), composerTag, composer().string().stripWhiteSpace() ); setExtendedTag( f->file(), discNumberTag, discNumber() ? QString::number( discNumber() ) : QString() ); setExtendedTag( f->file(), bpmTag, bpm() ? QString::number( bpm() ) : QString() ); @@ -1740,6 +1755,9 @@ void MetaBundle::setComposer( const AtomicString &composer ) { aboutToChange( Composer ); m_composer = composer; reactToChange( Composer ); } +void MetaBundle::setAlbumArtist( const AtomicString &albumArtist ) +{ aboutToChange( AlbumArtist ); m_albumArtist = albumArtist; reactToChange( AlbumArtist ); } + void MetaBundle::setPlayCount( int playcount ) { aboutToChange( PlayCount ); m_playCount = playcount; reactToChange( PlayCount ); } @@ -1765,6 +1783,7 @@ m_title = QDeepCopy<QString>(m_title); m_artist = m_artist.deepCopy(); + m_albumArtist = m_albumArtist.deepCopy(); m_album = m_album.deepCopy(); m_comment = m_comment.deepCopy(); m_composer = m_composer.deepCopy(); --- trunk/extragear/multimedia/amarok/src/metabundle.h #609410:609411 @@ -61,6 +61,7 @@ Filename = 0, Title, Artist, + AlbumArtist, Composer, Year, Album, @@ -244,6 +245,7 @@ const KURL &url() const; QString title() const; AtomicString artist() const; + AtomicString albumArtist() const; AtomicString composer() const; AtomicString album() const; AtomicString genre() const; @@ -295,6 +297,7 @@ void setPath( const QString &path ); void setTitle( const QString &title ); void setArtist( const AtomicString &artist ); + void setAlbumArtist( const AtomicString &albumArtist ); void setComposer( const AtomicString &composer ); void setAlbum( const AtomicString &album ); void setGenre( const AtomicString &genre ); @@ -343,7 +346,7 @@ static QStringList genreList(); protected: - enum ExtendedTags { composerTag, discNumberTag, bpmTag, compilationTag }; + enum ExtendedTags { composerTag, albumArtistTag, discNumberTag, bpmTag, compilationTag }; /** Called before the tags in \p columns are changed. */ virtual void aboutToChange( const QValueList<int> &columns ); @@ -360,6 +363,7 @@ KURL m_url; QString m_title; AtomicString m_artist; + AtomicString m_albumArtist; AtomicString m_composer; AtomicString m_album; AtomicString m_comment; @@ -483,11 +487,13 @@ { return url().isLocalFile() ? url().directory() : url().upURL().prettyURL(); } -inline QString MetaBundle::title() const { return m_title; } -inline AtomicString MetaBundle::artist() const { return m_artist; } -inline AtomicString MetaBundle::album() const { return m_album; } -inline AtomicString MetaBundle::comment() const { return m_comment; } -inline AtomicString MetaBundle::genre() const { return m_genre; } +inline QString MetaBundle::title() const { return m_title; } +inline AtomicString MetaBundle::artist() const { return m_artist; } +inline AtomicString MetaBundle::album() const { return m_album; } +inline AtomicString MetaBundle::comment() const { return m_comment; } +inline AtomicString MetaBundle::genre() const { return m_genre; } +inline AtomicString MetaBundle::composer() const { return m_composer; } +inline AtomicString MetaBundle::albumArtist() const { return m_albumArtist; } inline QString MetaBundle::streamName() const { return m_streamName; } inline QString MetaBundle::streamUrl() const { return m_streamUrl; } inline QString MetaBundle::uniqueId() const { return m_uniqueId; } @@ -504,7 +510,6 @@ return CompilationUnknown; } -inline AtomicString MetaBundle::composer() const { return m_composer; } inline QString MetaBundle::type() const { --- trunk/extragear/multimedia/amarok/src/xmlloader.cpp #609410:609411 @@ -80,6 +80,7 @@ { case Artist: case Composer: + case AlbumArtist: case Year: case Album: case DiscNumber: @@ -109,6 +110,7 @@ { case Artist: case Composer: + case AlbumArtist: case Year: case Album: case DiscNumber:
Checked in.