Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
Matěj Laitl
matej at laitl.cz
Fri Mar 16 16:01:02 UTC 2012
-----------------------------------------------------------
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 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 (updated)
-----------
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/91ad54aa/attachment.html>
More information about the Amarok-devel
mailing list