Changeset View
Changeset View
Standalone View
Standalone View
kcmkwin/common/effectsmodel.cpp
| Show First 20 Lines • Show All 557 Lines • ▼ Show 20 Line(s) | 556 | for (int i = 0; i < m_effects.count(); ++i) { | |||
|---|---|---|---|---|---|
| 558 | if (effect.enabledByDefaultFunction && effect.status != Status::EnabledUndeterminded) { | 558 | if (effect.enabledByDefaultFunction && effect.status != Status::EnabledUndeterminded) { | ||
| 559 | updateEffectStatus(index(i, 0), Status::EnabledUndeterminded); | 559 | updateEffectStatus(index(i, 0), Status::EnabledUndeterminded); | ||
| 560 | } else if ((bool)effect.status != effect.enabledByDefault) { | 560 | } else if ((bool)effect.status != effect.enabledByDefault) { | ||
| 561 | updateEffectStatus(index(i, 0), effect.enabledByDefault ? Status::Enabled : Status::Disabled); | 561 | updateEffectStatus(index(i, 0), effect.enabledByDefault ? Status::Enabled : Status::Disabled); | ||
| 562 | } | 562 | } | ||
| 563 | } | 563 | } | ||
| 564 | } | 564 | } | ||
| 565 | 565 | | |||
| 566 | bool EffectsModel::isDefaults() const | ||||
| 567 | { | ||||
| 568 | return std::all_of(m_effects.constBegin(), m_effects.constEnd(), [](const EffectData &effect) { | ||||
zzag: Minor thing, but could you please use std::all_of? | |||||
| 569 | if (effect.enabledByDefaultFunction && effect.status != Status::EnabledUndeterminded) { | ||||
I suspect this lambda would have benefited from being a single return statement (not 100% sure though) ervin: I suspect this lambda would have benefited from being a single return statement (not 100% sure… | |||||
| 570 | return false; | ||||
| 571 | } | ||||
zzag: No `else` after a return statement. | |||||
This inline comment hasn't been addressed. I asked you to remove a else after a return statement, i.e. if (effect.enabledByDefaultFunction && effect.status != Status::EnabledUndeterminded) { return false; } if ((bool)effect.status != effect.enabledByDefault) { return false; } return true; zzag: This inline comment hasn't been addressed.
I asked you to remove a `else` after a `return`… | |||||
I misunderstood what you meant, in my last change I added : } else {
return true;
}Should be good now meven: I misunderstood what you meant, in my last change I added :
```
} else {
return true;
}… | |||||
| 572 | if ((bool)effect.status != effect.enabledByDefault) { | ||||
ervin: Urgh! C-style cast, please don't. Should be a static_cast or something. | |||||
Agreed, using it in modern C++ code is not good but on the other hand we have (bool)effect.status != effect.enabledByDefault just right above this method. If my memory serves me well, the c-style cast is leftover from the monolithic compositing kcm. This minor issue can be addressed in another patch though... zzag: Agreed, using it in modern C++ code is not good but on the other hand we have `(bool)effect. | |||||
ervin: Sure, can only be in another patch anyway, that one got merged anyway. ;-) | |||||
| 573 | return false; | ||||
| 574 | } | ||||
| 575 | return true; | ||||
| 576 | }); | ||||
| 577 | } | ||||
| 578 | | ||||
| 566 | bool EffectsModel::needsSave() const | 579 | bool EffectsModel::needsSave() const | ||
| 567 | { | 580 | { | ||
| 568 | return std::any_of(m_effects.constBegin(), m_effects.constEnd(), | 581 | return std::any_of(m_effects.constBegin(), m_effects.constEnd(), | ||
| 569 | [](const EffectData &data) { | 582 | [](const EffectData &data) { | ||
| 570 | return data.changed; | 583 | return data.changed; | ||
| 571 | } | 584 | } | ||
| 572 | ); | 585 | ); | ||
| 573 | } | 586 | } | ||
| ▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Line(s) | 653 | auto buttons = new QDialogButtonBox( | |||
| 642 | QDialogButtonBox::Cancel | | 655 | QDialogButtonBox::Cancel | | ||
| 643 | QDialogButtonBox::RestoreDefaults, | 656 | QDialogButtonBox::RestoreDefaults, | ||
| 644 | dialog | 657 | dialog | ||
| 645 | ); | 658 | ); | ||
| 646 | connect(buttons, &QDialogButtonBox::accepted, dialog, &QDialog::accept); | 659 | connect(buttons, &QDialogButtonBox::accepted, dialog, &QDialog::accept); | ||
| 647 | connect(buttons, &QDialogButtonBox::rejected, dialog, &QDialog::reject); | 660 | connect(buttons, &QDialogButtonBox::rejected, dialog, &QDialog::reject); | ||
| 648 | connect(buttons->button(QDialogButtonBox::RestoreDefaults), &QPushButton::clicked, | 661 | connect(buttons->button(QDialogButtonBox::RestoreDefaults), &QPushButton::clicked, | ||
| 649 | module, &KCModule::defaults); | 662 | module, &KCModule::defaults); | ||
| 663 | connect(module, &KCModule::defaulted, this, [=](bool defaulted) { | ||||
God I wish we had good support for clang-format. Could you please remove whitespace between [=] and (bool defaulted)? Sorry for annoying you with whitespace issues. :/ zzag: God I wish we had good support for clang-format. Could you please remove whitespace between `… | |||||
| 664 | buttons->button(QDialogButtonBox::RestoreDefaults)->setEnabled(!defaulted); | ||||
| 665 | }); | ||||
| 650 | 666 | | |||
| 651 | auto layout = new QVBoxLayout(dialog); | 667 | auto layout = new QVBoxLayout(dialog); | ||
| 652 | layout->addWidget(module); | 668 | layout->addWidget(module); | ||
| 653 | layout->addWidget(buttons); | 669 | layout->addWidget(buttons); | ||
| 654 | 670 | | |||
| 655 | if (dialog->exec() == QDialog::Accepted) { | 671 | if (dialog->exec() == QDialog::Accepted) { | ||
| 656 | module->save(); | 672 | module->save(); | ||
| 657 | } | 673 | } | ||
| Show All 11 Lines | |||||
Minor thing, but could you please use std::all_of?