> On 2010-11-05 07:41:17, Rick W. Chen wrote: > > ChangeLog, lines 8-9 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3201#file3201line8> > > > > Please don't put changelog diffs in the review request. the patch will > > almost certainly fail to apply if ChangeLog has been changed since then.
Fixed in my latest patch. > On 2010-11-05 07:41:17, Rick W. Chen wrote: > > src/context/LyricsManager.cpp, line 287 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3205#file3205line287> > > > > From intro_and_style.txt: > > > > * For pointer and reference variable declarations put a space between > > the type > > and the * or & and no space before the variable name. > > Thanks for reminding me - fixed. > On 2010-11-05 07:41:17, Rick W. Chen wrote: > > src/context/applets/lyrics/LyricsApplet.cpp, lines 285-294 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3207#file3207line285> > > > > Some braces here would be nice Makes sense -> done :) > On 2010-11-05 07:41:17, Rick W. Chen wrote: > > src/context/applets/lyrics/LyricsApplet.cpp, line 297 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3207#file3207line297> > > > > Object::connect: No such slot > > LyricsApplet::lyricsChangedMessageButtonPressed( const MessageButton ) Oops - should be fixed now > On 2010-11-05 07:41:17, Rick W. Chen wrote: > > src/context/applets/lyrics/LyricsApplet.cpp, line 436 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3207#file3207line436> > > > > no space after if Done. > On 2010-11-05 07:41:17, Rick W. Chen wrote: > > src/themes/context/Amarok-Mockup/colors, lines 53-67 > > <http://git.reviewboard.kde.org/r/100013/diff/2/?file=3214#file3214line53> > > > > These changed the colours for some of the strings the the context view. > > e.g. text scrolling widget. It became hard to read because I have a light > > background. Are you able to change the brush for the showMessage directly > > without changing other colours? > > Leo Franchi wrote: > I have the same issue: http://i.imgur.com/LZ4Up.jpg I'm not sure yet how to fix this. It looks like it's not possible to set a custom brush for showMessage(). - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100013/#review296 ----------------------------------------------------------- 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
