-----------------------------------------------------------
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
_______________________________________________
Amarok-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/amarok-devel
