Review Request 118861: GSoC Review 1: parsing and highlighting of lyrics

[Mark Kretschmann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118861/#review60753
-----------------------------------------------------------



src/context/applets/lyrics/LrcParser.h
<https://git.reviewboard.kde.org/r/118861/#comment42341>

    The default argument 0 should be removed here. It can lead to accidental memory leaks when no object ownership is specified.



src/context/applets/lyrics/LrcParser.h
<https://git.reviewboard.kde.org/r/118861/#comment42347>

    Spelling error in API: "Postion" -> "Position".



src/context/applets/lyrics/LrcParser.h
<https://git.reviewboard.kde.org/r/118861/#comment42346>

    API issue: The lrc parameter should be a const reference. Changing the original reference is not what the user would expect here.



src/context/applets/lyrics/LrcParser.h
<https://git.reviewboard.kde.org/r/118861/#comment42348>

    There is no need to abbreviate "increment".



src/context/applets/lyrics/LrcParser.h
<https://git.reviewboard.kde.org/r/118861/#comment42349>

    There is no need to abbreviate "decrement".



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42342>

    Generally, please make more liberal use of empty lines (whitespace) to separate logical code blocks.



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42343>

    Change this into two separate declarations to avoid confusion.



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42344>

    Use QString::clear() instead.



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42345>

    Out-commented code must not remain in production code.



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42350>

    Lines should be const.



src/context/applets/lyrics/LrcParser.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42351>

    No magic numbers please. What does 60 and 100 mean? Can be declared as constants.



src/context/applets/lyrics/LyricsApplet.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42352>

    curLine should be const.



src/context/applets/lyrics/LyricsApplet.cpp
<https://git.reviewboard.kde.org/r/118861/#comment42353>

    trimmed can be made const when newLrc() takes a const reference.


- Mark Kretschmann


On June 21, 2014, 11:42 a.m., Vedant Agarwala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118861/
> -----------------------------------------------------------
> 
> (Updated June 21, 2014, 11:42 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> I have been working on my project since some time and have had to make changes as I went on. I decided to get the highlighting working first then we could get to make it more pleasing.
> 
> So right now, I am able to parse LRC files and highlight the line in the present lyrics display applet itself. The text is just "selected" as the song progresses. 2 buttons increase and decrease the offset so that one can sync the line with track. This data is saved to the database (and file, if user allows).
> 
> 
> Diffs
> -----
> 
>   src/context/applets/lyrics/CMakeLists.txt b2a86d4 
>   src/context/applets/lyrics/LrcParser.h PRE-CREATION 
>   src/context/applets/lyrics/LrcParser.cpp PRE-CREATION 
>   src/context/applets/lyrics/LyricsApplet.h 20c71d8 
>   src/context/applets/lyrics/LyricsApplet.cpp 829a166 
> 
> Diff: https://git.reviewboard.kde.org/r/118861/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> To test it, just right-click on a track ->"edit track details"->"lyrics" and paste the lyrics with timestamps i.e. the contents of a LRC file. As soon as that track starts playing, the lyrics applet will strip the timestamps, show the lyrics and highlight them.
> 
> 
> Thanks,
> 
> Vedant Agarwala
> 
>

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