[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 109965: Refactor assetimporters
From: "Aaron J. Seigo" <aseigo () kde ! org>
Date: 2013-04-25 13:56:43
Message-ID: 20130425135643.1659.87051 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On April 13, 2013, 2:12 a.m., Aaron J. Seigo wrote:
> > assetimporters/projectgutenberg/src/gutenbergdatabase.cpp, line 326
> > <http://git.reviewboard.kde.org/r/109965/diff/1/?file=138225#file138225line326>
> >
> > this is more partnerId than partnerQuery?
>
> Giorgos Tsiapaliokas wrote:
> yes you can call it a partnerId, but I didn't. Because partnerQuery is a virtual method of \
> databasecommon and I wanted to emphasize the fact that *this* partner has changed.
>
> So I believe we should leave the method as partnerQuery or to rename all of them into partnerId.
> We have to emphasize the change in the value(if it changes at all), no?
What is important is not the implementation of the method, but what it takes and what it returns. This \
method takes nothing (languageQuery below takes a language by name) and it returns the id of the partner \
(languageQuery returns the id of the language). It's usually better to name the method after what the \
method signature says rather than the implementation.
- Aaron J.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109965/#review30976
-----------------------------------------------------------
On April 20, 2013, 10:52 a.m., Giorgos Tsiapaliokas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109965/
> -----------------------------------------------------------
>
> (Updated April 20, 2013, 10:52 a.m.)
>
>
> Review request for Plasma.
>
>
> Description
> -------
>
> This patch
>
> * removes the duplicated code in assetimporters
> * adds asset's size into the db
> * and fixes a few small issues
>
>
> Diffs
> -----
>
> assetimporters/CMakeLists.txt 24e76a0
> assetimporters/database-common/channelscatalog.h 5d39c02
> assetimporters/database-common/channelscatalog.cpp 6ca0aef
> assetimporters/database-common/database.h 9883216
> assetimporters/database-common/database.cpp e860bdd
> assetimporters/kdeartwork-wallpapers/CMakeLists.txt 56d19b9
> assetimporters/kdeartwork-wallpapers/database.h 6991758
> assetimporters/kdeartwork-wallpapers/database.cpp d75cdda
> assetimporters/kdeartwork-wallpapers/kdewallpapersdatabase.h PRE-CREATION
> assetimporters/kdeartwork-wallpapers/kdewallpapersdatabase.cpp PRE-CREATION
> assetimporters/kdeartwork-wallpapers/main.cpp 708a949
> assetimporters/obs/CMakeLists.txt 2dbcd42
> assetimporters/obs/channelscatalog.h PRE-CREATION
> assetimporters/obs/channelscatalog.cpp PRE-CREATION
> assetimporters/obs/packagedatabase.h 99f4e17
> assetimporters/obs/packagedatabase.cpp ae43b8e
> assetimporters/projectgutenberg/CMakeLists.txt b86cc49
> assetimporters/projectgutenberg/src/CMakeLists.txt 2d48e9c
> assetimporters/projectgutenberg/src/database.h 8dba0ba
> assetimporters/projectgutenberg/src/database.cpp 75cba69
> assetimporters/projectgutenberg/src/gutenbergdatabase.h PRE-CREATION
> assetimporters/projectgutenberg/src/gutenbergdatabase.cpp PRE-CREATION
> assetimporters/projectgutenberg/src/main.cpp 46f2340
> sql/bodega.sql 44f8641
>
> Diff: http://git.reviewboard.kde.org/r/109965/diff/
>
>
> Testing
> -------
>
> I haven't noticed regression.
>
>
> Thanks,
>
> Giorgos Tsiapaliokas
>
>
[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="http://git.reviewboard.kde.org/r/109965/">http://git.reviewboard.kde.org/r/109965/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 13th, 2013, 2:12 a.m. UTC, <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/109965/diff/1/?file=138225#file138225line326" style="color: black; \
font-weight: bold; text-decoration: \
underline;">assetimporters/projectgutenberg/src/gutenbergdatabase.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">326</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">GutenbergDatabase</span><span class="o">::</span><span class="n">partnerQuery</span><span \
class="p">()</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">this is more partnerId than partnerQuery?</pre> </blockquote>
<p>On April 20th, 2013, 10:51 a.m. UTC, <b>Giorgos Tsiapaliokas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">yes you can call it a partnerId, but I didn't. Because \
partnerQuery is a virtual method of databasecommon and I wanted to emphasize the fact that *this* partner \
has changed.
So I believe we should leave the method as partnerQuery or to rename all of them into partnerId.
We have to emphasize the change in the value(if it changes at all), no?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; \
white-space: -o-pre-wrap; word-wrap: break-word;">What is important is not the implementation of the \
method, but what it takes and what it returns. This method takes nothing (languageQuery below takes a \
language by name) and it returns the id of the partner (languageQuery returns the id of the language). \
It's usually better to name the method after what the method signature says rather than the \
implementation.</pre> <br />
<p>- Aaron J.</p>
<br />
<p>On April 20th, 2013, 10:52 a.m. UTC, Giorgos Tsiapaliokas 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 Plasma.</div>
<div>By Giorgos Tsiapaliokas.</div>
<p style="color: grey;"><i>Updated April 20, 2013, 10:52 a.m.</i></p>
<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;">This patch
* removes the duplicated code in assetimporters
* adds asset's size into the db
* and fixes a few small issues</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;">I haven't noticed regression.</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>assetimporters/CMakeLists.txt <span style="color: grey">(24e76a0)</span></li>
<li>assetimporters/database-common/channelscatalog.h <span style="color: grey">(5d39c02)</span></li>
<li>assetimporters/database-common/channelscatalog.cpp <span style="color: grey">(6ca0aef)</span></li>
<li>assetimporters/database-common/database.h <span style="color: grey">(9883216)</span></li>
<li>assetimporters/database-common/database.cpp <span style="color: grey">(e860bdd)</span></li>
<li>assetimporters/kdeartwork-wallpapers/CMakeLists.txt <span style="color: grey">(56d19b9)</span></li>
<li>assetimporters/kdeartwork-wallpapers/database.h <span style="color: grey">(6991758)</span></li>
<li>assetimporters/kdeartwork-wallpapers/database.cpp <span style="color: grey">(d75cdda)</span></li>
<li>assetimporters/kdeartwork-wallpapers/kdewallpapersdatabase.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>assetimporters/kdeartwork-wallpapers/kdewallpapersdatabase.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>assetimporters/kdeartwork-wallpapers/main.cpp <span style="color: grey">(708a949)</span></li>
<li>assetimporters/obs/CMakeLists.txt <span style="color: grey">(2dbcd42)</span></li>
<li>assetimporters/obs/channelscatalog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>assetimporters/obs/channelscatalog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>assetimporters/obs/packagedatabase.h <span style="color: grey">(99f4e17)</span></li>
<li>assetimporters/obs/packagedatabase.cpp <span style="color: grey">(ae43b8e)</span></li>
<li>assetimporters/projectgutenberg/CMakeLists.txt <span style="color: grey">(b86cc49)</span></li>
<li>assetimporters/projectgutenberg/src/CMakeLists.txt <span style="color: grey">(2d48e9c)</span></li>
<li>assetimporters/projectgutenberg/src/database.h <span style="color: grey">(8dba0ba)</span></li>
<li>assetimporters/projectgutenberg/src/database.cpp <span style="color: grey">(75cba69)</span></li>
<li>assetimporters/projectgutenberg/src/gutenbergdatabase.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>assetimporters/projectgutenberg/src/gutenbergdatabase.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>assetimporters/projectgutenberg/src/main.cpp <span style="color: grey">(46f2340)</span></li>
<li>sql/bodega.sql <span style="color: grey">(44f8641)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109965/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic