<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 7th, 2010, 4:48 a.m., <b>Rick W. Chen</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'm able to resolve the text colour problem. For the labels that were affected, plasma theme's text colour were used. Removing those let the labels use app colours.
I made a patch ontop of yours at git@git.kde.org:clones/amarok/rickc/amarok branch rr/improve-lyrics-applet-v3. Please try it out and see if there are any other problems. Thanks.</pre>
</blockquote>
<p>On November 7th, 2010, 2:11 p.m., <b>Martin Blumenstingl</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;">There's one issue remaining: sometimes the text from showMessage() is misaligned.
http://www.abload.de/img/text-misplaced95wb.png
Unfortunately I have no idea yet how to fix it.
It may be a Plasma/Qt bug.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Bad news: it looks like this is a Qt bug.
<notmart> somebody told me that could be fixed in qt master, dunno if already in 4.7 branch
<notmart> actually i have seen this happening on every qgraphicslayout type</pre>
<br />
<p>- Martin</p>
<br />
<p>On November 6th, 2010, 5:18 p.m., Martin Blumenstingl 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 Martin Blumenstingl.</div>
<p style="color: grey;"><i>Updated 2010-11-06 17:18:27</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 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>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">(f01ca7a)</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">(ddd73cb)</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>