<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/104307/">http://git.reviewboard.kde.org/r/104307/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 22nd, 2012, 9:46 p.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/104307/diff/1/?file=53586#file53586line225" style="color: black; font-weight: bold; text-decoration: underline;">src/amarokurls/AmarokUrl.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; ">void AmarokUrl::rename( const QString &name )</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">196</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">AmarokUrl</span><span class="o">::</span><span class="n">rename</span><span class="p">(</span> <span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">name</span> <span class="p">)</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">225</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">AmarokUrl</span><span class="o">::</span><span class="n">rename</span><span class="p">(</span> <span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">name</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Err, what is this? This does something that I woudn't expect and it doesn't even document why. Please explain what you try to achieve with this, with examples.</pre>
</blockquote>
<p>On March 23rd, 2012, 10:46 p.m., <b>Jasneet Bhatti</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is actually not really related to this bug. It's another bug I found: Whenever you rename two bookmarks to the same name, and then delete the bookmark that was created/renamed first, the one created later gets deleted.
So I was trying to implement the rename function in such a way that whenever the user renames the bookmark, the position of the bookmark stays appended to the bookmark name(the regular expression and subsequent conditions check if the user has kept the position or has deleted it after renaming the bookmark). But as you said, AmarokUrl is used for other kinds of bookmarks too, so I guess this method might not be a good idea. Please do suggest an alternative. Meanwhile, I'm reverting back to the original rename function.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hmm, okay. So don't include this fix in this patch, and give me some time to think more about this, because I see no quick solution right now. (Or open another review request if you come up with something)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 22nd, 2012, 9:46 p.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/104307/diff/1/?file=53588#file53588line571" style="color: black; font-weight: bold; text-decoration: underline;">src/amarokurls/BookmarkModel.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; ">BookmarkModel::moveBookmark( const QString& name, int milliseconds )</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">571</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">DEBUG_BLOCK</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Please don't add DEBUG_BLOCKs and debug() for code that you are not actually debugging. (I know, it is in other methods here, you can take this as an opportunity to remove these, too)</pre>
</blockquote>
<p>On March 23rd, 2012, 10:46 p.m., <b>Jasneet Bhatti</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't completely understand the concept of DEBUG_BLOCKs. So I tried to structure the function like other similar functions. How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> I don't completely understand the concept of DEBUG_BLOCKs
They are simple, they print BEGIN methodName() to Amarok's debugging output (amarok --debug) when method is entered and __END: methodName() when the block goes out of scope.
> How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ?
When adding new code, you know whether you debug it or not. For existing code, I usually look at the time the line was last modified (git blame shows that). If it is a year or so, the debugging was probably just left out by mistake or laziness. (Ok, there are cases where debugging printouts should be keft forefer for tricky code, these should ba scarce though)
Answering your question, probably all DEBUG_BLOCKs and debug() calls should be removed from BookmarkModel, but please don't do it in this patch, just don't add it to the new code. (because each commit should focus on just one thing)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 22nd, 2012, 9:46 p.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/104307/diff/1/?file=53589#file53589line37" style="color: black; font-weight: bold; text-decoration: underline;">src/widgets/BookmarkTriangle.h</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; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">BookmarkTriangle</span> <span class="p">(</span> <span class="n">QWidget</span> <span class="o">*</span><span class="n">parent</span><span class="p">,</span> <span class="kt">int</span> <span class="n">milliseconds</span><span class="p">,</span> <span class="n">QString</span> <span class="n">name</span> <span class="p">,</span> <span class="n">bool</span> <span class="n">showPopup</span> <span class="o">=</span> <span class="nb">false</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">BookmarkTriangle</span> <span class="p">(</span> <span class="n">QWidget</span> <span class="o">*</span><span class="n">parent</span><span class="p">,</span> <span class="kt">int</span> <span class="n">milliseconds</span><span class="p">,</span> <span class="n">QString</span> <span class="n">name</span> <span class="p">,</span> <span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="n"><span class="hl">sliderwidth</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n">bool</span> <span class="n">showPopup</span> <span class="o">=</span> <span class="nb">false</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters.</pre>
</blockquote>
<p>On March 23rd, 2012, 10:46 p.m., <b>Jasneet Bhatti</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't know of a case when the slider width changes. Does it happen while streaming live ?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I thought about the case when Amarok window is resized during playback. (plase correct me if I'm wrong)</pre>
<br />
<p>- Matěj</p>
<br />
<p>On March 23rd, 2012, 10:50 p.m., Jasneet Bhatti wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/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 Jasneet Bhatti.</div>
<p style="color: grey;"><i>Updated March 23, 2012, 10:50 p.m.</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;">This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed.</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;">Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me.</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/context/applets/similarartists/ArtistWidget.cpp <span style="color: grey">(18d8cc2)</span></li>
<li>src/context/applets/upcomingevents/UpcomingEventsApplet.cpp <span style="color: grey">(9664201)</span></li>
<li>src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp <span style="color: grey">(dc5c500)</span></li>
<li>src/amarokurls/PlayUrlGenerator.h <span style="color: grey">(131b737)</span></li>
<li>src/amarokurls/PlayUrlGenerator.cpp <span style="color: grey">(90e71ff)</span></li>
<li>src/amarokurls/ContextUrlGenerator.cpp <span style="color: grey">(16986f6)</span></li>
<li>src/amarokurls/NavigationUrlGenerator.cpp <span style="color: grey">(d1e21e2)</span></li>
<li>src/amarokurls/BookmarkModel.h <span style="color: grey">(73ae345)</span></li>
<li>src/amarokurls/BookmarkModel.cpp <span style="color: grey">(9218088)</span></li>
<li>src/amarokurls/AmarokUrl.cpp <span style="color: grey">(19ba210)</span></li>
<li>src/amarokurls/AmarokUrl.h <span style="color: grey">(6a1d67f)</span></li>
<li>src/playlist/PlaylistViewUrlGenerator.cpp <span style="color: grey">(0ffcb9f)</span></li>
<li>src/services/ServiceCapabilities.cpp <span style="color: grey">(6129f8e)</span></li>
<li>src/widgets/BookmarkTriangle.h <span style="color: grey">(46e9118)</span></li>
<li>src/widgets/BookmarkTriangle.cpp <span style="color: grey">(4c59d42)</span></li>
<li>src/widgets/SliderWidget.cpp <span style="color: grey">(5e72e13)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104307/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>