Review Request 109470: A patch which allows us to distinguish between cached lyrics and lyrics newly downloaded from the internet.

[Jasneet Bhatti]

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


While we wait for the active developers to decide if this feature is going to be introduced, there are a few formatting issues to be addressed before we proceed.
As a rule, before you submit a patch, always make sure it adheres to the coding style laid out in HACKING/intro_and_style.txt
And don't let this bother you too much, almost everyone has coding style issues in their first patch.


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

    The defines and includes shouldn't be clubbed together. Let the newline remain here. 



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

    No need for a newline here.



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

    Again, no need for the newlines. Keep the coding style consistent.



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

    Leading/Trailing whitespaces to be removed.
    
    You might not be aware of their presence in the text editor/IDE you're using. So make sure the indentation is done using spaces and not tabs. If you're using Kate, you can do this by going to Settings->Configure Kate->Editor Component->Editing->Indentation and setting 'Indent using' to 'Spaces'.
    You could also use Mark's Vim configuration present in HACKING/intro_and_style.txt if you are using Vim.
    



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

    Correct the formatting issues here.
    Refer to the 'Formatting' section of HACKING/intro_and_style to correct the issues : spaces between brackets and arguments, trailing whitespaces, indentation, etc.



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

    Trailing whitespace.



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

    Unnecessary newlines.



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

    Here too.



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

    Indentation issue.



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

    Superfluous newline.



src/context/engines/lyrics/LyricsEngine.h
<http://git.reviewboard.kde.org/r/109470/#comment21850>

    Indentation issue.



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

    As mentioned previously, defines and includes are not to be clubbed together.



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

    Trailing whitespace.



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

    Spaces needed between brackets and argument : newCacheLyrics( lyrics )



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

    Superfluous newline.



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

    Indentation issue.



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

    Spaces needed between brackets and function arguments.



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

    Let the newline remain here.


- Jasneet Bhatti


On March 14, 2013, 7:10 p.m., mayank jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109470/
> -----------------------------------------------------------
> 
> (Updated March 14, 2013, 7:10 p.m.)
> 
> 
> Review request for Amarok, Samikshan Bairagya, Jasneet Bhatti, Bartosz Brachaczek, Tirtha Chatterjee, and Matěj Laitl.
> 
> 
> Description
> -------
> 
> It required modifications, when there is no change in the lyrics downloaded and lyrics retrieved from cache the title display of the lyrics browser changes to "Cached Lyrics" from "Lyrics" so we can tell the difference between old and new. 
> 
> 
> Diffs
> -----
> 
>   src/context/applets/lyrics/LyricsApplet.cpp 2394964 
>   src/context/engines/lyrics/LyricsEngine.h b187b73 
>   src/context/engines/lyrics/LyricsEngine.cpp 2befa91 
> 
> Diff: http://git.reviewboard.kde.org/r/109470/diff/
> 
> 
> Testing
> -------
> 
> Its working fine!
> 
> 
> Thanks,
> 
> mayank jha
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130314/a9153e50/attachment-0001.html>