D26797: KCM/Component Refactor UI to a single list of combobox

[Kevin Ottens]

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

INLINE COMMENTS

> componentchooser.cpp:80
> +
> +    if (cfgType==QLatin1String("internal_email")) {
> +        loadedConfigWidget = new CfgEmailClient(this);

I know it's older code but since you touched it, what about adding the spaces around == ?
Likewise for the few ifs below.

> componentchooser.cpp:98
>  
> -void ComponentChooser::slotServiceSelected(QListWidgetItem* it) {
> -	if (!it) return;
> -
> -	if (somethingChanged) {
> -		if (KMessageBox::questionYesNo(this,i18n("<qt>You changed the default component of your choice, do want to save that change now ?</qt>"),QString(),KStandardGuiItem::save(),KStandardGuiItem::discard())==KMessageBox::Yes) save();
> -	}
> -	const QString &service = it->data(Qt::UserRole).toString();
> -	KConfig cfg(service, KConfig::SimpleConfig);
> -
> -	ComponentDescription->setText(cfg.group(QByteArray()).readEntry("Comment",i18n("No description available")));
> -	ComponentDescription->setMinimumSize(ComponentDescription->sizeHint());
> -
> -	configWidget = configWidgetMap.value(service);
> -	if (configWidget) {
> -        configContainer->setCurrentWidget(configWidget);
> -        const auto plugin = dynamic_cast<CfgPlugin*>(configWidget);
> -        headerGroupBox->setTitle(it->text());
> -        plugin->load(&cfg);
> -        emit defaulted(plugin->isDefaults());
> -	}
> -
> -	emitChanged(false);
> -	latestEditedService = service;
> -}
> +void ComponentChooser::emitChanged(bool val) {
>  

Curly brace on its own line.

> componentchooser.cpp:100
>  
> +    bool somethingChanged = val;
> +    if (!somethingChanged) {

Does that make any sense to keep the val parameter on that method if you loop through all the plugins because of defaulted anyway?
If it goes away the two loops could thus be merged.

> componentchooser.cpp:103
> +        // check if another plugin has changed
> +        for (auto it= configWidgetMap.constKeyValueBegin(); it != configWidgetMap.constKeyValueEnd(); ++it) {
> +            CfgPlugin *plugin = dynamic_cast<CfgPlugin *>( (*it).second );

Space before = missing

> componentchooser.cpp:104
> +        for (auto it= configWidgetMap.constKeyValueBegin(); it != configWidgetMap.constKeyValueEnd(); ++it) {
> +            CfgPlugin *plugin = dynamic_cast<CfgPlugin *>( (*it).second );
> +            somethingChanged |= plugin->hasChanged();

No need for the spaces within the parenthesis

> componentchooser.cpp:111
> +    bool isDefault = true;
> +    for (auto it= configWidgetMap.constKeyValueBegin(); it != configWidgetMap.constKeyValueEnd(); ++it) {
> +        CfgPlugin *plugin = dynamic_cast<CfgPlugin *>( (*it).second );

Ditto

> componentchooser.cpp:112
> +    for (auto it= configWidgetMap.constKeyValueBegin(); it != configWidgetMap.constKeyValueEnd(); ++it) {
> +        CfgPlugin *plugin = dynamic_cast<CfgPlugin *>( (*it).second );
> +        isDefault &= plugin->isDefaults();

Ditto

> componentchooser.cpp:127
>  void ComponentChooser::load() {
> -	if( configWidget )
> -	{
> -		CfgPlugin * plugin = dynamic_cast<CfgPlugin*>( configWidget );
> -		if( plugin )
> -		{
> -			KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> -			plugin->load( &cfg );
> -		}
> +    for (auto it= configWidgetMap.constKeyValueBegin(); it != configWidgetMap.constKeyValueEnd(); ++it) {
> +

Space before =

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

No space after *, no space within parenthesis

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

Ditto

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

Ditto

REPOSITORY
  R119 Plasma Desktop

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

To: meven, #plasma, #vdg, ngraham, ervin
Cc: 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200121/f83b32ef/attachment-0001.html>