Re: Review Request: Load all tracks from XSPF using MetaProxy.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104256/ --- (Updated March 14, 2012, 10:54 a.m.) Review request for Amarok. Changes --- This one should build. Description --- Load all tracks from XSPF using MetaProxy. Added a worker that does the actual trackForUrl call and subscribes to trackProviderAdded signal as needed. This addresses bug 295199. https://bugs.kde.org/show_bug.cgi?id=295199 Diffs (updated) - src/CMakeLists.txt 4241e69 src/core-impl/collections/support/CollectionManager.cpp 37fe03a src/core-impl/meta/proxy/MetaProxy.h d8329be src/core-impl/meta/proxy/MetaProxy.cpp d1577a2 src/core-impl/meta/proxy/MetaProxyWorker.h PRE-CREATION src/core-impl/meta/proxy/MetaProxyWorker.cpp PRE-CREATION src/core-impl/meta/proxy/MetaProxy_p.h 792675d src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 4cb49fb src/core/collections/support/TrackForUrlWorker.h 64d23bd src/core/collections/support/TrackForUrlWorker.cpp 7e8f289 src/services/ampache/AmpacheServiceCollection.h a48c8f2 src/services/ampache/AmpacheServiceCollection.cpp b684e34 Diff: http://git.reviewboard.kde.org/r/104256/diff/ Testing --- Loaded XSPF attached to https://bugs.kde.org/show_bug.cgi?id=295199 Could impact AmpacheService. Not checked yet. Thanks, Bart Cerneels ___ 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/#review11394 --- Ship it! I think in this case the use of a Capability is completely justified. It's the Capabilities that just add complexity that are problematic. Screenshot: Changes to the Configure Collection dialog http://git.reviewboard.kde.org//r/104213/#scomment32 If there are = 3 options, don't use a combobox. Screenshot: Revamped Transcode dialog http://git.reviewboard.kde.org//r/104213/#scomment33 This is hard to understand and contains some language errors. Perhaps Use this for next tracks ? - Bart Cerneels On March 9, 2012, 11:31 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 9, 2012, 11:31 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. [1] https://git.reviewboard.kde.org/r/103752/ FEATURE: 280526 FEATURE: 264681 CCBUG: 291722 BUG: 263775 FIXED-IN: 2.6 REVIEW: TODO DIGEST: Feature: much improved transcoding -- Next commit squelched for the purpose of review board -- 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
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote: I think in this case the use of a Capability is completely justified. It's the Capabilities that just add complexity that are problematic. Thanks for review! On March 14, 2012, 11:10 a.m., Bart Cerneels wrote: Screenshot: Changes to the Configure Collection dialog http://git.reviewboard.kde.org If there are = 3 options, don't use a combobox. Yup, there are 2 or 3. Should I use radio buttons? On March 14, 2012, 11:10 a.m., Bart Cerneels wrote: Screenshot: Revamped Transcode dialog http://git.reviewboard.kde.org This is hard to understand and contains some language errors. Perhaps Use this for next tracks ? I particularly suck at english. :) I wanted to justify that this option will be used for both copying and moving and that the setting is used just for the particular collection. Additionally, even the Just copy will be remembered. All suggestions for the GUI string that will make these clear are welcome. - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104213/#review11394 --- On March 9, 2012, 11:31 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 9, 2012, 11:31 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. [1] https://git.reviewboard.kde.org/r/103752/ FEATURE: 280526 FEATURE: 264681 CCBUG: 291722 BUG: 263775 FIXED-IN: 2.6 REVIEW: TODO DIGEST: Feature: much improved transcoding
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote: Screenshot: Changes to the Configure Collection dialog http://git.reviewboard.kde.org If there are = 3 options, don't use a combobox. Matěj Laitl wrote: Yup, there are 2 or 3. Should I use radio buttons? Use a groupbox: http://qt-project.org/doc/qt-4.8/widgets-groupbox.html It might make the dialog to tall. Might not be worth it for such an advanced feature. Add a screenshot please so we can see what works. Please :) On March 14, 2012, 11:10 a.m., Bart Cerneels wrote: Screenshot: Revamped Transcode dialog http://git.reviewboard.kde.org This is hard to understand and contains some language errors. Perhaps Use this for next tracks ? Matěj Laitl wrote: I particularly suck at english. :) I wanted to justify that this option will be used for both copying and moving and that the setting is used just for the particular collection. Additionally, even the Just copy will be remembered. All suggestions for the GUI string that will make these clear are welcome. It's still not clear to me. But perhaps just Save settings ? - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104213/#review11394 --- On March 9, 2012, 11:31 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 9, 2012, 11:31 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. [1] https://git.reviewboard.kde.org/r/103752/ FEATURE: 280526 FEATURE: 264681