Review Request 123279: Add color tab feature

Milian Wolff mail at milianw.de
Thu Apr 23 23:11:34 UTC 2015 

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



plugins/projectmanagerview/projectmodelitemdelegate.cpp (line 35)
<https://git.reviewboard.kde.org/r/123279/#comment54240>

    unrelated change (and the notes.txt above should also be removed from the patch).



plugins/projectmanagerview/projecttreeview.h (line 55)
<https://git.reviewboard.kde.org/r/123279/#comment54241>

    this is an override? mark it as such (and remove the virtual keyword instead)
    
    also, please add spaces after operators, here: after the commas.
    
    then remove the trailing whitespace in the next line please



plugins/projectmanagerview/projecttreeview.cpp (line 455)
<https://git.reviewboard.kde.org/r/123279/#comment54242>

    I'd write it like this (also note: off by default):
    
    const bool applyColor = KSharedConfig::openConfig()->group( "UiSettings" ).readEntry("TabBarColor", 0);
    if (applyColor) {
       ...
    }



plugins/projectmanagerview/projecttreeview.cpp (line 457)
<https://git.reviewboard.kde.org/r/123279/#comment54243>

    this is, I think, quite costly. Can you try using the ProjectModel instead, see e.g. vcsoverlayproxymodel.cpp for how to use it.



plugins/projectmanagerview/projecttreeview.cpp (line 464)
<https://git.reviewboard.kde.org/r/123279/#comment54244>

    did you test this with a dark color scheme? I guess it won't look good and you'll need to use KColorScheme and some tinting or similar.



shell/mainwindow.h (line 53)
<https://git.reviewboard.kde.org/r/123279/#comment54245>

    here and below: remove unrelated whitespace changes please



shell/mainwindow.cpp (line 204)
<https://git.reviewboard.kde.org/r/123279/#comment54250>

    here and below: rtrim the unrelated whitespace changes



shell/mainwindow.cpp (line 356)
<https://git.reviewboard.kde.org/r/123279/#comment54246>

    style: put the { on its own line, like in the other functions



shell/mainwindow.cpp (line 357)
<https://git.reviewboard.kde.org/r/123279/#comment54247>

    the default should be off, no? also, please use bool instead of 0 / 1



shell/mainwindow.cpp (line 358)
<https://git.reviewboard.kde.org/r/123279/#comment54248>

    you can use this blindly, it will exist



shell/mainwindow.cpp (line 375)
<https://git.reviewboard.kde.org/r/123279/#comment54251>

    if (!urlDoc) {
        return;
    }



shell/mainwindow.cpp (line 381)
<https://git.reviewboard.kde.org/r/123279/#comment54252>

    auto project = Core::self()->projectController()->findProjectForUrl(urlDoc->url());
    if (!project) {
        return;
    }



shell/mainwindow.cpp (line 390)
<https://git.reviewboard.kde.org/r/123279/#comment54249>

    isEmpty? but with my code above you can just remove this, if you have a valid project, it will have a name



shell/mainwindow.cpp (line 392)
<https://git.reviewboard.kde.org/r/123279/#comment54253>

    share this code with the tree view somehow, otherwise the colors will easily get out-of-sync



shell/mainwindow.cpp (line 398)
<https://git.reviewboard.kde.org/r/123279/#comment54262>

    in the container you find the urlDoc again for the string, which is a huge waste of resources. just pass in the URLDoc in the API here and then find the corresponding tab



shell/project.cpp (line 164)
<https://git.reviewboard.kde.org/r/123279/#comment54254>

    unrelated whitespace changes



shell/project.cpp (line 228)
<https://git.reviewboard.kde.org/r/123279/#comment54255>

    I'd rather this to be self-consistent. listen to a projectOpened/Closed signal in the UIController and update the tab colors then. remove all changes here



shell/settings/uiconfig.kcfg (line 16)
<https://git.reviewboard.kde.org/r/123279/#comment54257>

    default should be off



shell/uicontroller.cpp (line 340)
<https://git.reviewboard.kde.org/r/123279/#comment54256>

    unrelated



sublime/container.h (line 63)
<https://git.reviewboard.kde.org/r/123279/#comment54258>

    rtrim, also below



sublime/container.h (line 64)
<https://git.reviewboard.kde.org/r/123279/#comment54259>

    here and below: take QColor and QString by const&



sublime/container.cpp (line 480)
<https://git.reviewboard.kde.org/r/123279/#comment54260>

    put { on its own line (see other functions)



sublime/container.cpp (line 481)
<https://git.reviewboard.kde.org/r/123279/#comment54261>

    here and below: remove this->



sublime/container.cpp (line 490)
<https://git.reviewboard.kde.org/r/123279/#comment54263>

    as I said above, improve the API and take in the UrlDocument directly, no need for the conversions and string comparisons



sublime/urldocument.h (line 34)
<https://git.reviewboard.kde.org/r/123279/#comment54264>

    is this correct, I've pushed it now so you can remove this from your patch.


- Milian Wolff


On April 23, 2015, 10:20 p.m., Sebastien Speierer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123279/
> -----------------------------------------------------------
> 
> (Updated April 23, 2015, 10:20 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This patch adds a checkbox in the KDevelop setting that allows KDevelop to color the tabs (of the open files) depending of their project.
> This feature was asked by several users on the KDE bugtracking system.
> 
> 
> Diffs
> -----
> 
>   .kateproject.d/notes.txt PRE-CREATION 
>   plugins/projectmanagerview/projectmodelitemdelegate.cpp 69fd6c4 
>   plugins/projectmanagerview/projecttreeview.h 6512157 
>   plugins/projectmanagerview/projecttreeview.cpp dbd25ff 
>   shell/mainwindow.h ae22648 
>   shell/mainwindow.cpp 8757942 
>   shell/project.cpp e7e6efa 
>   shell/settings/uiconfig.kcfg b168294 
>   shell/settings/uiconfig.ui a0d76de 
>   shell/uicontroller.cpp 2108ff2 
>   sublime/container.h 9049f55 
>   sublime/container.cpp 4444077 
>   sublime/urldocument.h 4a02d89 
> 
> Diff: https://git.reviewboard.kde.org/r/123279/diff/
> 
> 
> Testing
> -------
> 
> I tested the feature (open file before/after enable the coloring, tried to open files from several different projects,...), the implementation seems robust.
> 
> 
> File Attachments
> ----------------
> 
> Screenshot01
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/22/c544c7ee-765e-4851-be5c-86d5b2241dab__kdevelop5_env_1.png
> Screenshot #2 project_manager
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/23/79fa321d-3952-43d7-83e8-4af07116c82f__kdevelop5_env_2.png
> 
> 
> Thanks,
> 
> Sebastien Speierer
> 
>

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

More information about the KDevelop-devel mailing list