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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       amarok-devel
Subject:    Re: Review Request 118861: GSoC Review 1: parsing and highlighting of lyrics
From:       "Mark Kretschmann" <kretschmann () kde ! org>
Date:       2014-06-23 8:46:10
Message-ID: 20140623084610.6826.31994 () probe ! kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

-----------------------------------------------------------
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
> 
> 

[Attachment #5 (text/html)]

<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;">  <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="https://git.reviewboard.kde.org/r/118861/">https://git.reviewboard.kde.org/r/118861/</a>
  </td>
    </tr>
   </table>
   <br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282834#file282834line12" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.h</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">12</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">explicit</span> <span class="n">LrcParser</span><span \
class="p">(</span><span class="n">QObject</span> <span class="o">*</span><span \
class="n">parent</span> <span class="o">=</span> <span class="mi">0</span><span \
class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The default \
argument 0 should be removed here. It can lead to accidental memory leaks when no \
object ownership is specified.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282834#file282834line18" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.h</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">18</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="kt">int</span> <span class="n">lineForTrackPostion</span><span \
class="p">(</span> <span class="kt">int</span> <span class="n">trackPos</span> <span \
class="p">)</span> <span class="k">const</span><span class="p">;</span></pre></td>  \
</tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Spelling \
error in API: &quot;Postion&quot; -&gt; &quot;Position&quot;.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282834#file282834line28" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.h</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">28</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="kt">void</span> <span class="n">newLrc</span><span class="p">(</span> <span \
class="n">QString</span> <span class="o">&amp;</span><span class="n">lrc</span> <span \
class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">API issue: \
The lrc parameter should be a const reference. Changing the original reference is not \
what the user would expect here.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282834#file282834line29" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.h</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">29</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="kt">void</span> <span class="nf">incOffset</span><span class="p">(</span> \
<span class="kt">int</span> <span class="n">value</span> <span class="o">=</span> \
<span class="mi">50</span> <span class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There is no \
need to abbreviate &quot;increment&quot;.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282834#file282834line30" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.h</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">30</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="kt">void</span> <span class="nf">decOffset</span><span class="p">(</span> \
<span class="kt">int</span> <span class="n">value</span> <span class="o">=</span> \
<span class="mi">50</span> <span class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There is no \
need to abbreviate &quot;decrement&quot;.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line44" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">44</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">DEBUG_BLOCK</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Generally, \
please make more liberal use of empty lines (whitespace) to separate logical code \
blocks.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line47" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">47</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">QString</span> <span class="n">lyrics</span> <span class="o">=</span> <span \
class="n">track</span><span class="o">-&gt;</span><span \
class="n">cachedLyrics</span><span class="p">(),</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Change this \
into two separate declarations to avoid confusion.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line94" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">94</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">m_lyrics</span> <span class="o">=</span> <span \
class="s">&quot;&quot;</span><span class="p">;</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Use \
QString::clear() instead.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line100" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">100</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="c1">//        lrc.remove( 0, posOfOffset ); // remove all lines till the line \
containing the offset tag</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Out-commented code must not remain in production code.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line113" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">113</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">QStringList</span> <span class="n">lines</span> <span class="o">=</span> \
<span class="n">lrc</span><span class="p">.</span><span class="n">split</span><span \
class="p">(</span> <span class="s">&quot;</span><span class="se">\n</span><span \
class="s">&quot;</span><span class="p">,</span> <span class="n">QString</span><span \
class="o">::</span><span class="n">SkipEmptyParts</span> <span \
class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Lines \
should be const.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282835#file282835line124" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LrcParser.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">124</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">            \
<span class="kt">int</span> <span class="n">timestamp</span> <span class="o">=</span> \
<span class="p">(</span> <span class="n">capTexts</span><span class="p">[</span><span \
class="mi">2</span><span class="p">].</span><span class="n">toInt</span><span \
class="p">()</span> <span class="o">*</span> <span class="mi">60</span> <span \
class="o">+</span> <span class="n">capTexts</span><span class="p">[</span><span \
class="mi">4</span><span class="p">].</span><span class="n">toDouble</span><span \
class="p">()</span> <span class="p">)</span> <span class="o">*</span> <span \
class="mi">100</span><span class="p">;</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">No magic \
numbers please. What does 60 and 100 mean? Can be declared as constants.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282837#file282837line444" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LyricsApplet.cpp</a>  <span \
style="font-weight: normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
  <tr>

   <td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">LyricsAppletPrivate::_trackDataChanged( Meta::TrackPtr track )</pre></td>

  </tr>
 </tbody>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">443</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">        <span \
class="kt">int</span> <span class="n">curLine</span> <span class="o">=</span> <span \
class="n">lrcParser</span><span class="o">-&gt;</span><span \
class="n">lineForTrackPostion</span><span class="p">(</span> <span \
class="n">engine</span><span class="o">-&gt;</span><span \
class="n">trackPositionMs</span><span class="p">()</span> <span class="o">/</span> \
<span class="mi">10</span> <span class="p">);</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">curLine \
should be const.</pre> </div>
<br />

<div>

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/118861/diff/1/?file=282837#file282837line653" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/context/applets/lyrics/LyricsApplet.cpp</a>  <span \
style="font-weight: normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
  <tr>

   <td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">LyricsApplet::dataUpdated( const QString&amp; name, const \
Plasma::DataEngine::Data&amp; data )</pre></td>

  </tr>
 </tbody>

 <tbody>

  <tr>
    <th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">598</font></th>  <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; ">            <span \
class="n">QString</span> <span class="n">trimmed</span> <span class="o">=</span> \
<span class="n">lyrics</span><span class="p">.</span><span class="n">text</span><span \
class="p">.</span><span class="n">trimmed</span><span class="p">();</span></pre></td> \
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">652</font></th>  <td bgcolor="#ffffff" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">            \
<span class="n">QString</span> <span class="n">trimmed</span> <span \
class="o">=</span> <span class="n">lyrics</span><span class="p">.</span><span \
class="n">text</span><span class="p">.</span><span class="n">trimmed</span><span \
class="p">();</span></pre></td>  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">trimmed can \
be made const when newLrc() takes a const reference.</pre> </div>
<br />

<p>- Mark Kretschmann</p>

<br />
<p>On June 21st, 2014, 11:42 a.m. UTC, Vedant Agarwala wrote:</p>

<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;">  <tr>
  <td>

<div>Review request for Amarok.</div>
<div>By Vedant Agarwala.</div>

<p style="color: grey;"><i>Updated June 21, 2014, 11:42 a.m.</i></p>

<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
amarok
</div>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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 &quot;selected&quot; 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). </pre>
  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Yes.

To test it, just right-click on a track -&gt;&quot;edit track \
details&quot;-&gt;&quot;lyrics&quot; 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.</pre>  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>src/context/applets/lyrics/CMakeLists.txt <span style="color: \
grey">(b2a86d4)</span></li>

 <li>src/context/applets/lyrics/LrcParser.h <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>src/context/applets/lyrics/LrcParser.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>src/context/applets/lyrics/LyricsApplet.h <span style="color: \
grey">(20c71d8)</span></li>

 <li>src/context/applets/lyrics/LyricsApplet.cpp <span style="color: \
grey">(829a166)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/118861/diff/" style="margin-left: \
3em;">View Diff</a></p>

  </td>
 </tr>
</table>

  </div>
 </body>
</html>

_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

[prev in list] [next in list] [prev in thread] [next in thread] 