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

Sun Apr 14 14:53:43 UTC 2013

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

Good direction, although please see suggestions below to make the patch perfect. Also please adapt Transcoding::SelectConfigWidget::fillInChoices() to cope with new Transcoding::Configuration::prettyName() semantics and to show also all other TrackSelection choices for given encoder and parameters.

File Attachment: Transcoding-Assistant Dialog
<http://git.reviewboard.kde.org//r/109781/#fcomment38>
    Missing "are", I would strip the "file" from "file formats" as it should be obvious.

File Attachment: Transcoding-Assistant Dialog
<http://git.reviewboard.kde.org//r/109781/#fcomment39>
    Please put the 3 radios under the the "Transcode" group so that it is clear that the checkbox below applies to everything

src/core-impl/collections/umscollection/UmsCollectionLocation.h
<http://git.reviewboard.kde.org/r/109781/#comment23060>

    Hmm, m_sourceUrlToTrackMap looks hacky. What if you had:

    QList<KUrlPair> m_copyList;
    QList<KUrlPair> m_transcodeList;

    and

    void addTranscode( ..., ... );

    instead of m_transferList?

src/core/collections/CollectionLocation.cpp
<http://git.reviewboard.kde.org/r/109781/#comment22979>

    I think that just removing the configuration.isValid() from the condition would have exactly the same effect, wouldn't it?

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

    nitpick: the const specifier doesn't make much sense for passed-by-value arguments - therefore we don't usually use it.

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

    Hmm, I don't think playableFileTypes should be part of the configuration. Instead, it should probably become a parameter of isJustCopy(), defaulting to empty QStringList(), which would mean "every format is playable".

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

    I don't think you need this - that's why you document the enum values that they shouldn't be changed.

    Formatting nitpick: we usually add a blank line before a documented method.

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

    Ditto shouldn't be needed.

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

    Shouldn't be inside the configuration.

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

    include style: use "something.h" for Amarok includes

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

    TrackSelection trackSelection = TrackSelection( serialized.readEntry( "TrackSelection", int( TranscodeAll ) ) );

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

    grammar: it's == it is; its == possessive pronoun

    nitpick: I usually add space between // and actual comment

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

    group.writeEntry( QLatin1String( "TrackSelection" ), int( m_trackSelection ) );

    (and please remove the surrounding blank lines)

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

    Please mention in the context (make the calls i18nc()) that the value is displayed next to "Transcode:" label

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

    ditto about Transcode: label

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

    else
        return false;

    would make code reading less hard.

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

    nitpick: space before and after //

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

    else
        return false;

    would be better

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

    Handling TranscodeAll and removing the default would make the code more readable and also let the compiler warn us in future when we add the enum value and forgot to handle it here.

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

    Please make the string so that it goes well with a "Transcode:" label and title-cased, for example: "All Tracks to %1".  Also please mention in the context that the value is displayed next to "Transcode:" label (all 3 occasions)

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

    I would add the word "track":

    "Non-%1 Tracks to %1"

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

    Handling TranscodeAll instead of the default case behaves better wrt future additions and compiler warnings

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

    Won't be needed in the next patch version.

- Matěj Laitl

On April 7, 2013, 5: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 7, 2013, 5: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
> -----
> 
>   src/transcoding/TranscodingAssistantDialog.cpp 8348b12 
>   src/transcoding/TranscodingAssistantDialog.ui 2505bd3 
>   src/transcoding/TranscodingAssistantDialog.h 76287a7 
>   src/core/transcoding/TranscodingConfiguration.cpp f97a43d 
>   src/core/transcoding/TranscodingConfiguration.h 98b2bb8 
>   src/core/collections/CollectionLocationDelegate.h 6316d6f 
>   src/core/collections/CollectionLocation.cpp 08f6910 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp 2a30c9f 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.h 45ba596 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp f19cd9d 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.h b59beec 
>   src/core-impl/collections/ipodcollection/support/IpodTranscodeCapability.cpp 5850e71 
>   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c 
>   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 3c2d9f2 
>   ChangeLog 7b394ac 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 8bc4552 
>   src/transcoding/TranscodingOptionsStackedWidget.h 9495d44 
>   src/transcoding/TranscodingOptionsStackedWidget.cpp cf4f328 
>   tests/core/collections/MockCollectionLocationDelegate.h 5efbe2e7 
> 
> Diff: http://git.reviewboard.kde.org/r/109781/diff/
> 
> 
> Testing
> -------
> 
> Works as expected
> All build tests passed
> 
> 
> File Attachments
> ----------------
> 
> Transcoding-Assistant Dialog
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/04/07/snapshot12.png
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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