Review Request 109965: Refactor assetimporters

[Aaron J. Seigo]

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


looking very good. a few comments below, but this should be ready soon, i imagine.


assetimporters/database-common/database.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22993>

    would be nicer imho to cache this value after doing a look up once in the database. so if this the "KDE" partner, do a lookup for the KDE partner and then cache the result for later re-use (we can assume it won't change in the database :)



assetimporters/database-common/database.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22994>

    awesome, sizes!
    
    very stoked to see that.



assetimporters/database-common/database.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22995>

    may as well move this after the check to line 481



assetimporters/kdeartwork-wallpapers/kdewallpapersdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22996>

    same as with books: query the db for the value and then assign it to the members (m_licenseId, m_partnerId) would be better



assetimporters/obs/packagedatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22997>

    spacing: foreach (



assetimporters/projectgutenberg/src/gutenbergdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22999>

    this is more partnerId than partnerQuery?



assetimporters/projectgutenberg/src/gutenbergdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment22998>

    this can be done once and then the value cached



assetimporters/projectgutenberg/src/gutenbergdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment23000>

    as with partnerQuery above, this is probably more accurately named languageId



assetimporters/projectgutenberg/src/gutenbergdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment23001>

    this could also possibly be cached, e.g.:
    
    int id = m_languageIds.value(lang);
    
    if (id < 1) {
        QSqlQuery query;
        .. do the query ..
        id = res.toInt();
        m_languageIds.insert(lang, id);
    }
    
    return id;



assetimporters/projectgutenberg/src/gutenbergdatabase.cpp
<http://git.reviewboard.kde.org/r/109965/#comment23002>

    same as methods above


- Aaron J. Seigo


On April 12, 2013, 6:45 p.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109965/
> -----------------------------------------------------------
> 
> (Updated April 12, 2013, 6:45 p.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/kdewallpapers-channels.ini b22f395 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130413/5ba23eaa/attachment-0001.html>