----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100013/#review319 -----------------------------------------------------------
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 g...@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. - Rick W. On 2010-11-06 17:18:27, Martin Blumenstingl wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100013/ > ----------------------------------------------------------- > > (Updated 2010-11-06 17:18:27) > > > Review request for Amarok. > > > 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 > ----- > > 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 Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
