----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109781/#review31374 -----------------------------------------------------------
Thanks, this starts to look really well, although see below for my usual series of nitpicks (hopefully the last) ChangeLog <http://git.reviewboard.kde.org/r/109781/#comment23388> Add "... moving; patch by Anmol Ahuja (BR ...", mind hard 90-char line length limit. src/core-impl/collections/umscollection/UmsCollectionLocation.h <http://git.reviewboard.kde.org/r/109781/#comment23391> Oh, I don't think there's a need to add items in batches, I just thought you'd just add void addTranscode( const KUrl &from, const KUrl &to ); in addition to void addCopy( const KUrl &from, const KUrl &to ); (yes, please remove the virtual specifiers) src/core-impl/collections/umscollection/UmsCollectionLocation.cpp <http://git.reviewboard.kde.org/r/109781/#comment23392> Additionally, this would be confusing, as addCopy would in fact *set* (overwrite) the existing lists. src/core/transcoding/TranscodingConfiguration.cpp <http://git.reviewboard.kde.org/r/109781/#comment23394> redundant include src/transcoding/TranscodingAssistantDialog.ui <http://git.reviewboard.kde.org/r/109781/#comment23397> Oh, this is a candidate for some serious layout cleanup, 7 nested layouts are really awful. I think: 1. "Transcode" groupBox should have vertical layout instead of the horizontal one 2. the radiobuttons you add should be direct children of "groupBox" 3. "horizontalLayout" (widget name) should be a direct child of "groupBox" 4. verticalLauout_5 and verticalLayout_6 don't need to exist - just place the QLabels directly under the parent horizonal layout. src/transcoding/TranscodingSelectConfigWidget.h <http://git.reviewboard.kde.org/r/109781/#comment23386> I don't think you set this value anywhere anymore. (there's a switch case for it, but it now newer fires AFAICS) src/transcoding/TranscodingSelectConfigWidget.cpp <http://git.reviewboard.kde.org/r/109781/#comment23387> The whole i18n and "Always (%1)" should go away, just display prettyName() directly - mind your changes to it. src/transcoding/TranscodingSelectConfigWidget.cpp <http://git.reviewboard.kde.org/r/109781/#comment23395> We split declarations of 2 same-typed variables to 2 lines per Amarok coding style: Configuration a; Configuration b; - Matěj Laitl On April 19, 2013, 8:17 a.m., Anmol Ahuja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109781/ > ----------------------------------------------------------- > > (Updated April 19, 2013, 8:17 a.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 destination file formats are different > "Transcode only when needed for playability" - transcode only when needed for > playability in the destination collection > > > Diffs > ----- > > ChangeLog cc8b166 > src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 8bc4552 > src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 3c2d9f2 > src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c > src/core-impl/collections/support/CollectionLocationDelegateImpl.h b59beec > src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp > f19cd9d > src/core-impl/collections/umscollection/UmsCollectionLocation.h 45ba596 > src/core-impl/collections/umscollection/UmsCollectionLocation.cpp 2a30c9f > src/core/collections/CollectionLocation.cpp 08f6910 > src/core/collections/CollectionLocationDelegate.h 6316d6f > src/core/transcoding/TranscodingConfiguration.h 98b2bb8 > src/core/transcoding/TranscodingConfiguration.cpp f97a43d > src/transcoding/TranscodingAssistantDialog.h 76287a7 > src/transcoding/TranscodingAssistantDialog.cpp 8348b12 > src/transcoding/TranscodingAssistantDialog.ui 2505bd3 > src/transcoding/TranscodingOptionsStackedWidget.h 9495d44 > src/transcoding/TranscodingOptionsStackedWidget.cpp cf4f328 > src/transcoding/TranscodingSelectConfigWidget.h aa5f196 > src/transcoding/TranscodingSelectConfigWidget.cpp adb593c > 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 > ---------------- > > TranscodingAssistantDialog > > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/18/TranscodingAssistantDialog_1.png > > > Thanks, > > Anmol Ahuja > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
