Review Request 109781: Bug #312407 - don't transcode from mp3 to mp3

[Matěj Laitl]

> On April 3, 2013, 10:11 p.m., Matěj Laitl wrote:
> > src/core/transcoding/TranscodingConfiguration.h, line 61
> > <http://git.reviewboard.kde.org/r/109781/diff/4/?file=127284#file127284line61>
> >
> >     You've introduced and enum, why do you accept an int? Also please rename transConfig to trackSelection and don't provide default value for it.
> 
> Anmol Ahuja wrote:
>     But with the default value, don't calls like:
>     Transcoding::Configuration configuration( Transcoding::JUST_COPY );
>     seem better than
>     Transcoding::Configuration configuration( Transcoding::JUST_COPY, Transcoding::Configuration::TranscodeAll );

Oh, you're right, please keep the default value then. :-) (perhaps at the other place too, if it makes sense)

Also please mention somewhere in documentation that the TrackSelection is not used if the encoder is Transcoding::JUST_COPY.
On the other hand, TrackSelection value *should* be used when encoder is Transcoding::INVALID (meaning ask every time) - it should serve as a default value for the radio button in the Transcode dialog (please make it so if it isn't already the case); documentation of this behaviour somewhere would be nice, too.


- Matěj


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109781/#review30336
-----------------------------------------------------------


On April 1, 2013, 2:46 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109781/
> -----------------------------------------------------------
> 
> (Updated April 1, 2013, 2:46 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Added 3 checkbox in the TranscodingAssistantDialog:
> "Transcode all tracks" - transcode all tracks, the current default behavior
> "Ignore files which're already the selected format" - transcode only if source and setination file formats are different
> "Transcode only when needed for playability" - transcode only when needed for playability in the destination collection
> 
> 
> Diffs
> -----
> 
>   ChangeLog 8db61e8 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 11fa33e 
>   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp e0ba0ac 
>   src/core/transcoding/TranscodingConfiguration.h 98b2bb8 
>   src/core/transcoding/TranscodingConfiguration.cpp f97a43d 
>   src/transcoding/TranscodingAssistantDialog.h 76287a7 
>   src/transcoding/TranscodingAssistantDialog.cpp 6bba0ec 
>   src/transcoding/TranscodingAssistantDialog.ui 2505bd3 
>   src/transcoding/TranscodingJob.h 6170c2a 
>   src/transcoding/TranscodingJob.cpp cab76a7 
>   src/transcoding/TranscodingOptionsStackedWidget.h 9495d44 
>   src/transcoding/TranscodingOptionsStackedWidget.cpp cf4f328 
> 
> Diff: http://git.reviewboard.kde.org/r/109781/diff/
> 
> 
> Testing
> -------
> 
> Works as expected
> All build tests passed
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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