Review Request: Restore heuristics to guess whether album is a compilation
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104294/ --- Review request for Amarok. Description --- Older amarok used the following heuristics to determine whether a particular album is a compilation (from a comment in amarok-2.2.1/src/collection/sqlcollection/ScanResultProcessor.cpp): //using the following heuristics: //if more than one album is in the dir, use the artist of each track as albumartist //if all tracks have the same artist, use it as albumartist //try to find the albumartist A: tracks must have the artist A or A feat. B (and variants) //if no albumartist could be found, it's a compilation However, more recent Amarok versions started to merge different albums with different artist in separate directories together, as explained above. Amarok started to assume all albums with same name to be compilations (even if in separate directories) since the following commit: dfd8b457d7094144563c51b2528afdbe23ffc344 Ralf Engels Fix all collection scanner auto tests. Now, amarok first scans all directories (sorting albums by the name) and then tries to process *album names*, one at a time. If it finds more than one instance of an album name, it assumes it to be a compilation. Thus, it lost the heuristics in employed before (if more than one album is in the dir...). While it is still possible to force the right behavior by selecting Do not show under Various Artists for each of the erroneous albums, it would still be better to restore the original heuristics as there may be lots of albums merged this way. I think the old heuristics made sense (why would albums be put into separate directories otherwise, if they are a single compilation album?). The attached patch restores the following logic: If any given directory contains tracks that were sorted into a single album and and that album was not created as a compilation (i.e. it has non-empty artists), this album is excluded from being merged with other albums to create a compilation. Diffs - src/core-impl/collections/db/ScanResultProcessor.cpp 4f02a16 Diff: http://git.reviewboard.kde.org/r/104294/diff/ Testing --- Thanks, Alexey Neyman ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Restore heuristics to guess whether album is a compilation
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104294/#review11457 --- All I can comment on is the coding style, which is good :). I did notice more false positives in various artists recently. I'm not sure if these are worse then uncatched VA's. If it were up to me VA would be implemented completely in the view so it works for all collections at once and can easily be turned off. - Bart Cerneels On March 16, 2012, 12:13 a.m., Alexey Neyman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104294/ --- (Updated March 16, 2012, 12:13 a.m.) Review request for Amarok. Description --- Older amarok used the following heuristics to determine whether a particular album is a compilation (from a comment in amarok-2.2.1/src/collection/sqlcollection/ScanResultProcessor.cpp): //using the following heuristics: //if more than one album is in the dir, use the artist of each track as albumartist //if all tracks have the same artist, use it as albumartist //try to find the albumartist A: tracks must have the artist A or A feat. B (and variants) //if no albumartist could be found, it's a compilation However, more recent Amarok versions started to merge different albums with different artist in separate directories together, as explained above. Amarok started to assume all albums with same name to be compilations (even if in separate directories) since the following commit: dfd8b457d7094144563c51b2528afdbe23ffc344 Ralf Engels Fix all collection scanner auto tests. Now, amarok first scans all directories (sorting albums by the name) and then tries to process *album names*, one at a time. If it finds more than one instance of an album name, it assumes it to be a compilation. Thus, it lost the heuristics in employed before (if more than one album is in the dir...). While it is still possible to force the right behavior by selecting Do not show under Various Artists for each of the erroneous albums, it would still be better to restore the original heuristics as there may be lots of albums merged this way. I think the old heuristics made sense (why would albums be put into separate directories otherwise, if they are a single compilation album?). The attached patch restores the following logic: If any given directory contains tracks that were sorted into a single album and and that album was not created as a compilation (i.e. it has non-empty artists), this album is excluded from being merged with other albums to create a compilation. Diffs - src/core-impl/collections/db/ScanResultProcessor.cpp 4f02a16 Diff: http://git.reviewboard.kde.org/r/104294/diff/ Testing --- Thanks, Alexey Neyman ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104213/ --- (Updated March 16, 2012, 4:01 p.m.) Review request for Amarok and Teo Mrnjavac. Changes --- gui string changes as suggested by Bart Teo. I'll probably merge this later today. Description (updated) --- Rework transcoding: remember encoder, transcode on move, cleaner code This is a major rework of transcoding feature that brings following user-visible changes to Amarok: * Amarok can remember preferred transcoding configuration per each collection that supports transcoding. Therefore, the Use default configuration work-around can go away and the Transcode or copy? dialog can (and is) be one-step now. This preference can be changed in configuration. * Transcoding is now supported even during the move operation. No worries, only successfully transcoded tracks are removed from their original location. * Only formats playable on the target collection are offered. Already used tested in yet-to-be-merged iPod collection rewrite. * The Organize Tracks dialog title and progress bar operation name now more verbosely describe actual operation to prevent user mistakes. * Double-transcode when ripping audio CDs that caused failures is avoided. (ChangeLog entry for this was miscredited to my earilier commit) Technically, following changes are made: * many methods that accepted optional TranscodingConfiguration now either have it mandatory or not at all. * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and INVALID along with convenience methods isValid() and isJustCopy(). This simplifies logic in many methods. * CollectionLocation::prepare{Copy,Move}() now don't have optional TranscodingConfiguration parameter. Depending on target collection, CollectionLocation determines it automatically or asks user in showSourceDialog() (overridable). AudioCdCollectionLocation already overrides it. * Collections that support transcoding now should expose TranscodeCapability which is used to a) indicate that transcoding is supported; b) query which file formats are playable on target collection; c) read save unset preferred transcoding parameters. Why the hell the new Capability? Many Amarok devs dislike the concept of capabilities[1]. Why the hell I introduced the new one? In ideal world Amarok would be able to transcode everything regardless of the target collection. This is however not doable witch current copyUrlToCollection() design - target collection needs to do non-trivial things such as re-reading file tags, accounting for different file name and space requirements etc. See my comments in [1]. We therefore need a way for target collection to indicate it supports transcoding (in order not to fool user). Some collection locations such as TrashCollectionLocation should even intentionally disallow transcoding. Additionally, we want to be able to query supported destination file formats, to save preferred transcoding paremeters etc. I simply didn't want to pollute already over-crowded CollectionLocation with three more methods used by only a few subclasses. On the other hand, TranscodeCapability is not the central idea of this patch and I can factor it into CollectionLocation should there be a voice supporting it. v2 patch version: gui string changes as suggested by Bart Teo [1] https://git.reviewboard.kde.org/r/103752/ FEATURE: 280526 FEATURE: 264681 CCBUG: 291722 BUG: 263775 FIXED-IN: 2.6 REVIEW: 104213 DIGEST: Feature: much improved transcoding Transcoding::Property: remove NUMERIC, LIST, TEXT types These types were not used since Teo reworked all encoders to use the TRADEOFF type. Remove them and associated code to make codebase cleaner so that new code doesn't need to introduce case statements in switches that will be never used, thus error-prone. Individual types can be resurrected from this commit if there is a need for them in future. CollectionLocation: display source dialog in next mainloop iteration This is to make CollectionLocation::prepareCopy/Move() return fast as it advertises and not after several seconds when a modal dialog is shown. Diffs (updated) - ChangeLog bd777eadf39aec71efb74dfed7502f564553d998 src/CMakeLists.txt 4241e69000c8b7fb944e4c86ddff3128829fb381 src/browsers/CollectionTreeView.h 26ac68a55242d52cdb1d789cef839643b56ccf5a src/browsers/CollectionTreeView.cpp 7b3dd81b1cfd0fbb4d74b7eb71552a4ad92d74e5 src/browsers/filebrowser/FileView.h 890b9f458e7ae504a52019b8f41e3e6d2ba218a5 src/browsers/filebrowser/FileView.cpp 425641af15e66c2670bce719eff1615f416ad6b7 src/configdialog/dialogs/CollectionConfig.cpp 7704fb9fab5a09c4635c1ec7526ae05047df0c6f src/configdialog/dialogs/CollectionConfig.ui
Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721. The bookmark is movable within the slider. If it is dragged outside the range, it will revert to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark's position hasn't changed. In addition, also fixed a bug that deleted a different bookmark that shared the same name(possible by manual renaming), by appending the location of the bookmark even in case of manual renaming. Diffs - src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104213/#review11494 --- This review has been submitted with commit 1a0287f7925d92a05a530ab83fffbc9afae67e86 by Matěj Laitl to branch master. - Commit Hook On March 16, 2012, 4:01 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104213/ --- (Updated March 16, 2012, 4:01 p.m.) Review request for Amarok and Teo Mrnjavac. Description --- Rework transcoding: remember encoder, transcode on move, cleaner code This is a major rework of transcoding feature that brings following user-visible changes to Amarok: * Amarok can remember preferred transcoding configuration per each collection that supports transcoding. Therefore, the Use default configuration work-around can go away and the Transcode or copy? dialog can (and is) be one-step now. This preference can be changed in configuration. * Transcoding is now supported even during the move operation. No worries, only successfully transcoded tracks are removed from their original location. * Only formats playable on the target collection are offered. Already used tested in yet-to-be-merged iPod collection rewrite. * The Organize Tracks dialog title and progress bar operation name now more verbosely describe actual operation to prevent user mistakes. * Double-transcode when ripping audio CDs that caused failures is avoided. (ChangeLog entry for this was miscredited to my earilier commit) Technically, following changes are made: * many methods that accepted optional TranscodingConfiguration now either have it mandatory or not at all. * TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and INVALID along with convenience methods isValid() and isJustCopy(). This simplifies logic in many methods. * CollectionLocation::prepare{Copy,Move}() now don't have optional TranscodingConfiguration parameter. Depending on target collection, CollectionLocation determines it automatically or asks user in showSourceDialog() (overridable). AudioCdCollectionLocation already overrides it. * Collections that support transcoding now should expose TranscodeCapability which is used to a) indicate that transcoding is supported; b) query which file formats are playable on target collection; c) read save unset preferred transcoding parameters. Why the hell the new Capability? Many Amarok devs dislike the concept of capabilities[1]. Why the hell I introduced the new one? In ideal world Amarok would be able to transcode everything regardless of the target collection. This is however not doable witch current copyUrlToCollection() design - target collection needs to do non-trivial things such as re-reading file tags, accounting for different file name and space requirements etc. See my comments in [1]. We therefore need a way for target collection to indicate it supports transcoding (in order not to fool user). Some collection locations such as TrashCollectionLocation should even intentionally disallow transcoding. Additionally, we want to be able to query supported destination file formats, to save preferred transcoding paremeters etc. I simply didn't want to pollute already over-crowded CollectionLocation with three more methods used by only a few subclasses. On the other hand, TranscodeCapability is not the central idea of this patch and I can factor it into CollectionLocation should there be a voice supporting it. v2 patch version: gui string changes as suggested by Bart Teo [1] https://git.reviewboard.kde.org/r/103752/ FEATURE: 280526 FEATURE: 264681 CCBUG: 291722 BUG: 263775 FIXED-IN: 2.6 REVIEW: 104213 DIGEST: Feature: much improved transcoding Transcoding::Property: remove NUMERIC, LIST, TEXT types These types were not used since Teo reworked all encoders to use the TRADEOFF type. Remove them and associated code to make codebase cleaner so that new code doesn't need to introduce case statements in switches that will be never used, thus error-prone. Individual types can be resurrected from this commit if there is a need for them in future. CollectionLocation: display source dialog in next mainloop iteration This is to make CollectionLocation::prepareCopy/Move() return fast as it advertises and not after several seconds when a modal dialog is shown. Diffs - ChangeLog bd777eadf39aec71efb74dfed7502f564553d998 src/CMakeLists.txt 4241e69000c8b7fb944e4c86ddff3128829fb381 src/browsers/CollectionTreeView.h