Re: Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)
> On Jan. 17, 2012, 6:40 p.m., Ralf Engels wrote: > > While working on the multiple-artists feature I came to the following > > conclusion: > > > > Amarok should only try to guess album artists (or all artists in case of a > > "Ralf featuring Matej") for internal usage and not try to write them back. > > That would mean that the album artist could still be displayed as being > > empty while the track artist is used in the Collection. > > > > Still, the patch is good. Ship it. Yup, I agree with the conclusion. But still I'm unable to come up with something reasonable. [brainstorm] Album::prettyAlbumArtist() (the guessed one) and Album::albumArtist() (the true one)? Also, I think that Album::hasAlbumArtist() is completely redundant (I fear to trust all Amarok code to return non-null albumArtist() if hasAlbumArtist() == true) and Album::isCompilation() is virtually !Album::hasAlbumArtist(). Perhaps we could remove both and define compilations as "Album::prettyAlbumArtist() == null ptr"? [/brainstorm] Speaking about the patch I've also received favourable review by email from Maximilian Kossick that will result in some docstring clarifications and a few more comments. If noone opposes, I'll push this tomorrow or so. - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/#review9892 --- On Jan. 17, 2012, 2:10 p.m., Matěj Laitl wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103715/ > --- > > (Updated Jan. 17, 2012, 2:10 p.m.) > > > Review request for Amarok, Bart Cerneels and Ralf Engels. > > > Description > --- > > MetaFile: guess album artist as in SQL collection scanner, compilations > > Call albumArtist = ArtistHelper::bestGuessAlbumArtist( albumArtist, > artist, genre, composer ); afer the data are read from file tags. > > This makes album artist field consistent for tracks in Local collection > and tracks in file browser or drag & dropped to Amarok playlist, but it > has some downsides: > * Amarok will essentially lie about what album artist is assigned to a >track, most notably will replace empty album artist by track artist >and will replace "Various Artists" by empty artist. > * Editing album artist will be strange in corner cases: in particular >if user sets album artist to "Various Artists", it will be written >to file tags as-is, but displayed as empty album artist. Other case >would be when user sets album artist to empty string, but this is >currently prevented due to a bug/feature in TagDialog. > > (every occurrence of "Various Artists" also accepts localized version > of the string) > > Above change is needed for upcoming "MemoryCollection: use also album > artist as identifying key in album map" change for UmsCollection, but > if someone opposes, similar change can be made on UmsCollection level. > > Also, implement MetaFile::FileAlbum isCompilation() which is now easy > as it can return albumArtist.isEmpty() > > DIGEST: unify album artist handling in Amarok > > > > > > MemoryCollection: use also album artist as identifying key in album map > > This has some far-reaching consequences which I believe are only > positive, most notably Amarok is able to differentiate between 2 albums > with same name but different album artists in USB Mass Storage > collection and in "new" iPod collection. This unifies album artist > handling across MemoryCollection and SqlCollection, 2 major collection > storage implementations in Amarok. > > Technically, AlbumMap is changed from: > typedef QMap AlbumMap; > to: > class AlbumMap : public QMap > > with insert(), remove(), value(), contains() QMap methods hidden and > reimplemented for convenience and to prevent coding mistakes. (you no > longer can add album under wrong key) > > This change is tested to work with new iPod collection, UMS collection > (depends on previous "MetaFile: guess album artist as in SQL collection > scanner, compilations"), all tests still pass. > > Only area that received just very basic testing are online > service collections that are also affected by this change. > > DIGEST: Albums with same name but different album artist are now > correctly separated in USB Mass Storage, iPod and various > online service collections. > > > Diffs > - > > ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b > src/core-impl/collections/daap/daapreader/Reader.cpp > 9f749c5fe892f0cbcbacd8454d8fc80940f6bb43 > src/core-impl/collections/ipodcollection/handler/IpodHandler.h > ceccd7cc3028051d9b93201caea180a10af15ed3 > src/core-impl/collectio
Re: Amarok's iPod Collection: Volunteering to help
On 17. 1. 2012 Santiago G. wrote: > Hi. My name is Santiago, I'm the poster of this bug request: > https://bugs.kde.org/show_bug.cgi?id=291722 Hi Santiago! > Today I have been fighting all afternoon to be able to transfer music > from my music collection to my iPod touch. So far I couldn't not find a > complete solution, and that annoys me. Even using Apple's iTunes > (running in a virtual machine) I have the issue of not being able to > transcode files as I would like. > > Being an Amarok lover, and knowing that what you are working on will be > awesome: making the whole process simple and easy (as it should be in > the first place), I wanted to volunteer to help you in any way you want. > My coding skills aren't that great yet, but I can help you with testing, > documentation, translation, or whatever you may need. That would be great, I do have older iPod nanos to test, but devices with iOS are accessed in a slightly different way so I would certainly use a lot of testing from you. For start, I'd like you to compile amarok from sources, start using that version (Version 2.5-GIT must appear in Amarok about dialog) and verify it works at least as good (wrt iPod support) as Amarok 2.5 from kubuntu. [1] should definitely help you doing so, but please use my git clone git://anongit.kde.org/clones/amarok/laitl/amarok.git instead of official git://anongit.kde.org/amarok.git [1] http://amarok.kde.org/wiki/2.0_Development_HowTo > I have posted the output of the command you told me, and if you want I > could get you the data for an iPad 2. Yup, that was already useful for me, I'll let you know when I'll need more. > Also, if you want to get in touch in a more direct way---IRC, MSN > Messenger, etc.---feel free to tell me, I have free time I could spend. Good, my MSN is stro...@hotmail.com and Jabber stro...@gmail.com but I generally prefer e-mails because I can handle them in batches, but don't hesitate to contact me using MSN/Jabber for short&quick things. > Thank you very much for your work. Best of luck. > > /Santiago/ Thanks for your help, this will directly contribute to better support of iPod devices in Amarok. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/#review9892 --- Ship it! While working on the multiple-artists feature I came to the following conclusion: Amarok should only try to guess album artists (or all artists in case of a "Ralf featuring Matej") for internal usage and not try to write them back. That would mean that the album artist could still be displayed as being empty while the track artist is used in the Collection. Still, the patch is good. Ship it. - Ralf Engels On Jan. 17, 2012, 2:10 p.m., Matěj Laitl wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103715/ > --- > > (Updated Jan. 17, 2012, 2:10 p.m.) > > > Review request for Amarok, Bart Cerneels and Ralf Engels. > > > Description > --- > > MetaFile: guess album artist as in SQL collection scanner, compilations > > Call albumArtist = ArtistHelper::bestGuessAlbumArtist( albumArtist, > artist, genre, composer ); afer the data are read from file tags. > > This makes album artist field consistent for tracks in Local collection > and tracks in file browser or drag & dropped to Amarok playlist, but it > has some downsides: > * Amarok will essentially lie about what album artist is assigned to a >track, most notably will replace empty album artist by track artist >and will replace "Various Artists" by empty artist. > * Editing album artist will be strange in corner cases: in particular >if user sets album artist to "Various Artists", it will be written >to file tags as-is, but displayed as empty album artist. Other case >would be when user sets album artist to empty string, but this is >currently prevented due to a bug/feature in TagDialog. > > (every occurrence of "Various Artists" also accepts localized version > of the string) > > Above change is needed for upcoming "MemoryCollection: use also album > artist as identifying key in album map" change for UmsCollection, but > if someone opposes, similar change can be made on UmsCollection level. > > Also, implement MetaFile::FileAlbum isCompilation() which is now easy > as it can return albumArtist.isEmpty() > > DIGEST: unify album artist handling in Amarok > > > > > > MemoryCollection: use also album artist as identifying key in album map > > This has some far-reaching consequences which I believe are only > positive, most notably Amarok is able to differentiate between 2 albums > with same name but different album artists in USB Mass Storage > collection and in "new" iPod collection. This unifies album artist > handling across MemoryCollection and SqlCollection, 2 major collection > storage implementations in Amarok. > > Technically, AlbumMap is changed from: > typedef QMap AlbumMap; > to: > class AlbumMap : public QMap > > with insert(), remove(), value(), contains() QMap methods hidden and > reimplemented for convenience and to prevent coding mistakes. (you no > longer can add album under wrong key) > > This change is tested to work with new iPod collection, UMS collection > (depends on previous "MetaFile: guess album artist as in SQL collection > scanner, compilations"), all tests still pass. > > Only area that received just very basic testing are online > service collections that are also affected by this change. > > DIGEST: Albums with same name but different album artist are now > correctly separated in USB Mass Storage, iPod and various > online service collections. > > > Diffs > - > > ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b > src/core-impl/collections/daap/daapreader/Reader.cpp > 9f749c5fe892f0cbcbacd8454d8fc80940f6bb43 > src/core-impl/collections/ipodcollection/handler/IpodHandler.h > ceccd7cc3028051d9b93201caea180a10af15ed3 > src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp > 83c5d27839dcb5f18a4233daa5367fb18962c7af > src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp > e892952ba0ede760a66e71ffa9e43c123b74a6d7 > > src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp > 46000b3f1950cc64feef8321bcf698980c2742fe > src/core-impl/collections/playdarcollection/PlaydarCollection.cpp > 1e598f0d20ea88bef4c60877eefb71a8b934218d > src/core-impl/collections/support/MemoryCollection.h > e2b28dea72393456f5c62ecf06ce7832b49e2e4f > src/core-impl/collections/support/MemoryMatcher.cpp > 624d6d4c0eb63745293d083982b936e5c050ff0a > src/core-impl/collections/support/MemoryMeta.h > 45eefb7a4c085fa32478799e296579eef117072b > src/core-impl/collections/support/MemoryMeta.cpp PRE-CREATION > src/core-impl/coll
Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/ --- Review request for Amarok, Bart Cerneels and Ralf Engels. Description --- MetaFile: guess album artist as in SQL collection scanner, compilations Call albumArtist = ArtistHelper::bestGuessAlbumArtist( albumArtist, artist, genre, composer ); afer the data are read from file tags. This makes album artist field consistent for tracks in Local collection and tracks in file browser or drag & dropped to Amarok playlist, but it has some downsides: * Amarok will essentially lie about what album artist is assigned to a track, most notably will replace empty album artist by track artist and will replace "Various Artists" by empty artist. * Editing album artist will be strange in corner cases: in particular if user sets album artist to "Various Artists", it will be written to file tags as-is, but displayed as empty album artist. Other case would be when user sets album artist to empty string, but this is currently prevented due to a bug/feature in TagDialog. (every occurrence of "Various Artists" also accepts localized version of the string) Above change is needed for upcoming "MemoryCollection: use also album artist as identifying key in album map" change for UmsCollection, but if someone opposes, similar change can be made on UmsCollection level. Also, implement MetaFile::FileAlbum isCompilation() which is now easy as it can return albumArtist.isEmpty() DIGEST: unify album artist handling in Amarok MemoryCollection: use also album artist as identifying key in album map This has some far-reaching consequences which I believe are only positive, most notably Amarok is able to differentiate between 2 albums with same name but different album artists in USB Mass Storage collection and in "new" iPod collection. This unifies album artist handling across MemoryCollection and SqlCollection, 2 major collection storage implementations in Amarok. Technically, AlbumMap is changed from: typedef QMap AlbumMap; to: class AlbumMap : public QMap with insert(), remove(), value(), contains() QMap methods hidden and reimplemented for convenience and to prevent coding mistakes. (you no longer can add album under wrong key) This change is tested to work with new iPod collection, UMS collection (depends on previous "MetaFile: guess album artist as in SQL collection scanner, compilations"), all tests still pass. Only area that received just very basic testing are online service collections that are also affected by this change. DIGEST: Albums with same name but different album artist are now correctly separated in USB Mass Storage, iPod and various online service collections. Diffs - ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b src/core-impl/collections/daap/daapreader/Reader.cpp 9f749c5fe892f0cbcbacd8454d8fc80940f6bb43 src/core-impl/collections/ipodcollection/handler/IpodHandler.h ceccd7cc3028051d9b93201caea180a10af15ed3 src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp 83c5d27839dcb5f18a4233daa5367fb18962c7af src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp e892952ba0ede760a66e71ffa9e43c123b74a6d7 src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp 46000b3f1950cc64feef8321bcf698980c2742fe src/core-impl/collections/playdarcollection/PlaydarCollection.cpp 1e598f0d20ea88bef4c60877eefb71a8b934218d src/core-impl/collections/support/MemoryCollection.h e2b28dea72393456f5c62ecf06ce7832b49e2e4f src/core-impl/collections/support/MemoryMatcher.cpp 624d6d4c0eb63745293d083982b936e5c050ff0a src/core-impl/collections/support/MemoryMeta.h 45eefb7a4c085fa32478799e296579eef117072b src/core-impl/collections/support/MemoryMeta.cpp PRE-CREATION src/core-impl/collections/upnpcollection/UpnpCache.h da00fd1b11306ef49e10b703f601147a5762e18e src/core-impl/collections/upnpcollection/UpnpCache.cpp 58130795298911ab1849fdc64314b8ed128df3d4 src/core-impl/meta/file/File_p.h af2dbf670e23357dca0efc48421c7d0d6f884f32 src/core/meta/support/MetaKeys.h f4ffa0eb9889787b1cebb9ea4781930fbcf3fd01 src/core/meta/support/MetaKeys.cpp 3e8f0fa8587badf044c6a00806230adfe257a6fd src/services/ampache/AmpacheServiceQueryMaker.cpp 72862b917fb397b02187d14cd97c1c2c5fa4f79d tests/TestTrackOrganizer.cpp 75faf04b332e9e9c0ff279037ae66053e1011cd4 tests/browsers/TestSingleCollectionTreeItemModel.cpp 99ec36f7eae0560024669e63ca5102829d610748 tests/synchronization/TestMasterSlaveSynchronizationJob.cpp 707e578d042c3b13ecea7069c1d8b496896a3c5c tests/synchronization/TestOneWaySynchronizationJob.cpp 058611fd8fdde491e6b2eb62c5895e20324613b1 tests/synchronization/TestUnionJob.cpp 591d695d1ea0641951e5aa7188a69
