<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/100068/">http://git.reviewboard.kde.org/r/100068/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1002#file1002line1134" style="color: black; font-weight: bold; text-decoration: underline;">src/browsers/CollectionTreeView.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void CollectionTreeView::slotEditTracks()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1134</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Transcoding</span><span class="o">::</span><span class="n">Configuration</span> <span class="n">configuration</span> <span class="o">=</span> <span class="n">Transcoding</span><span class="o">::</span><span class="n">Configuration</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This should really be moved into CollectionLocation, as a first step before CollectionLocation starts calling its sub-classes to do the actual work.
Oh, and what about automatic transcoding, e.g. when copying oggs to an ipod?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1004#file1004line328" style="color: black; font-weight: bold; text-decoration: underline;">src/browsers/filebrowser/FileView.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">FileView::slotCopyTracks( const Meta::TrackList& tracks )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">327</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Transcoding</span><span class="o">::</span><span class="n">Configuration</span> <span class="n">configuration</span> <span class="o">=</span> <span class="n">Transcoding</span><span class="o">::</span><span class="n">Configuration</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This looks like duplicated code, the same as in CollectionTreeView.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1004#file1004line329" style="color: black; font-weight: bold; text-decoration: underline;">src/browsers/filebrowser/FileView.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">FileView::slotCopyTracks( const Meta::TrackList& tracks )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">328</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">dialog</span><span class="p">.</span><span class="n">exec</span><span class="p">()</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This method blocks, which forces Qt to start another event loop. This should be avoided if possible</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1017#file1017line41" style="color: black; font-weight: bold; text-decoration: underline;">src/core/transcoding/TranscodingController.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Controller *</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">41</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">Controller</span><span class="o">::</span><span class="n">instance</span><span class="p">()</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Static instance methods are evil. That way it becomes really hard to mock the Controller. Please use an interface and a registry, i.e. Amarok::Components to get the necessary indirection to avoid the dependency hell created by static factory methods.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1017#file1017line86" style="color: black; font-weight: bold; text-decoration: underline;">src/core/transcoding/TranscodingController.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Controller *</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">for</span><span class="p">(</span> <span class="n">QList</span><span class="o"><</span> <span class="n">Format</span> <span class="o">*</span> <span class="o">>::</span><span class="n">const_iterator</span> <span class="n">it</span> <span class="o">=</span> <span class="n">m_formats</span><span class="p">.</span><span class="n">constBegin</span><span class="p">();</span> <span class="n">it</span> <span class="o">!=</span> <span class="n">m_formats</span><span class="p">.</span><span class="n">constEnd</span><span class="p">();</span> <span class="o">++</span><span class="n">it</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">How about a foreach loop instead?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100068/diff/1/?file=1045#file1045line77" style="color: black; font-weight: bold; text-decoration: underline;">src/transcoding/TranscodingJob.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Job::init()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">77</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o"><<</span> <span class="n">m_src</span><span class="p">.</span><span class="n">path</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">How is this going to work for non-local source tracks?</pre>
</div>
<br />
<p>- Maximilian</p>
<br />
<p>On October 17th, 2010, 8:51 p.m., Teo Mrnjavac wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Teo Mrnjavac.</div>
<p style="color: grey;"><i>Updated 2010-10-17 20:51:11</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The attached diff is the work done during this summer to implement transcoding, it should apply to master as of right now. There are some known issues with how FFmpeg behaves with metadata, and I'm not sure I made all the right choices with how I modified CollectionLocation-related stuff.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The patch is the result of git diff for the range of several dozen transcoding-related commits prior to HEAD, rebased on current master, and should build. Transcoding from file browser to collection and inside sqlcollection should definitely work. FFmpeg is required, and Amarok should detect the encoders that are supported by your build of FFmpeg at runtime and populate the dialog accordingly.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/App.cpp <span style="color: grey">(445fdc3)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(f8559c4)</span></li>
<li>src/browsers/CollectionTreeView.h <span style="color: grey">(63c99b5)</span></li>
<li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(ed34beb)</span></li>
<li>src/browsers/filebrowser/FileView.h <span style="color: grey">(11547fe)</span></li>
<li>src/browsers/filebrowser/FileView.cpp <span style="color: grey">(9cb7c27)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h <span style="color: grey">(f65cd4c)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp <span style="color: grey">(a858d44)</span></li>
<li>src/core-impl/collections/sqlcollection/CMakeLists.txt <span style="color: grey">(f530d67)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp <span style="color: grey">(a1966ac)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlCollectionLocation.h <span style="color: grey">(ec3a6d8)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlCollectionLocation.cpp <span style="color: grey">(4b023fc)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(5863ca1)</span></li>
<li>src/core/collections/CollectionLocation.h <span style="color: grey">(567f6d3)</span></li>
<li>src/core/collections/CollectionLocation.cpp <span style="color: grey">(dbf3b37)</span></li>
<li>src/core/transcoding/TranscodingConfiguration.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingConfiguration.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingController.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingController.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingDefines.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingProperty.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/TranscodingProperty.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingAacFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingAacFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingAlacFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingAlacFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingFlacFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingFlacFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingMp3Format.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingMp3Format.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingNullFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingNullFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingVorbisFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingVorbisFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingWmaFormat.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/transcoding/formats/TranscodingWmaFormat.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/dialogs/OrganizeCollectionDialog.h <span style="color: grey">(2c0adf7)</span></li>
<li>src/dialogs/OrganizeCollectionDialog.cpp <span style="color: grey">(2607e99)</span></li>
<li>src/dialogs/TrackOrganizer.h <span style="color: grey">(9d62467)</span></li>
<li>src/dialogs/TrackOrganizer.cpp <span style="color: grey">(135d520)</span></li>
<li>src/transcoding/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingAssistantDialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingAssistantDialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingAssistantDialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingOptionsStackedWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingOptionsStackedWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertyComboBoxWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertyComboBoxWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertySliderWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertySliderWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertySpinBoxWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertySpinBoxWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertyWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/transcoding/TranscodingPropertyWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/collections/sqlcollection/TestSqlCollectionLocation.cpp <span style="color: grey">(671759f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100068/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>