Review Request: Improve lyrics applet

[Martin Blumenstingl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100013/
-----------------------------------------------------------

(Updated 2010-11-06 17:18:27.760440)


Review request for Amarok.


Changes
-------

-Coding style fixes (thanks Rick and Leo)
-Fixed the QObject::connect() warning
-Removed the ChangeLog entries


Summary
-------

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 :)


This addresses bug 207621.
    https://bugs.kde.org/show_bug.cgi?id=207621


Diffs (updated)
-----

  src/context/Applet.h 613f217 
  src/context/Applet.cpp 291e481 
  src/context/LyricsManager.h 24de425 
  src/context/LyricsManager.cpp 8dfb05e 
  src/context/applets/lyrics/LyricsApplet.h 0270311 
  src/context/applets/lyrics/LyricsApplet.cpp 2392da0 
  src/context/engines/lyrics/LyricsEngine.h bf4a702 
  src/context/engines/lyrics/LyricsEngine.cpp cbcdaa2 
  src/core-impl/collections/db/sql/SqlMeta.cpp f01ca7a 
  src/core-impl/collections/nepomukcollection/NepomukTrack.cpp 02f5bc7 
  src/dialogs/TagDialog.cpp ddd73cb 
  src/scriptengine/AmarokLyricsScript.cpp 5e7a94e 
  src/themes/context/Amarok-Mockup/colors a0b9488 

Diff: http://git.reviewboard.kde.org/r/100013/diff


Testing
-------

I've been using a patched amarok for the past two days -> I couldn't find bugs so far.
But I need more testers :)


Screenshots
-----------

The warning which is shown when the user might lose changes
  http://git.reviewboard.kde.org/r/100013/s/15/
The warning which is shown when asking the user if he wants to refetch lyrics
  http://git.reviewboard.kde.org/r/100013/s/16/


Thanks,

Martin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101106/0b75eee4/attachment.htm