Review Request 119602: support config dialog with current krunner config dialog

[Vishesh Handa]

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


It would be nice if you could upload a screenshot.


kcms/runners/kcm.h
<https://git.reviewboard.kde.org/r/119602/#comment44452>

    Why does this need to be a member variable?



kcms/runners/kcm.h
<https://git.reviewboard.kde.org/r/119602/#comment44453>

    Why does this need to be a member variable?



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44444>

    Hardcoding the size?



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44454>

    Just to be sure, the code path where the tab widget is used is completely untested, right? Or are there some plugins which provide the same categories?



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44455>

    I know this is just convention, but it owuld be nicer if you just returned over here.
    
    if (m_moduleProxyList.isEmpty())
        return;



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44456>

    I'm confused. When the user clicks cancel, then we reload all these proxy modules? And then we delete all of them?



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44446>

    I've only taken a cursory glance at the code, but  for each runner, you're reloading the config, and then iterating over all the plugins to create this hash.
    
    Maybe you just want to do that once?



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44445>

    coding style



kcms/runners/kcm.cpp
<https://git.reviewboard.kde.org/r/119602/#comment44448>

    Please use a proper enum.


- Vishesh Handa


On Aug. 4, 2014, 12:26 p.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119602/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2014, 12:26 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> -------
> 
> unlike the previous one, this still uses category based config.
> tough, if in a category there are runners with a config dialog, enable a config button in the item.
> If there would be more runners with config in one category, it would put all the config dialogs in a tabbar.
> The code for showing the config dialog comes from KPluginLoader
> 
> 
> Diffs
> -----
> 
>   kcms/runners/kcm.cpp 0de07fb 
>   kcms/runners/kcm.h 0458430 
> 
> Diff: https://git.reviewboard.kde.org/r/119602/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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