Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code

Commit Hook null at kde.org
Fri Mar 16 20:09:59 UTC 2012 

-----------------------------------------------------------
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 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 d0ccdb33e8e54ecf4db205e10dbd8396eac488ad 
>   src/core-impl/collections/db/sql/SqlCollection.h 3a96bea1aa21761f12d956f7905f87808e0efc4a 
>   src/core-impl/collections/db/sql/SqlCollection.cpp 79714153661e50b70885b3e82cb6529b79ff98e2 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.h 1db72726d041713158be0fe91a93be3b1044b70e 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 2c33da62565a9be4b5710cce590bac99d28b47ae 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h 24bb306c5b28a6d21f66f598f4caccbbb60f5bf8 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.h 08a1762bb4c72b7ba32658651e00bf356e2cffba 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 23986570a42ad78557a394581233921abc668a8f 
>   src/core-impl/collections/support/PlaylistCollectionLocation.h 967d2150cf2c76f563dac0ff5396e84448826aed 
>   src/core-impl/collections/support/TrashCollectionLocation.h a9c92495abebd13506ed52a185355f21b55ee347 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.h 3fc0af405a8d47374afacdf5c1190d112b126d21 
>   src/core/CMakeLists.txt 4db2105f08246898585bbe38ba4fbe0d006bd138 
>   src/core/capabilities/Capability.h 42ffd7e89fd67d1b1b1b0b6f3a8ddc35cde5943e 
>   src/core/capabilities/TranscodeCapability.h PRE-CREATION 
>   src/core/capabilities/TranscodeCapability.cpp PRE-CREATION 
>   src/core/collections/CollectionLocation.h 35480b2000cccf9ece99b7654ff3852087b3144e 
>   src/core/collections/CollectionLocation.cpp 9b258ce9a19ab22f7027b674da4b76d1f15d973b 
>   src/core/collections/CollectionLocationDelegate.h c49a7445e260665e508795ae8dbee4ac9f271f71 
>   src/core/transcoding/TranscodingConfiguration.h 378fe12ffa86764fe3ecf0cc5f2a38d8a47a0561 
>   src/core/transcoding/TranscodingConfiguration.cpp 40f09be71ee0c75cbeeb6d3a4702c2a6f5ec3b99 
>   src/core/transcoding/TranscodingController.h e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3 
>   src/core/transcoding/TranscodingController.cpp e16b6fe7d3d60621ef0a237951ac038f7846b51e 
>   src/core/transcoding/TranscodingDefines.h 4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c 
>   src/core/transcoding/TranscodingFormat.h 5bf4e6098be4f3e08dcd45c48fdd264418660293 
>   src/core/transcoding/TranscodingProperty.h 928854baa31eb4e78b3fe80768eb4f9811a0f5bf 
>   src/core/transcoding/TranscodingProperty.cpp 2b6752cfbc39e37880b05930dd9e797d60200c97 
>   src/core/transcoding/formats/TranscodingNullFormat.h 96393e80f9a74a34a6858fcc114f2719089fac71 
>   src/core/transcoding/formats/TranscodingNullFormat.cpp 98f09d1853833d2fd8b8de0603612ae337c6ef52 
>   src/services/magnatune/MagnatuneCollectionLocation.cpp d75ea9edd0af9317b66f223961d325635fb26e33 
>   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h c168a1f7bb07e798702b01f488912fd0b2c191c5 
>   src/transcoding/CMakeLists.txt 06d5be85075581e979dc5fd8b411286200049846 
>   src/transcoding/TranscodingAssistantDialog.h 718c01d40fed0113087b90e425a9102b365076cb 
>   src/transcoding/TranscodingAssistantDialog.cpp b64e74b9ee75c8b597a07c008a4e161333bb0d86 
>   src/transcoding/TranscodingAssistantDialog.ui 912a1c2d636f4b24b1efd94185c0ae41f11ee96a 
>   src/transcoding/TranscodingOptionsStackedWidget.cpp 54c936dd041d9e23820afc8dbde662b5bb0dcd46 
>   src/transcoding/TranscodingPropertyComboBoxWidget.h ecab1583fcb27ab78497e9c56f8c26a0e9a8b05a 
>   src/transcoding/TranscodingPropertyComboBoxWidget.cpp dbb2462f526ad5b47c14efa5c0f0983db296cb20 
>   src/transcoding/TranscodingPropertySliderWidget.cpp 0a49da06127d9fa4dde9f8df95650e1a9a5ed6bc 
>   src/transcoding/TranscodingPropertySpinBoxWidget.h f1fa7f561c518f7420fc904252e35e8c107dd707 
>   src/transcoding/TranscodingPropertySpinBoxWidget.cpp dcc34efa2c66e1f7be2af64524be238cd4f2fe8e 
>   src/transcoding/TranscodingPropertyWidget.cpp 4767c5208542b3815a4ca57023150bc2f5c7cf42 
>   src/transcoding/TranscodingSelectConfigWidget.h PRE-CREATION 
>   src/transcoding/TranscodingSelectConfigWidget.cpp PRE-CREATION 
>   tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp 7e90d1cd5acc6ef101897ad277c3a2f992bf3150 
>   tests/core/collections/MockCollectionLocationDelegate.h a58ca4b344e98f6b75da19cdd6b38172ecc95f59 
> 
> Diff: http://git.reviewboard.kde.org/r/104213/diff/
> 
> 
> Testing
> -------
> 
> Works, both with Local Collection and in-the-works iPod collection.
> 
> 
> Screenshots
> -----------
> 
> Better Organize dialog title
>   http://git.reviewboard.kde.org/r/104213/s/456/
> Changes to the Configure Collection dialog
>   http://git.reviewboard.kde.org/r/104213/s/467/
> Revamped Transcode dialog
>   http://git.reviewboard.kde.org/r/104213/s/468/
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120316/cb9a71a8/attachment.html>

More information about the Amarok-devel mailing list