<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/100013/">http://git.reviewboard.kde.org/r/100013/</a>
</td>
</tr>
</table>
<br />
<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 Martin Blumenstingl.</div>
<p style="color: grey;"><i>Updated 2010-11-04 18:37:00.435783</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">added screenshots</pre>
</td>
</tr>
</table>
<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 fixes multiple issues with the lyrics applet.
I also cleaned up some of the lyrics related code.
Without my patch it's possible that the user loses some of his changes while he's editing the lyrics of the current track in the lyrics applet.
My patch fixes this issue. Now the applet simply asks the user if changes should be saved or not.
Now that the user wouldn't simply loose all changes he had made I started working on another task:
synchronizing the lyrics in the lyrics applet with the one from Meta::Track::cachedLyrics().
There was simply a call to notifyObservers() missing in the implementation of Meta::Track::setCachedLyrics().
(Plus some code in the LyricsEngine which checks if the lyrics have changed.)
I'm not sure if calling notifyObservers() is nice here, as for example the OSD shows again (just as if the song had changed).
Now if the user changes the lyrics via TagDialog the changes are synchronized with the lyrics applet.
If the user is editing the track in both, TagDialog and the lyrics applet the applet asks the user what to do with the changes.
When implementing the confirmation dialogs I found out that using KMessageBox is bad.
The reason: either you have to block the whole main window (so the user can't click stuff there anymore), or you still allow access to everything.
In my opinion this was bad, as I only wanted the user's attention in one applet.
Thus I decided to use Plasma::Applet::showMessage()
When porting all KMessageBoxes (there was only one) to Plasma::Applet::showMessage I found out that Amarok's plasma theme is broken.
I added a minimal fix - maybe more work is needed to make it look nice with all themes (but I'm not a UI guy, thus I'm bad at such things).
My changes also make it easier to fix other bugs.
For example #228766: one could now use a script which simply tells the lyrics script to re-fetch the lyrics (in case the current ones are broken).
=> I already have a proof-of-concept script here :)</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;">I've been using a patched amarok for the past two days -> I couldn't find bugs so far.
But I need more testers :)</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=207621">207621</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ChangeLog <span style="color: grey">(28daaca)</span></li>
<li>src/context/Applet.h <span style="color: grey">(613f217)</span></li>
<li>src/context/Applet.cpp <span style="color: grey">(291e481)</span></li>
<li>src/context/LyricsManager.h <span style="color: grey">(24de425)</span></li>
<li>src/context/LyricsManager.cpp <span style="color: grey">(8dfb05e)</span></li>
<li>src/context/applets/lyrics/LyricsApplet.h <span style="color: grey">(0270311)</span></li>
<li>src/context/applets/lyrics/LyricsApplet.cpp <span style="color: grey">(2392da0)</span></li>
<li>src/context/engines/lyrics/LyricsEngine.h <span style="color: grey">(bf4a702)</span></li>
<li>src/context/engines/lyrics/LyricsEngine.cpp <span style="color: grey">(cbcdaa2)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(3943a1f)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukTrack.cpp <span style="color: grey">(02f5bc7)</span></li>
<li>src/dialogs/TagDialog.cpp <span style="color: grey">(92e1947)</span></li>
<li>src/scriptengine/AmarokLyricsScript.cpp <span style="color: grey">(5e7a94e)</span></li>
<li>src/themes/context/Amarok-Mockup/colors <span style="color: grey">(a0b9488)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100013/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/100013/s/15/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2010/11/04/changes-lost-warning_400x100.png" style="border: 1px black solid;" alt="The warning which is shown when the user might lose changes" /></a>
<a href="http://git.reviewboard.kde.org/r/100013/s/16/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2010/11/04/refetch-warning_400x100.png" style="border: 1px black solid;" alt="The warning which is shown when asking the user if he wants to refetch lyrics" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>