Review Request: Add a theme selector to the "Configure Desktop" dialog

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://mattr.info/r/138/#review134
-----------------------------------------------------------


in general, i really like this patch. there are a few bits that can be touched up, but with those corners covered, please commit =)



trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.cpp
<http://mattr.info/r/138/#comment98>

    this will miss themes in the $KDEHOME or in other $KDEDIRS. what you probably want is something like (warning untested code written at 2:15am after getting up to go the washroom and not being able to resist checking my email on the way .. so probably full of little errors ;):
    
    QStringList themes = KStandardDirs::findAllResources("data", "desktoptheme/*/metadata.desktop", KStandardDirs::NoDuplicates);
    int suffixLength = 17; // length of "/metadata.desktop"
    
    foreach (const QString &theme, themes) {
        QString path = theme.left(theme.length() - suffixLength);
        int rootThemeSepIndex = path.lastIndexOf("/", -1);
        QString themeRoot = theme.left(rootThemeSepIndex);
        path = path.right(theme.length() - rootThemeSepIndex);
        m_themes << path;
    
        ... set up the package file (see next comment)
    }



trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.cpp
<http://mattr.info/r/138/#comment97>

    You could actually use Plasma::Package here. e.g.:
    
    #include "packages_p.h"
    
    QString packageRoot = StandardDirs::locate("data", "desktoptheme/");
    ThemePackageStructure tps;
    
    foreach (QString theme, m_themes) {
         Package package(packageRoot, theme, tps);
         Plasma::SvgPanel *svg = new Plasma::SvgPanel(package.filePath("widgets/background"), this)
    
    that would be much more resilient to future changes in the Theme structure.



trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.cpp
<http://mattr.info/r/138/#comment99>

    m_theme->setCurrentIndex(m_themeModel->indexOf(Plasma::Theme::self()->themeName());



trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.cpp
<http://mattr.info/r/138/#comment100>

    Plasma::Theme::setThemeName takes care of writing the setting out to the config file. you shouldn't need to do that yourself.


- Aaron


On 2008-02-15 02:43:54, Andre Duffeck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://mattr.info/r/138/
> -----------------------------------------------------------
> 
> (Updated 2008-02-15 02:43:54)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch adds a theme selector to the "Configure Desktop" dialog (until there is a better place for it).
> 
> Requires http://mattr.info/r/136/
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/plasma/containments/desktop/BackgroundDialog.ui
>   trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.h
>   trunk/KDE/kdebase/workspace/plasma/containments/desktop/backgrounddialog.cpp
> 
> Diff: http://mattr.info/r/138/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Screenshot
>   http://mattr.info/r/138/s/9/
> 
> 
> Thanks,
> 
> Andre
> 
>