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

Wed Apr 3 22:11:27 UTC 2013

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

Good start, but I propose some architectural changes, please see below.

src/core-impl/collections/db/sql/SqlCollectionLocation.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22584>

    Hmm, what if TranscodingConfiguration::isJustCopy() took a const TrackPtr &track argument and would decide whether to copy the track or not based on passed track->format()?

    nitpick: we usualy strip { }'s for single-statement if blocks.

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22585>

    Better name is needed, I propose TrackSelection. Also, space before {

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22587>

    nitpick: space between * and Transcode

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22586>

    Please strip val prefix from all enum values, no need to have it. I also don't understand "none selected"

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22592>

    These are not flags, but mutually exclusive options, number them 0, 1, 2, 3 ... (normally we don't specify the numbers in this case, but these numbers will get written to files, so we need to preserve the numbers and rather specify them explicitly - please document that)

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22588>

    Better to keep naming convention: TranscodeUnlessSameType

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22590>

    Well, the thing is not specific to MP3, that was just an example; also, the logic is opposite.

    The correct definition would be:
    Transcode *unless*:
    a) the target format is the same as source format
    *and*
    b) the target bitrate is higher than source bitrate (with 10% tolerance to prevent reencodings)

    Reason: it makes absolutely no sense to transcode to the same format with higher bitrate.

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22589>

    Consistent naming:
    TranscodeUnlessRaisesBitrate
    also please comment the line out until this is implemented

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22591>

    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.

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22593>

    No, just:
    TrackSelection trackSelection() const;

src/core/transcoding/TranscodingConfiguration.h
<http://git.reviewboard.kde.org/r/109781/#comment22594>

     * enum defined above, not int
     * code is written once and read many times, therefore we discourage use of abbreviations + more descriptive name would be: TrackSelection m_trackSelection;

    nitpick: I would strip the newlive above.

src/core/transcoding/TranscodingConfiguration.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22595>

    You'll have to extend this method

src/core/transcoding/TranscodingConfiguration.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22596>

    You'll have to extend this method.

src/core/transcoding/TranscodingConfiguration.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22599>

    Make this "Ask Before Each Transfer"

src/core/transcoding/TranscodingConfiguration.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22600>

    Make this "Never"

src/core/transcoding/TranscodingConfiguration.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22598>

    Integrate code from Transcoding::SelectConfigWidget::fillInChoices() here so that it reads (MP3 and the bitrate are just examples):

    All Tracks to MP3, VBR 175 kb/s
    Non-MP3 Tracks to MP3, VBR 175 kb/s
    When Needed to MP3, VBR 175 kb/s

src/transcoding/TranscodingAssistantDialog.h
<http://git.reviewboard.kde.org/r/109781/#comment22602>

    No abbreviations please, also please adapt to new naming in TranscodingConfiguration

src/transcoding/TranscodingAssistantDialog.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22603>

    No, please don't reimplement radio button functionality yourself. ;)

src/transcoding/TranscodingAssistantDialog.ui
<http://git.reviewboard.kde.org/r/109781/#comment22601>

    These should be radio buttons because the options are mutually exclusive. Also please provide a screenshot of the modified dialog.

src/transcoding/TranscodingJob.h
<http://git.reviewboard.kde.org/r/109781/#comment22604>

    No, just don't create the job if transcoding shouldn't happen. (and don't touch the code of the TanscondingJob)

src/transcoding/TranscodingJob.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22605>

    This method will go away, but I just want to let you know that guessing format from file extension is unreliable and discouraged.

    Use Meta::Track::type() instead.

src/transcoding/TranscodingOptionsStackedWidget.h
<http://git.reviewboard.kde.org/r/109781/#comment22606>

    Please don't provide default value, every called should specify it.

- Matěj Laitl

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/20130403/9ce562a2/attachment-0001.html>