From amarok-devel  Sat Nov 16 14:25:09 2013
From: "Mark Kretschmann" <kretschmann () kde ! org>
Date: Sat, 16 Nov 2013 14:25:09 +0000
To: amarok-devel
Subject: Re: Review Request 113390: GSoC 2013 Revamping Scripting - Part 6/6 : Misc src/
Message-Id: <20131116142509.17610.91763 () vidsolbach ! de>
X-MARC-Message: https://marc.info/?l=amarok-devel&m=138461192625923
MIME-Version: 1
Content-Type: multipart/mixed; boundary="--===============0538632284342917905=="

--===============0538632284342917905==
Content-Type: multipart/alternative;
 boundary="===============3709567823118327018=="

--===============3709567823118327018==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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



src/CMakeLists.txt
<http://git.reviewboard.kde.org/r/113390/#comment31457>

    So where should it be? :)



src/amarokconfig.kcfg
<http://git.reviewboard.kde.org/r/113390/#comment31461>

    The user probably does not want to see meaningless errors about some script they installed. This is a debugging feature for developers, right?



src/configdialog/dialogs/ScriptsConfig.cpp
<http://git.reviewboard.kde.org/r/113390/#comment31459>

    Don't use exclamation marks in user-visible strings.
    
    Also I'm wondering why an error dialog is shown here. Maybe the button should simply be disabled if nothing is selected. Then there would be no need for error handling.



src/configdialog/dialogs/ScriptsConfig.cpp
<http://git.reviewboard.kde.org/r/113390/#comment31460>

    Please reword the string. It's pretty unclear.
    
    E.g. "Default scripts that are bundled with Amarok cannot be uninstalled."



src/configdialog/dialogs/ScriptsConfig.cpp
<http://git.reviewboard.kde.org/r/113390/#comment31458>

    Can be changed to:
    
    return findSpecFile( subdir );
    


- Mark Kretschmann


On Oct. 22, 2013, 4:06 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113390/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 4:06 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> Miscellaneous changes made in the src/ folder.
> 
> 
> Diffs
> -----
> 
>   src/App.h 9ee071d 
>   src/App.cpp eb3f483 
>   src/CMakeLists.txt 70fb67b 
>   src/EngineController.h 80ec17b 
>   src/EngineController.cpp cf29cf8 
>   src/amarokconfig.kcfg 84cfa5b 
>   src/amarokurls/BookmarkGroup.h ea42c30 
>   src/browsers/CollectionTreeItem.h 0a5d197 
>   src/browsers/CollectionTreeItem.cpp 4c35523 
>   src/browsers/CollectionTreeItemModelBase.h 00e99fb 
>   src/browsers/CollectionTreeView.h 454a70d 
>   src/browsers/CollectionTreeView.cpp 6fad164 
>   src/browsers/collectionbrowser/CollectionWidget.h 7480015 
>   src/browsers/collectionbrowser/CollectionWidget.cpp 95a16dc 
>   src/configdialog/dialogs/ScriptSelector.h 9ce1168 
>   src/configdialog/dialogs/ScriptSelector.cpp 117cedf 
>   src/configdialog/dialogs/ScriptsConfig.h 6a47847 
>   src/configdialog/dialogs/ScriptsConfig.cpp 714b63e 
>   src/configdialog/dialogs/ScriptsConfig.ui b4b8d37 
>   src/context/applets/lyrics/LyricsApplet.cpp 7db356d 
>   src/context/applets/songkick/SongkickApplet.cpp ed93e56 
>   src/context/engines/lyrics/LyricsEngine.cpp d3273b0 
>   src/core-impl/collections/support/jobs/WriteTagsJob.h a92ab8d 
>   src/core-impl/collections/support/jobs/WriteTagsJob.cpp 5834cd4 
>   src/core/collections/Collection.h 94d24d7 
>   src/core/collections/QueryMaker.h 92b6a65 
>   src/core/meta/support/MetaConstants.cpp 40d7122 
>   src/dialogs/DiagnosticDialog.cpp c9bfce6 
>   src/dialogs/EqualizerDialog.h 6c9e19e 
>   src/dialogs/EqualizerDialog.cpp acf0da0 
>   src/dialogs/deviceconfiguredialog.cpp e54edad 
>   src/dynamic/BiasFactory.cpp 2a887c2 
>   src/dynamic/TrackSet.h dee8ee3 
>   src/dynamic/TrackSet.cpp 456a10a 
>   src/equalizer/EqualizerPresets.h be8d267 
>   src/equalizer/EqualizerPresets.cpp b9aa13b 
>   src/playback/EqualizerController.h PRE-CREATION 
>   src/playback/EqualizerController.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h a835c35 
>   src/services/scriptable/ScriptableService.cpp f254e3e 
>   src/services/scriptable/ScriptableServiceInfoParser.cpp 998135b 
>   src/services/scriptable/ScriptableServiceManager.cpp aa28a6c 
>   src/services/scriptable/ScriptableServiceQueryMaker.cpp 07233dd 
> 
> Diff: http://git.reviewboard.kde.org/r/113390/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>


--===============3709567823118327018==
Content-Type: text/html; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit




<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="http://git.reviewboard.kde.org/r/113390/">http://git.reviewboard.kde.org/r/113390/</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="http://git.reviewboard.kde.org/r/113390/diff/1/?file=204537#file204537line484" style="color: black; font-weight: bold; text-decoration: underline;">src/CMakeLists.txt</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">484</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c">#shouldn&#39;t be here</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;">So where should it be? :)</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="http://git.reviewboard.kde.org/r/113390/diff/1/?file=204540#file204540line596" style="color: black; font-weight: bold; text-decoration: underline;">src/amarokconfig.kcfg</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">596</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    &lt;entry key=&quot;EnableDeprecationWarnings&quot; type=&quot;Bool&quot;&gt;</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 user probably does not want to see meaningless errors about some script they installed. This is a debugging feature for developers, right?</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="http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line255" style="color: black; font-weight: bold; text-decoration: underline;">src/configdialog/dialogs/ScriptsConfig.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">244</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">        <span class="n">KMessageBox</span><span class="o">::</span><span class="n">error</span><span class="p">(</span> <span class="k">this</span><span class="p">,</span> <span class="n">i18n</span><span class="p">(</span> <span class="s">&quot;Please select a script!&quot;</span> <span class="p">)</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;">Don&#39;t use exclamation marks in user-visible strings.

Also I&#39;m wondering why an error dialog is shown here. Maybe the button should simply be disabled if nothing is selected. Then there would be no need for error handling.</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="http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line260" style="color: black; font-weight: bold; text-decoration: underline;">src/configdialog/dialogs/ScriptsConfig.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">249</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">response</span> <span class="o">=</span> <span class="n">KMessageBox</span><span class="o">::</span><span class="n">warningContinueCancel</span><span class="p">(</span> <span class="k">this</span><span class="p">,</span> <span class="n">i18n</span><span class="p">(</span> <span class="s">&quot;You are advised to only uninstall manually &quot;</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;">Please reword the string. It&#39;s pretty unclear.

E.g. &quot;Default scripts that are bundled with Amarok cannot be uninstalled.&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="http://git.reviewboard.kde.org/r/113390/diff/1/?file=204552#file204552line289" style="color: black; font-weight: bold; text-decoration: underline;">src/configdialog/dialogs/ScriptsConfig.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">278</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                    <span class="k">const</span> <span class="n">KArchiveFile</span> <span class="o">*</span><span class="n">file</span> <span class="o">=</span> <span class="n">findSpecFile</span><span class="p">(</span> <span class="n">subDir</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;">Can be changed to:

return findSpecFile( subdir );
</pre>
</div>
<br />



<p>- Mark Kretschmann</p>


<br />
<p>On October 22nd, 2013, 4:06 p.m. UTC, Anmol Ahuja wrote:</p>








<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Anmol Ahuja.</div>


<p style="color: grey;"><i>Updated Oct. 22, 2013, 4:06 p.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;">Miscellaneous changes made in the src/ folder.</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/App.h <span style="color: grey">(9ee071d)</span></li>

 <li>src/App.cpp <span style="color: grey">(eb3f483)</span></li>

 <li>src/CMakeLists.txt <span style="color: grey">(70fb67b)</span></li>

 <li>src/EngineController.h <span style="color: grey">(80ec17b)</span></li>

 <li>src/EngineController.cpp <span style="color: grey">(cf29cf8)</span></li>

 <li>src/amarokconfig.kcfg <span style="color: grey">(84cfa5b)</span></li>

 <li>src/amarokurls/BookmarkGroup.h <span style="color: grey">(ea42c30)</span></li>

 <li>src/browsers/CollectionTreeItem.h <span style="color: grey">(0a5d197)</span></li>

 <li>src/browsers/CollectionTreeItem.cpp <span style="color: grey">(4c35523)</span></li>

 <li>src/browsers/CollectionTreeItemModelBase.h <span style="color: grey">(00e99fb)</span></li>

 <li>src/browsers/CollectionTreeView.h <span style="color: grey">(454a70d)</span></li>

 <li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(6fad164)</span></li>

 <li>src/browsers/collectionbrowser/CollectionWidget.h <span style="color: grey">(7480015)</span></li>

 <li>src/browsers/collectionbrowser/CollectionWidget.cpp <span style="color: grey">(95a16dc)</span></li>

 <li>src/configdialog/dialogs/ScriptSelector.h <span style="color: grey">(9ce1168)</span></li>

 <li>src/configdialog/dialogs/ScriptSelector.cpp <span style="color: grey">(117cedf)</span></li>

 <li>src/configdialog/dialogs/ScriptsConfig.h <span style="color: grey">(6a47847)</span></li>

 <li>src/configdialog/dialogs/ScriptsConfig.cpp <span style="color: grey">(714b63e)</span></li>

 <li>src/configdialog/dialogs/ScriptsConfig.ui <span style="color: grey">(b4b8d37)</span></li>

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

 <li>src/context/applets/songkick/SongkickApplet.cpp <span style="color: grey">(ed93e56)</span></li>

 <li>src/context/engines/lyrics/LyricsEngine.cpp <span style="color: grey">(d3273b0)</span></li>

 <li>src/core-impl/collections/support/jobs/WriteTagsJob.h <span style="color: grey">(a92ab8d)</span></li>

 <li>src/core-impl/collections/support/jobs/WriteTagsJob.cpp <span style="color: grey">(5834cd4)</span></li>

 <li>src/core/collections/Collection.h <span style="color: grey">(94d24d7)</span></li>

 <li>src/core/collections/QueryMaker.h <span style="color: grey">(92b6a65)</span></li>

 <li>src/core/meta/support/MetaConstants.cpp <span style="color: grey">(40d7122)</span></li>

 <li>src/dialogs/DiagnosticDialog.cpp <span style="color: grey">(c9bfce6)</span></li>

 <li>src/dialogs/EqualizerDialog.h <span style="color: grey">(6c9e19e)</span></li>

 <li>src/dialogs/EqualizerDialog.cpp <span style="color: grey">(acf0da0)</span></li>

 <li>src/dialogs/deviceconfiguredialog.cpp <span style="color: grey">(e54edad)</span></li>

 <li>src/dynamic/BiasFactory.cpp <span style="color: grey">(2a887c2)</span></li>

 <li>src/dynamic/TrackSet.h <span style="color: grey">(dee8ee3)</span></li>

 <li>src/dynamic/TrackSet.cpp <span style="color: grey">(456a10a)</span></li>

 <li>src/equalizer/EqualizerPresets.h <span style="color: grey">(be8d267)</span></li>

 <li>src/equalizer/EqualizerPresets.cpp <span style="color: grey">(b9aa13b)</span></li>

 <li>src/playback/EqualizerController.h <span style="color: grey">(PRE-CREATION)</span></li>

 <li>src/playback/EqualizerController.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>src/playlistmanager/PlaylistManager.h <span style="color: grey">(a835c35)</span></li>

 <li>src/services/scriptable/ScriptableService.cpp <span style="color: grey">(f254e3e)</span></li>

 <li>src/services/scriptable/ScriptableServiceInfoParser.cpp <span style="color: grey">(998135b)</span></li>

 <li>src/services/scriptable/ScriptableServiceManager.cpp <span style="color: grey">(aa28a6c)</span></li>

 <li>src/services/scriptable/ScriptableServiceQueryMaker.cpp <span style="color: grey">(07233dd)</span></li>

</ul>

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







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








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


--===============3709567823118327018==--

--===============0538632284342917905==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--===============0538632284342917905==--