----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
