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

Bart Cerneels bart.cerneels at kde.org
Wed Mar 14 13:12:19 UTC 2012



> 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
> 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 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.
> 
> --------------------------------------------------------------------------------------------------
> Next commit squelched for the purpose of review board
> --------------------------------------------------------------------------------------------------
> 
> 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 6f318678272275bb6de35fd8cd6355dd9c175f2d 
>   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 12b975fc7d5c5d56e314f3fce4384eb559e88274 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f6fef5ef634ef1a40cea604f12a379cfc6 
>   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/457/
> Revamped Transcode dialog
>   http://git.reviewboard.kde.org/r/104213/s/458/
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120314/376c5cec/attachment-0001.html>


More information about the Amarok-devel mailing list