Review Request: load the images for the themes correctly

[Aaron J. Seigo]

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



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16609>

    this should be a member of the class and set up in the constructor. data() gets called often, and creating it over and over here is wasteful



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16607>

    much faster if written as:
    
    } else if (themeDialogOptions.contains(key)) {
        return themeDialogOptions.value(key);
    }



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16608>

    this would also be better with a simple check if key starts with [plasmate]/themeImageDialog/. it could even be combined with the line below:
    
    const char *imagePrefix = "[plasmate]/themeImageDialog/";
    if (!qstrncmp(key, "images") || !qstrncmp(key, imagePrefix, qstrlen(imagePrefix))) {
    
    



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16612>

    minor tip: tmpPaths can be const



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16611>

    foreach ( <-- missing space :)



plasmate/packagemodel.cpp
<http://git.reviewboard.kde.org/r/106680/#comment16610>

    this looks wrong; in the Package definition there is an entry for "colors". even if it is not created by default it should still show up in the model. the whole point of this model, after all, is to have a generic way to represent a Plasma::Package.
    
    same with the checks for Plasma/Theme in the data() method. none of these should be necessary.


- Aaron J. Seigo


On Nov. 2, 2012, 8:46 a.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106680/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2012, 8:46 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> create a new theme package->click on the new
> 
> a file dialog should appear but instead a simple edit box appears requesting a new filename.
> 
> This patch solves the issue
> 
> 
> Diffs
> -----
> 
>   plasmate/editors/editpage.h 5cb3ea6 
>   plasmate/editors/editpage.cpp 7e82ff2 
>   plasmate/packagemodel.cpp 9eb0914 
> 
> Diff: http://git.reviewboard.kde.org/r/106680/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

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