Review Request 127979: [WIP] Implement color scheme selection from these installed in the system using KColorSchemeManager

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127979/#review97276
-----------------------------------------------------------




shell/mainwindow.cpp (line 478)
<https://git.reviewboard.kde.org/r/127979/#comment65668>

    QStringLiteral instead of QLatin1String



shell/thememanager.h (line 36)
<https://git.reviewboard.kde.org/r/127979/#comment65670>

    please add a space before the {



shell/thememanager.h (line 38)
<https://git.reviewboard.kde.org/r/127979/#comment65669>

    please don't indent inside a namespace



shell/thememanager.h (line 43)
<https://git.reviewboard.kde.org/r/127979/#comment65672>

    remove the empty lines after public: etc



shell/thememanager.h (line 45)
<https://git.reviewboard.kde.org/r/127979/#comment65671>

    does this really need to be a singleton? the mainwindow could simply instantiate the theme manager and be done with it. less code here and one singleton less



shell/thememanager.h (line 47)
<https://git.reviewboard.kde.org/r/127979/#comment65673>

    please don't try to align function names, we don't do that anywhere so don't do it here please.
    
    also, do you really want to make the pointer const, or should the window be const?



shell/thememanager.h (line 68)
<https://git.reviewboard.kde.org/r/127979/#comment65675>

    can be deleted once you don't put this into a singleton



shell/thememanager.h (line 70)
<https://git.reviewboard.kde.org/r/127979/#comment65674>

    no need to pimpl this class, it's not exported



shell/thememanager.cpp (line 48)
<https://git.reviewboard.kde.org/r/127979/#comment65676>

    again, most of this can, and should, be removed - simply don't make it a singleton and also don't pimpl it



shell/thememanager.cpp (line 111)
<https://git.reviewboard.kde.org/r/127979/#comment65680>

    join with next line please



shell/thememanager.cpp (line 124)
<https://git.reviewboard.kde.org/r/127979/#comment65679>

    I'd put all of this into the constructor of the ThemeManager, once it isn't a singleton anymore



shell/thememanager.cpp (line 126)
<https://git.reviewboard.kde.org/r/127979/#comment65677>

    const auto theme = currentThemeName()



shell/thememanager.cpp (line 132)
<https://git.reviewboard.kde.org/r/127979/#comment65681>

    introduce a temporary for the icon to improve readability and reduce the length of this line:
    
        auto icon = QIcon::fromTheme(QStringLiteral("..."));
        themeMenuActionGroup = manager->createSchemeSelectionMenu(icon, ...);



shell/thememanager.cpp (line 134)
<https://git.reviewboard.kde.org/r/127979/#comment65678>

    can you use a new-style connect here please?


- Milian Wolff


On July 6, 2016, 9:52 a.m., Alexander Zhigalin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127979/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 9:52 a.m.)
> 
> 
> Review request for KDevelop and Kevin Funk.
> 
> 
> Bugs: 279592
>     http://bugs.kde.org/show_bug.cgi?id=279592
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> [WIP] Implement color scheme selection from these installed in the system using KColorSchemeManager
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt ec521ab8f5dcdd1cc175809f56169259e241ea34 
>   shell/CMakeLists.txt 752c435b81d60e6bf9d438a0367662a8be346a33 
>   shell/mainwindow.h 8340064ee7e1a3b95695b430270cc848ac69eeac 
>   shell/mainwindow.cpp cf1b15d99365a1274f49bbde18bf3c5c17ba7ccb 
>   shell/thememanager.h PRE-CREATION 
>   shell/thememanager.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127979/diff/
> 
> 
> Testing
> -------
> 
> Does compile but doesn't work for some reason.
> 
> 
> Thanks,
> 
> Alexander Zhigalin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160710/a0dc0cfa/attachment-0001.html>