Review Request: Fix multiple issues with the lyrics applet/engine

[Martin Blumenstingl]

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



src/context/applets/lyrics/LyricsApplet.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1712>

    Duplicated code - I just used showLyrics() instead.



src/context/applets/lyrics/LyricsApplet.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1713>

    Fixed a null-ptr when pressing "ESC" when the lyrics applet was added when amarok was already started and a track was playing.



src/context/applets/lyrics/LyricsApplet.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1714>

    This should've used the LyricsData struct, but it wasn't.
    .toString() simply returned QString() (empty string).



src/context/applets/lyrics/LyricsApplet.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1715>

    This may mean a tiny overhead for non-HTML lyrics, but it's probably good to have it anyway.



src/context/applets/lyrics/LyricsApplet.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1716>

    This makes the code even more robust as before: HTML lyrics with no <title> tag simply showed no title - now they do :).



src/context/engines/lyrics/LyricsEngine.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1717>

    We should update in any case.
    The LyricsApplet and update() already check if the lyrics were really updated.



src/context/engines/lyrics/LyricsEngine.cpp
<http://git.reviewboard.kde.org/r/100892/#comment1718>

    See what the comment says, that code-block was simply misplaced.


- Martin


On March 19, 2011, 8:43 p.m., Martin Blumenstingl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100892/
> -----------------------------------------------------------
> 
> (Updated March 19, 2011, 8:43 p.m.)
> 
> 
> Review request for Amarok and Rick W. Chen.
> 
> 
> Summary
> -------
> 
> This fixes the following lyrics related issues:
> -Amarok crashed when the lyrics applet was removed and re-added while a track was playing and the user pressed "ESC".
> -If no lyrics script was running cached lyrics were not displayed (but an error message that no scripts are running instead).
> -HTML lyrics were not displayed (this is a regression).
> -If the user edited the lyrics in TagDialog the changes were not "synchronized" to the lyrics applet (this is probably also a regression).
> 
> Can we make sure this patch gets into the next release?
> 
> 
> Diffs
> -----
> 
>   src/context/applets/lyrics/LyricsApplet.cpp c3d82db 
>   src/context/engines/lyrics/LyricsEngine.h dd6beb9 
>   src/context/engines/lyrics/LyricsEngine.cpp d346511 
> 
> Diff: http://git.reviewboard.kde.org/r/100892/diff
> 
> 
> Testing
> -------
> 
> -HTML lyrics are working again
> -Amarok does not crash anymore when re-adding the lyrics applet and pressing "ESC"
> etc.
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20110319/63a8d2ec/attachment-0001.htm