ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> componentchooser.cpp:62
> +        // fill the form layout
> +        const auto name = cg.readEntry("Name",i18n("Unknown"));
> +        CfgPlugin *loadedConfigWidget = 
> loadConfigWidget(cfg.group(QByteArray()).readEntry("configurationType"));

Missing space after comma

> componentchooser.cpp:106
> +    // check if another plugin has changed and default status
> +    for (CfgPlugin * plugin: configWidgetMap) {
> +        somethingChanged |= plugin->hasChanged();

No space after * and missing qAsConst

> componentchooser.cpp:137
>  void ComponentChooser::save() {
> -     if( configWidget )
> -     {
> -             CfgPlugin* plugin = dynamic_cast<CfgPlugin*>( configWidget );
> -             if( plugin )
> -             {
> -                     KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> +    for (auto it = configWidgetMap.constBegin(); it != 
> configWidgetMap.constEnd(); ++it){
> +

Missing space before {

> componentchooser.cpp:142
> +
> +        CfgPlugin *plugin = dynamic_cast<CfgPlugin*>( widget );
> +        if (plugin) {

No space between parenthesis

> componentchooser.cpp:154
>  void ComponentChooser::restoreDefault() {
> -    if (configWidget)
> -    {
> -        dynamic_cast<CfgPlugin*>(configWidget)->defaults();
> -        emitChanged(true);
> +    for (CfgPlugin* plugin : configWidgetMap) {
> +        plugin->defaults();

Space before the * and not after. Also you could have used auto or auto * for 
all those loops (just that const auto & makes no sense in that context).

> ervin wrote in componentchooser.cpp:132
> No space after *, no space within parenthesis

No space within parenthesis

> broulik wrote in componentchooserbrowser.cpp:104
> As a future step I would like those default components not hardcoded in the 
> code

Yes, it screams for GUI / config separation (in another commit)

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

Reply via email to