D25520: GridViewKCM expose a property to disable the GridView in a KCM without disabling the whole KCM

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R296 KDeclarative

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

To: crossi, #plasma, ervin, bport, mart, davidedmundson
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25149: Add a new template for KCMs

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> kcm.cpp:37
> +KAboutData* about = new KAboutData(
> +QStringLiteral("kcm_%{APPNAMELC}"), i18n("%{APPNAME} Configuration 
> Module"),
> +QStringLiteral("0.1"), QString(), KAboutLicense::GPL,

Nitpick: indentation looks broken here, I think it wasn't earlier.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24224: Start of the accessibility KCM

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  A few changes still needed.
  
  Also, in all the QML code we need to handle immutability of the settings 
(unfortunately that part can't be easily made magic like for the widgets). This 
would require for each control to have something like:
  `enabled: kcm.fooSettings.isImmutable("mySetting")`

INLINE COMMENTS

> kcmaccess.cpp:260
>  
> -void KAccessConfig::changeFlashScreenColor()
> -{
> -ui.invertScreen->setChecked(false);
> -ui.flashScreen->setChecked(true);
> -configChanged();
> -}
> -
> -void KAccessConfig::selectSound()
> -{
> -const QStringList list = 
> QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, 
> QStringLiteral("sound/"));
> -QString start;
> -if (!list.isEmpty())
> -start = list[0];
> -const QString fname = QFileDialog::getOpenFileName(this, QString(), 
> start);
> -if (!fname.isEmpty())
> -ui.soundEdit->setText(fname);
> -}
> -
> -
> -void KAccessConfig::configChanged()
> -{
> -emit changed(true);
> -}
> -
> -
>  void KAccessConfig::load()
>  {

I think this override can completely go away.

> kcmaccess.cpp:271
>  {
> -KConfigGroup cg(KSharedConfig::openConfig(QStringLiteral("kaccessrc")), 
> "Bell");
> -
> -cg.writeEntry("SystemBell", ui.systemBell->isChecked());
> -cg.writeEntry("ArtsBell", ui.customBell->isChecked());
> -cg.writePathEntry("ArtsBellFile", ui.soundEdit->text());
> -
> -cg.writeEntry("VisibleBell", ui.visibleBell->isChecked());
> -cg.writeEntry("VisibleBellInvert", ui.invertScreen->isChecked());
> -cg.writeEntry("VisibleBellColor", ui.colorButton->color());
> -
> -cg.writeEntry("VisibleBellPause", ui.duration->value());
> -cg.sync();
> -
> -KConfigGroup 
> keyboardGroup(KSharedConfig::openConfig(QStringLiteral("kaccessrc")), 
> "Keyboard");
> -
> -keyboardGroup.writeEntry("StickyKeys", ui.stickyKeys->isChecked());
> -keyboardGroup.writeEntry("StickyKeysLatch", 
> ui.stickyKeysLock->isChecked());
> -keyboardGroup.writeEntry("StickyKeysAutoOff", 
> ui.stickyKeysAutoOff->isChecked());
> -keyboardGroup.writeEntry("StickyKeysBeep", 
> ui.stickyKeysBeep->isChecked());
> -keyboardGroup.writeEntry("ToggleKeysBeep", 
> ui.toggleKeysBeep->isChecked());
> -keyboardGroup.writeEntry("kNotifyModifiers", 
> ui.kNotifyModifiers->isChecked());
> -
> -keyboardGroup.writeEntry("SlowKeys", ui.slowKeys->isChecked());
> -keyboardGroup.writeEntry("SlowKeysDelay", ui.slowKeysDelay->value());
> -keyboardGroup.writeEntry("SlowKeysPressBeep", 
> ui.slowKeysPressBeep->isChecked());
> -keyboardGroup.writeEntry("SlowKeysAcceptBeep", 
> ui.slowKeysAcceptBeep->isChecked());
> -keyboardGroup.writeEntry("SlowKeysRejectBeep", 
> ui.slowKeysRejectBeep->isChecked());
> -
> -
> -keyboardGroup.writeEntry("BounceKeys", ui.bounceKeys->isChecked());
> -keyboardGroup.writeEntry("BounceKeysDelay", ui.bounceKeysDelay->value());
> -keyboardGroup.writeEntry("BounceKeysRejectBeep", 
> ui.bounceKeysRejectBeep->isChecked());
> -
> -keyboardGroup.writeEntry("Gestures", ui.gestures->isChecked());
> -keyboardGroup.writeEntry("AccessXTimeout", ui.timeout->isChecked());
> -keyboardGroup.writeEntry("AccessXTimeoutDelay", 
> ui.timeoutDelay->value());
> -
> -keyboardGroup.writeEntry("AccessXBeep", ui.accessxBeep->isChecked());
> -keyboardGroup.writeEntry("GestureConfirmation", 
> ui.gestureConfirmation->isChecked());
> -keyboardGroup.writeEntry("kNotifyAccess", ui.kNotifyAccess->isChecked());
> -
> +m_mouseSettings->save();
> +m_keyboardSettings->save();

I don't think those calls to save are still needed. For sure you need to call 
the super class save() implementation though.

> bport wrote in kcmaccess.cpp:171
> new MouseSettings(this)
> 
> in order to avoid memory leak (same for other settings)

What @bport said, pass the parent to the ctor (will require adjusting the kcfgc 
files to get the corresponding ctors generated though).
Also it's generally better to do this kind of member initialization in the ctor 
member initializers list.

> kcmaccessibilitymouse.kcfg:34
> +
> +
> +  Mouse Key Curve

Wouldn't an Enum be more suited here?

> CMakeLists.txt:26
>  KF5::WindowSystem
> +Qt5::X11Extras
>  )

Why is there still changes in colors?

REPOSITORY
  R119 Plasma Desktop

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

To: tcanabrava, ngraham, bport, ervin
Cc: broulik, bport, ervin, mart, ngraham, whiting, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D25321: Remove dead code

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, davidedmundson, crossi, bport.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REPOSITORY
  R133 KScreenLocker

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

AFFECTED FILES
  kcm/kcm.cpp
  kcm/kcm.h

To: ervin, #plasma, davidedmundson, crossi, bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25322: Have the wallpaper combo managed by KConfigDialogManager

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, davidedmundson, crossi, bport.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  We use an intermediate property which maps to an index since it's
  exactly what KConfigDialogManager expects with a QComboBox.
  
  Also relayouted a bit the ctor and the file to make it easier to
  understand (several aspect were weaved together).

REPOSITORY
  R133 KScreenLocker

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

AFFECTED FILES
  CMakeLists.txt
  greeter/greeterapp.cpp
  kcfg/kscreenlockersettings.kcfg
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/kcm.ui
  kscreensaversettings.cpp
  kscreensaversettings.h

To: ervin, #plasma, davidedmundson, crossi, bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25290: KCM launchfeedback : port to KConfig XT

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> crossi wrote in main.qml:33
> I agree it's a bit tricky, we have a kind of enum choice written as a binary 
> representation in the config file.
> 
> Will try to work it with a QML enum to make it less confusing.

Or... but that's more work and I'd say it should come in another patch on top 
of that one. We migrate the config file to use an enum all the way. This 
boolean trap exists in the config after all.

REPOSITORY
  R119 Plasma Desktop

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

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

D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.

[Kevin Ottens]

ervin accepted this revision.
ervin added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> crossi wrote in kcmstyle.cpp:245
> m_effectsDirty is checked line 291, to trigger or not a refresh of all 
> applications.
> 
> This check before save is now useless, since if we can save, it means a 
> modification has occured, style or effects.

Not for this patch, but I still wonder if we could kill it. I think it'd be 
better to have it as a local variable whose value would be computed from the 
state of the relevant items in the settings object. This would remove the logic 
of maintaining an extra state exploited only in save(). Also "effectsDirty" is 
a weird name I think, it doesn't seem related to any concept of effects.

REPOSITORY
  R119 Plasma Desktop

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

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

D25149: Add a new template for KCMs

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> tcanabrava wrote in kcm.cpp:43
> What's the correct form then? No need to connect the settings *at all*?

No need indeed... it's *magic*! ;-)

> tcanabrava wrote in main.qml:39
> The default fake setting will not be immutable, or you mean that you want me 
> to do one mutable and one immutable settings for the example?

In fact you can't predict that, any setting can be made immutable (it's user 
and sysadmin controlled). No need to introduce another one.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Kevin Ottens]

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

INLINE COMMENTS

> %{APPNAMELC}settings.kcfgc:5
> +DefaultValueGetters=true
> +GenerateProperties=true

You also want "ParentInConstructor=true" in here.

> kcm.cpp:32
> +: KQuickAddons::ConfigModule(parent, args)
> +, m_settings(new %{APPNAME}Settings())
> +{

Pass this as parent here (currently you're leaking it)

> kcm.cpp:43
> +
> +connect(m_settings, &%{APPNAME}Settings::configChanged, this, [this] { 
> setNeedsSave(true); });
> +

Shouldn't be needed anymore (and likely wrong in most cases).

> kcm.h:26
> +
> +class %{APPNAME} : public KQuickAddons::ConfigModule
> +{

This should inherit from ManagedConfigModule now.

> kcm.h:36
> +
> +public Q_SLOTS:
> +void load() override;

None of those slots are needed with a ManagedConfigModule (except if you need 
to do something outside the realm of the settings of course, which is not the 
case by default.

> main.qml:39
> +
> +QQC2.TextField {
> +text: kcm.settings.exampleSetting

What about disabling it if the setting is immutable?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25287: Integrate the lnf and wallpaper settings with the KCM logic

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, mart, davidedmundson, bport, crossi.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This was currently very much disconnected and broken in quite a few
  cases. The main changes here are:
  
  - disabling ConfigPropertyMap autosave behavior
  - having the QML integration write directly in the settings objects
  - use the settings objects to update the state of the KCM propertly
  
  Further improvement is needed to simplify the handling of the wallpaper
  plugin combo, it's still a bit over the place and fragile.

REPOSITORY
  R133 KScreenLocker

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

AFFECTED FILES
  greeter/lnf_integration.cpp
  greeter/lnf_integration.h
  greeter/wallpaper_integration.cpp
  greeter/wallpaper_integration.h
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/lnfconfig.qml
  kcm/wallpaperconfig.qml

To: ervin, #plasma, mart, davidedmundson, bport, crossi
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25286: Allow to disable autosave behavior in ConfigPropertyMap

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:40b94af6f0f4: Allow to disable autosave behavior in 
ConfigPropertyMap (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25286?vs=69676=69677

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

AFFECTED FILES
  src/kdeclarative/configpropertymap.cpp
  src/kdeclarative/configpropertymap.h

To: ervin, #plasma, #frameworks, mart, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25286: Allow to disable autosave behavior in ConfigPropertyMap

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson, bport.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This is especially important when ConfigPropertyMap is used with KCMs,
  if it keeps saving on any change we never get the chance to know if the
  config was dirty or not.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/kdeclarative/configpropertymap.cpp
  src/kdeclarative/configpropertymap.h

To: ervin, #plasma, #frameworks, mart, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25212: Move the shortcut management in the settings object

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R133:68bdeb8cd7d9: Move the shortcut management in the 
settings object (authored by ervin).

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25212?vs=69435=69675

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

AFFECTED FILES
  CMakeLists.txt
  greeter/CMakeLists.txt
  greeter/greeterapp.cpp
  kcfg/kscreensaversettings.kcfgc
  kcfg/kscreensaversettingsbase.kcfgc
  kcm/CMakeLists.txt
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/kcm.ui
  kscreensaversettings.cpp
  kscreensaversettings.h
  ksldapp.cpp

To: ervin, #plasma, davidedmundson, bport, crossi, mart
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25210: Prepare KConfigSkeletonItem to allow inheriting its private class

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:44cfa0631d25: Prepare KConfigSkeletonItem to allow 
inheriting its private class (authored by ervin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25210?vs=69440=69673

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi, vkrause
Cc: vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25211: Add KPropertySkeletonItem

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:d63955cfe547: Add KPropertySkeletonItem (authored by 
ervin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25211?vs=69628=69674

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

[frameworks-kconfigwidgets] [Bug 413980] "Defaults" button should restore system wide defaults

[Kevin Ottens]

https://bugs.kde.org/show_bug.cgi?id=413980

--- Comment #4 from Kevin Ottens  ---
Currently 100% done (with all patches merged): desktoptheme and ksplash. There
are more which are basically done but in various stages of review, and even
more in the works.

-- 
You are receiving this mail because:
You are watching all bug changes.

D25274: KCM LookAndFeel : port to KConfig XT

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  Just a recurring typo, otherwise LGTM

INLINE COMMENTS

> kcm.cpp:220
> +
> +//Model has been cleared so pretend the selected look and fell changed 
> to force view update
> +emit m_settings->lookAndFeelPackageChanged();

s/fell/feel/ :-)

> kcm.h:38
>  Q_OBJECT
> +Q_PROPERTY(LookAndFeelSettings *lookAndFellSettings READ 
> lookAndFellSettings CONSTANT)
>  Q_PROPERTY(QStandardItemModel *lookAndFeelModel READ lookAndFeelModel 
> CONSTANT)

s/Fell/Feel/

> kcm.h:64
>  
> -//List only packages which provide at least one of the components
> -QList availablePackages(const QStringList 
> );
> -
> +LookAndFeelSettings *lookAndFellSettings() const;
>  QStandardItemModel *lookAndFeelModel() const;

s/Fell/Feel/

> main.qml:32
>  view.model: kcm.lookAndFeelModel
> -view.currentIndex: kcm.selectedPluginIndex
> +view.currentIndex: 
> kcm.pluginIndex(kcm.lookAndFellSettings.lookAndFeelPackage)
>  view.delegate: KCM.GridDelegate {

s/Fell/Feel/

> main.qml:58
>  onClicked: {
> -kcm.selectedPlugin = model.pluginName;
> +kcm.lookAndFellSettings.lookAndFeelPackage = model.pluginName;
>  view.forceActiveFocus();

s/Fell/Feel/

REPOSITORY
  R119 Plasma Desktop

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

To: crossi, #plasma, ervin, mart, bport, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> crossi wrote in kcmstyle.cpp:248
> m_model->selectedStyle() is bound to m_settings->widgetStyle() and vice 
> versa, so I need to save previous state to allow a rollback.
> I should have used m_settings->widgetStyle() here and after for consistency.

As far as rollback goes (might not be enough in your case), wouldn't checking 
for isSaveNeeded() on the settings and rollbacking it all be enough?

We can't rollback a single item though, I guess that would be the main 
limitation in your case... Maybe that's something to add to KConfigSkeletonItem?

Answer might be no to both, I'm just looking for opportunities to simplify code 
in modules. :-)

REPOSITORY
  R119 Plasma Desktop

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

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

D25212: Move the shortcut management in the settings object

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kcm.cpp:198
> This part appears to have gone?

Yes, it was never triggered. It turns out KKeySequenceWidget does that already 
as soon as you pick a new shortcut, so it could never end up conflicting here 
since said conflict would have been handled before.

> davidedmundson wrote in kscreensaversettings.cpp:46
> given we have defined the defaults at a kconfigskeleton level should we just 
> call ->shortcut() here?

Not sure what you mean here. We're inside the KConfigSkeleton.

Note that KGlobalAccel seems to expect a first call to setShortcut to associate 
the local QAction with the information from KGlobalAccel. I admit KGlobalAccel 
looks like an odd beast to me.

> davidedmundson wrote in kscreensaversettings.cpp:48
> Could we handle the alternative shortcut by using a second property?

We could but it's not exposed in the KCM, not sure what the gain would be.

REPOSITORY
  R133 KScreenLocker

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

To: ervin, #plasma, davidedmundson, bport, crossi, mart
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> kcmstyle.cpp:98
>  
> -connect(m_model, ::selectedStyleChanged, this, [this] {
> -m_selectedStyleDirty = true;
> -setNeedsSave(true);
> +connect(m_model, ::selectedStyleChanged, this, [this](const 
> QString& style) {
> +m_settings->setWidgetStyle(style);

Space before &, not after.

> kcmstyle.cpp:245
>  {
> -if (!m_selectedStyleDirty && !m_effectsDirty) {
> -return;

AFAICT this is the only place where m_effectsDirty is read. So I think you can 
remove that flag altogether, which should bring further simplifications.

> kcmstyle.cpp:248
>  bool newStyleLoaded = false;
> -if (m_selectedStyleDirty) {
> +if (m_model->selectedStyle() != m_previousStyle) {
>  QScopedPointer 
> newStyle(QStyleFactory::create(m_model->selectedStyle()));

Are you sure you need this m_previousStyle? I'd expect it to be exactly 
m_settings->widgetStyle().

REPOSITORY
  R119 Plasma Desktop

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

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

D25211: Add KPropertySkeletonItem

[Kevin Ottens]

ervin updated this revision to Diff 69628.
ervin added a comment.


  Avoid potential ownership problem on the propertyName

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25211?vs=69442=69628

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25249: KDirModel: port to qCDebug, with its own category

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: dfaure, mlaurent, sandsmark, ervin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25211: Add KPropertySkeletonItem

[Kevin Ottens]

ervin updated this revision to Diff 69442.
ervin added a comment.


  Fix member variable naming convention

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25211?vs=69432=69442

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25210: Prepare KConfigSkeletonItem to allow inheriting its private class

[Kevin Ottens]

ervin added a comment.


  In D25210#559977 , @vkrause wrote:
  
  > Ok as such, but I think KConfigSkeletonItemPrivate would need a virtual 
dtor for this to actually work safely when used by a sub-class?
  
  
  You mean like not leaking memory? woops. ;-)

REPOSITORY
  R237 KConfig

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

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25210: Prepare KConfigSkeletonItem to allow inheriting its private class

[Kevin Ottens]

ervin updated this revision to Diff 69440.
ervin added a comment.


  Add missing virtual dtor

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25210?vs=69431=69440

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25209: Register KKeySequenceWidget to KConfigDialogManager

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:c0a8cd4a2a95: Register KKeySequenceWidget to 
KConfigDialogManager (authored by ervin).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25209?vs=69430=69439

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

AFFECTED FILES
  src/kconfigdialogmanager.cpp

To: ervin, #frameworks, dfaure, davidedmundson, bport, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25208: Add missing property to KKeySequenceWidget

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R263:fab6d95a9fad: Add missing property to KKeySequenceWidget 
(authored by ervin).

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25208?vs=69429=69436

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

AFFECTED FILES
  src/kkeysequencewidget.h

To: ervin, #frameworks, dfaure, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25212: Move the shortcut management in the settings object

[Kevin Ottens]

ervin updated this revision to Diff 69435.
ervin added a comment.


  Remove stale updateState() function

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25212?vs=69433=69435

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

AFFECTED FILES
  CMakeLists.txt
  greeter/CMakeLists.txt
  greeter/greeterapp.cpp
  kcfg/kscreensaversettings.kcfgc
  kcfg/kscreensaversettingsbase.kcfgc
  kcm/CMakeLists.txt
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/kcm.ui
  kscreensaversettings.cpp
  kscreensaversettings.h
  ksldapp.cpp

To: ervin, #plasma, davidedmundson, bport, crossi
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25212: Move the shortcut management in the settings object

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, davidedmundson, bport, crossi.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This uses the new KPropertySkeletonItem facility to allow proper
  integration with KGlobalAccel while keeping all the nice semantic coming
  from KConfigXT.
  
  This required turning the kconfig_compiler code into a base class and
  inheriting from it to introduce the property managed via
  KPropertySkeletonItem.

REPOSITORY
  R133 KScreenLocker

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

AFFECTED FILES
  CMakeLists.txt
  greeter/CMakeLists.txt
  greeter/greeterapp.cpp
  kcfg/kscreensaversettings.kcfgc
  kcfg/kscreensaversettingsbase.kcfgc
  kcm/CMakeLists.txt
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/kcm.ui
  kscreensaversettings.cpp
  kscreensaversettings.h
  ksldapp.cpp

To: ervin, #plasma, davidedmundson, bport, crossi
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25210: Prepare KConfigSkeletonItem to allow inheriting its private class

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Frameworks, dfaure, davidedmundson, bport, crossi.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REPOSITORY
  R237 KConfig

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25211: Add KPropertySkeletonItem

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Frameworks, dfaure, davidedmundson, bport, crossi.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This new item allows to use a QObject property as a source for a
  setting. This is especially convenient for using with KCMs present in
  systemsettings which tend to store information outside of KConfig (for
  interfacing deeper with the system).

REPOSITORY
  R237 KConfig

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

AFFECTED FILES
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

To: ervin, #frameworks, dfaure, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25209: Register KKeySequenceWidget to KConfigDialogManager

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Frameworks, dfaure, davidedmundson, bport.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This makes it usable in a KConfigXT context.
  
  Depends on KKeySequenceWidget property to be available,
  introduced with: https://phabricator.kde.org/D25208

REPOSITORY
  R265 KConfigWidgets

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

AFFECTED FILES
  src/kconfigdialogmanager.cpp

To: ervin, #frameworks, dfaure, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25208: Add missing property to KKeySequenceWidget

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Frameworks, dfaure, davidedmundson, bport.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Oddly enough we had all the necessary to manage keySequence as a
  property but that was never exposed. This makes it for instance
  unavailable to Qt Designer and impossible to use with KConfigXT.

REPOSITORY
  R263 KXmlGui

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

AFFECTED FILES
  src/kkeysequencewidget.h

To: ervin, #frameworks, dfaure, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25198: KCM Colors : port color scheme state management to KConfigXT

[Kevin Ottens]

ervin added a comment.


  In D25198#559853 , @broulik wrote:
  
  > Note that the KCM also writes all of the colors into kdeglobals (for 
whatever reason) - does this also need to be ported? We don't read those colors 
in the KCM anymore, just write them out when applying, so maybe not.
  
  
  I assumed the styles were reading them... but maybe that's to be confirmed 
indeed.

REPOSITORY
  R119 Plasma Desktop

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

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

D25147: Remove gtkrc-2.0 legacy settings

[Kevin Ottens]

ervin added a comment.


  FWIW, LGTM as well, agree with Nate that it likely requires some more 
extensive testing though.

REPOSITORY
  R99 KDE Gtk Configuration Tool

BRANCH
  remove-legacy-gtkrc-settings (branched from master)

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

To: gikari, cblack, #plasma, apol
Cc: ervin, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new Template for KCM's.

[Kevin Ottens]

ervin added a comment.


  What about having the template use kcfg and ManagedConfigModule as well? It'd 
give better behaving modules by default and is a better practice than starting 
without them. I'd rather have people make the conscious decision to remove them 
because it turns out it's a KCM with no KConfig at all than people overlooking 
proper KConfig use by default.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

[zanshin] [Bug 413813] Hide completed tasks

[Kevin Ottens]

https://bugs.kde.org/show_bug.cgi?id=413813

Kevin Ottens  changed:

   What|Removed |Added

 Status|REPORTED|CONFIRMED
 Ever confirmed|0   |1

--- Comment #1 from Kevin Ottens  ---
Yes, I never get around to actually implement it, but that's clearly something
missing which I deem useful. I personally wouldn't have a use for it, I got the
habit of just killing done tasks on a regular basis during my reviews.

-- 
You are receiving this mail because:
You are watching all bug changes.

D25074: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R124:fe180972acbb: Disable the restore defaults button if the 
KCModule says so (authored by ervin).

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25074?vs=69275=69277

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

AFFECTED FILES
  core/ModuleView.cpp
  core/ModuleView.h

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

D25074: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in ModuleView.cpp:427
> Not saying this is wrong, but can you explain this change?
> 
> In https://phabricator.kde.org/D25072
> 
> we do setEnabled((buttons & Default) && !defaulted) rather than changing 
> visibility, presumably it's there to make sure buttons don't move as you 
> navigate between modules.

It feels kind of wrong indeed. Better align that behavior with the one from 
kcmshell. I'll make a reworked patch in that direction.

REPOSITORY
  R124 System Settings

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

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

D25074: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

ervin updated this revision to Diff 69275.
ervin added a comment.


  Deal with David's comment and realign ModuleView with KCMultiDialog regarding 
button state management.

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25074?vs=69044=69275

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

AFFECTED FILES
  core/ModuleView.cpp
  core/ModuleView.h

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

D25072: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R295:f2f04d67e221: Disable the restore defaults button if the 
KCModule says so (authored by ervin).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25072?vs=69042=69274

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

AFFECTED FILES
  src/kcmultidialog.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25070: Make KCModuleQml conform to the defaulted() signal

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R295:4dc999b9eba1: Make KCModuleQml conform to the defaulted() 
signal (authored by ervin).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25070?vs=69040=69272

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

AFFECTED FILES
  src/kcmoduleqml.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25071: Have KCModuleProxy take care of the defaulted state

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R295:dc836403bb5d: Have KCModuleProxy take care of the 
defaulted state (authored by ervin).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25071?vs=69270=69273

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

AFFECTED FILES
  src/kcmoduleproxy.cpp
  src/kcmoduleproxy.h
  src/kcmoduleproxy_p.h

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25069: Adjust KCModule to also channel information about defaults

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:0762203eb9df: Adjust KCModule to also channel information 
about defaults (authored by ervin).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25069?vs=69269=69271

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

AFFECTED FILES
  src/kcmodule.cpp
  src/kcmodule.h

To: ervin, #frameworks, #plasma, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25071: Have KCModuleProxy take care of the defaulted state

[Kevin Ottens]

ervin updated this revision to Diff 69270.
ervin added a comment.


  Add missing @since markers

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25071?vs=69041=69270

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

AFFECTED FILES
  src/kcmoduleproxy.cpp
  src/kcmoduleproxy.h
  src/kcmoduleproxy_p.h

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25069: Adjust KCModule to also channel information about defaults

[Kevin Ottens]

ervin updated this revision to Diff 69269.
ervin added a comment.


  Add missing @since markers

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25069?vs=69039=69269

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

AFFECTED FILES
  src/kcmodule.cpp
  src/kcmodule.h

To: ervin, #frameworks, #plasma, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24743: Update GTK settings according to Plasma settings

[Kevin Ottens]

ervin added a comment.


  Just a minor thing, otherwise LGTM, I'll let Kai give the last stamp of 
approval.

INLINE COMMENTS

> gtkconfig.cpp:113
> +setFont();
> +setFont();
> +setIconTheme(KIconLoader::Group::Desktop);

Are you sure you want to call setFont() twice here? That looks surprising.

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

To: gikari, #plasma, #vdg
Cc: ervin, ngraham, broulik, nicolasfella, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, 
MrPepe, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, mglb, crozbo, ndavis, ZrenBot, firef, alexeymin, 
skadinna, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, 
abetts, sebas, apol, ahiemstra, mbohlender, mart

D25094: Fix reset to defaults in the fonts KCM

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:7b0149c79ea4: Fix reset to defaults in the fonts KCM 
(authored by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25094?vs=69118=69119

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

AFFECTED FILES
  kcms/fonts/fonts.cpp
  kcms/fonts/fonts.h

To: ervin, #plasma, bport, davidedmundson, mart
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25094: Fix reset to defaults in the fonts KCM

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, bport, davidedmundson, mart.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Turns out one of the properties was utterly broken emitting the wrong
  signal, and the defaults() method was missing a piece of state to reset.
  
  Since it was making my eyes bleed I also removed the "const int &"
  parameters in the process. :-)

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/fonts/fonts.cpp
  kcms/fonts/fonts.h

To: ervin, #plasma, bport, davidedmundson, mart
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24848: fix kcm fonts "typo" on connect

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:08a8914d7169: fix kcm fonts typo on connect 
(authored by bport, committed by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24848?vs=68500=69094

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

AFFECTED FILES
  kcms/fonts/fonts.cpp

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

D24743: Update GTK settings according to Plasma settings

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> configeditor.cpp:49
> +QString 
> configLocation(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation));
> +QString gtk3ConfigPath(configLocation + "/gtk-3.0/settings.ini");
> +

Better wrap the const char* in a QStringLiteral.

Also, but more of a nitpick this time: style wise I'd favor using = for those 
initialization (and others in the file) than parenthesis. It gets too close the 
most vexing parse territory, I'd rather not potentially expose a future 
developer touching that file to it.

> configeditor.cpp:81
> +{
> +QString gtkrcPath(QDir::homePath() + "/.gtkrc-2.0");
> +QFile gtkrc(gtkrcPath);

QStringLiteral please.

> configeditor.cpp:96
> +} else {
> +return "";
> +}

QString() or {} instead of ""

> configeditor.cpp:102
> +{
> +const QRegularExpression regExp(paramName + "=[^\n]*($|\n)");
> +

QStringLiteral please

There are more of those in the file, I'll stop pointing them out. ;-)

> configeditor.h:45
> +QString readFileContents(QFile ) const;
> +pid_t getPidOfXSettingsd() const;
> +};

We don't prefix getters with get.

> configvalueprovider.cpp:35
> +
> +KSharedConfigPtr 
> config(KSharedConfig::openConfig(QStringLiteral("kdeglobals")));
> +KConfigGroup configGroup(config->group(QStringLiteral("General")));

I will contradict my previous comment about making those functions static or 
turning into a namespace. I think it'd be better to avoid creating and ditching 
those KSharedConfig instances at each call, so why not instantiate it when 
ConfigValueProvider is created?

You might have to load() from the config at right point in times though.

> configvalueprovider.h:37
> +
> +QString getConfigFontName() const;
> +QString getConfigIconThemeName() const;

We don't prefix getters with get

Beside none of those have any state AFAICT, why not make them static or turn 
this into a namespace?

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

To: gikari, #plasma, #vdg
Cc: ervin, ngraham, broulik, nicolasfella, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, 
MrPepe, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, mglb, crozbo, ndavis, ZrenBot, firef, alexeymin, 
skadinna, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, 
abetts, sebas, apol, ahiemstra, mbohlender, mart

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  Looks good to me. Please just wait a bit before pushing to give David a 
chance to object to my comments. ;-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kpropertywriter_p.h:26
> Throwing out another option
> 
>   class KPropertyWriter : public QObject, public QQmlPropertyValueSource
>   {
>   Q_INVOKABLE bool writeProperty(QVariant value);
>   }
>   
>   writeProperty(QVariant) {
> object()->setProperty(name(), value());
> // we can't use property().write() as that'll break the binding
>   }
>   
>   
>   PropertyWriter on currentIndex {
>  id: controlRootWriter
>   }
> 
> Though it's basically the same thing, so don't feel you have to, just wanted 
> to share the suggestion as it reduces two properties.

Didn't think about that one, clever trick indeed. :-)

That being said I think it'd be more work in the end, indeed setTarget is pure 
virtual in there, and also I'm not sure that calling write() on QQmlProperty 
doesn't break bindings.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added a comment.


  In D25000#556761 , @davidedmundson 
wrote:
  
  > There's a Plasma rule that if we're working round a Qt bug, there should be 
a Qt bug created and linked before accepting a workaround.
  >
  > From the sounds of it we want a QQuickControls::ComboBox::setIndex(int) 
invokable that doesn't update the binding?
  >  Or is it a more generic problem of somehow exposing 
QQmlPropertyData::WriteFlags ?
  
  
  It's the former, not the latter. And good luck having setIndex invokable, it 
would never go through a Qt review IMHO (and for good reason). Now we need this 
really because our own combo style insists on emulating a corner case of 
QComboBox behavior, it's fairly unusual and specific.
  I don't think it warrants putting something in Qt directly.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

ervin added a comment.


  Once we got D25000  completed, please 
remember to abandon that one.

REPOSITORY
  R119 Plasma Desktop

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

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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  Almost there! Glad we're getting near a proper fix. Can you confirm this 
works with *and* without D24916  applied?
  Can you also confirm this doesn't break the KScreen KCM? (as mentioned in 
Kai's comment on D24916 ).
  
  I want to make sure we don't introduce a regression for those who had started 
to workaround that bug on their side.

INLINE COMMENTS

> kpropertywriter.h:1
> +/*
> + *   Copyright (C) 2019 Cyril Rossi 

This file should be named kpropertywriter_p.h since it's not installed and the 
class not exported from a library.

> kpropertywriter.h:39
> +
> +Q_INVOKABLE bool writeProperty(QVariant value);
> +

const QVariant &

> kpropertywriter.h:43
> +void setTarget(QObject *target);
> +void setPropertyName(QString propertyName);
> +

const QString &

> kpropertywriter.h:47
> +void targetChanged(QObject *target);
> +void propertyNameChanged(QString propertyName);
> +

const QString &

> kpropertywriter.h:50
> +private:
> +QObject *m_target = nullptr;
> +QString m_propertyName;

If you want to play like this, you could have had a "using QObject::QObject;" 
for the ctor declaration and have no ctor definition in the cpp file. ;-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D25070: Make KCModuleQml conform to the defaulted() signal

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kcmoduleqml.cpp:80-82
> needsSave emits the current state and then connects for changes
> representsDefaults  only connects for changes
> 
> I would expect them to match as they're doing equivalent things.
> 
> It looks to me that it's this line that's somewhat pointless - we're in a 
> constructor so only super classes could possibly have connected and we can 
> see it doesn't.
> Can you confirm.

Yes, I confirm this is pointless and why I didn't replicate that for defaulted.

REPOSITORY
  R295 KCMUtils

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

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25071: Have KCModuleProxy take care of the defaulted state

[Kevin Ottens]

ervin added a comment.


  In D25071#556681 , @davidedmundson 
wrote:
  
  > Edit: ah, I see why.
  >
  > KCMMultiDialog and system settings re-evaluate the buttons on receipt of 
the changed signal
  >  Still seems maybe a bit odd, but it makes sense in context.
  
  
  I don't disagree, it makes it look odd. I just didn't feel going for the even 
more intrusive route, I touched quite a few moving pieces already.

REPOSITORY
  R295 KCMUtils

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

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25077: Use compile time checked connect

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:92f6e85438f8: Use compile time checked connect (authored 
by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25077?vs=69051=69052

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

AFFECTED FILES
  src/quickaddons/managedconfigmodule.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson, bport
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25077: Use compile time checked connect

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Benjamin made me realized we went through introspection for nothing
  here.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/quickaddons/managedconfigmodule.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25071: Have KCModuleProxy take care of the defaulted state

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kcmoduleproxy.cpp:196-197
> Why emit changed?
> 
> Given KCModuleProxy mirrors KCModule API I would have expected a defaulted 
> signal to match?

Because all the consumers I found surprisingly don't care about the distinction 
or the value of that bool. Also it would have led to more intrusive changes in 
said consumers. I admit I was surprised about that.

REPOSITORY
  R295 KCMUtils

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

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25069: Adjust KCModule to also channel information about defaults

[Kevin Ottens]

ervin marked an inline comment as done.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, #frameworks, #plasma, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25068: Adjust QtQuickSettings KCM to use KConfigXT in full

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:61cd3d4b19a9: Adjust QtQuickSettings KCM to use KConfigXT 
in full (authored by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25068?vs=69038=69049

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

AFFECTED FILES
  kcms/qtquicksettings/kcmqtquicksettings.cpp
  kcms/qtquicksettings/kcmqtquicksettings.h
  kcms/qtquicksettings/kcmqtquicksettingswidget.ui
  kcms/qtquicksettings/renderer.kcfg

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

D25076: Force reevaluating state on pending deletion changes

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:35c84cb5f9d2: Force reevaluating state on pending 
deletion changes (authored by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25076?vs=69046=69050

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

AFFECTED FILES
  kcms/desktoptheme/kcm.cpp

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

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a comment.


  nitpick, but otherwise LGTM, giving time to others (in particular Nate) to 
chip in.

INLINE COMMENTS

> iconsmodel.cpp:87
>  }
> -
>  emit pendingDeletionsChanged();

I'd say keep the empty line please.

REPOSITORY
  R119 Plasma Desktop

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

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

D24846: Port kcm icons to kconfigxt

[Kevin Ottens]

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

INLINE COMMENTS

> main.cpp:101
>  
> -connect(m_model, ::selectedThemeChanged, this, [this] {
> -m_selectedThemeDirty = true;
> -setNeedsSave(true);
> -});
> -connect(m_model, ::pendingDeletionsChanged, this, [this] {
> -setNeedsSave(true);
> -});
> +connect(m_model, SIGNAL(selectedThemeChanged()), this, 
> SLOT(_k_settingsChanged()));
> +connect(m_model, SIGNAL(pendingDeletionsChanged()), this, 
> SLOT(_k_settingsChanged()));

Shouldn't you be able to remove that connect? I'd expect this signal trickling 
down to changing the settings object, wouldn't it?

> main.cpp:102
> +connect(m_model, SIGNAL(selectedThemeChanged()), this, 
> SLOT(_k_settingsChanged()));
> +connect(m_model, SIGNAL(pendingDeletionsChanged()), this, 
> SLOT(_k_settingsChanged()));
>  

Now that my ManagedConfigModule change landed, you should use a proper compile 
time check connect to settingsChanged here.

> main.cpp:110
>  {
> +foreach (KIconTheme* theme, m_kicon_theme_map) {
> +delete theme;

Don't do foreach. Instead write:

  for (auto theme : qAsConst(m_iconThemeCache)) {

Or even better, just use qDeleteAll:

  qDeleteAll(m_iconThemeCache)

Or even better yet : try to use std::unique_ptr, std::shared_ptr or 
QScopedPointer as values in your associative container (I let you check which 
one fits best).

> main.cpp:137
>  {
> -return m_iconSizes[group];
> -}
> -
> -void IconModule::setIconSize(int group, int size)
> -{
> -if (iconSize(group) == size) {
> -return;
> +QString themeName(m_model->selectedTheme());
> +if (!m_kicon_theme_map.contains(m_model->selectedTheme())) {

nitpick, I find = more readable in such a context (and less prone to the most 
vexing parse since you don't use curly braces init).

I'd write: `const auto themeName = m_model->selectedTheme();`

> main.cpp:138
> +QString themeName(m_model->selectedTheme());
> +if (!m_kicon_theme_map.contains(m_model->selectedTheme())) {
> +m_kicon_theme_map.insert(themeName, new KIconTheme(themeName));

Couldn't you fill the cache as soon as the selected theme changes instead? This 
way you wouldn't need to modify your cache here.

> main.h:34
>  #include 
> +#include 
>  

s/QMap/QHash/

> main.h:83
>  
> -Q_INVOKABLE int iconSize(int group) const;
> -Q_INVOKABLE void setIconSize(int group, int size);
> -Q_INVOKABLE QList availableIconSizes(int group) const;
> +Q_INVOKABLE QList availableIconSizes(int group);
>  

Killing the const here is semantically wrong IMO.

> main.h:117
>  
> -QVector m_iconSizes;
> +QMap m_kicon_theme_map;
>  

You don't need the keys to be sorted, so please use QHash instead.

Also we do camel case here, not snake case. :-)

> IconSizePopup.qml:81
>  
> +property var sizesMap: [
> +{"settingName":"desktopSize", "displayName": 
> i18n("Desktop")},

I think I'd have expected that logic on the C++ side actually. Others might 
disagree. :-)

> IconSizePopup.qml:91
> +function sizeDisplayName(index) {
> +index = index <0 ? 0 : index;
> +return iconTypeList.sizesMap[index].displayName;

Urgh, I'd assert than silently swallow that I think.

REPOSITORY
  R119 Plasma Desktop

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

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

D25073: Get KQuickAddons::ConfigModule to expose if we're in the defaults state

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:b2788c1a0894: Get KQuickAddons::ConfigModule to expose if 
were in the defaults state (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25073?vs=69043=69047

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

AFFECTED FILES
  src/quickaddons/configmodule.cpp
  src/quickaddons/configmodule.h
  src/quickaddons/managedconfigmodule.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25075: Make the settingChanged() slot protected.

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:372dbb493df6: Make the settingChanged() slot protected. 
(authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25075?vs=69045=69048

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

AFFECTED FILES
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, bport, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25068: Adjust QtQuickSettings KCM to use KConfigXT in full

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in renderer.kcfg:10
> Will this mean we write "automaticloop" to the config file?
> 
> If so, we'll need to change  the KQuickAddons::QtQuickSettings::init
> As it passes the variables in the config directly to Qt envs, which wouldn't 
> be ideal.

Nope. KConfigXT is so clever that if you write a value which is the default to 
it, it kills the key in the config file instead. :-)

This is why you always want KConfigXT btw, managing defaults is error prone, 
people forget to kill the key when they do readEntry/writeEntry manually... 
which means you got something in the config file when the default changes, so 
you don't see the new default.

REPOSITORY
  R119 Plasma Desktop

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

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

D25076: Force reevaluating state on pending deletion changes

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, bport, mart, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This was missing and thus the apply button wouldn't get enabled in case
  of a pending deletion. This is due to this mechanism being outside of
  the KConfigXT realm.

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/desktoptheme/kcm.cpp

To: ervin, #plasma, bport, mart, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25075: Make the settingChanged() slot protected.

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, bport, mart, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This might be necessary for some subclasses which have some of the
  settings state managed outside of KConfigXT. Now they can simply call or
  connect to the slot.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, bport, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25074: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, mart, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REPOSITORY
  R124 System Settings

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

AFFECTED FILES
  core/ModuleView.cpp

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

D25073: Get KQuickAddons::ConfigModule to expose if we're in the defaults state

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  ManagedConfigModule is adjusted as well to manage that new piece of
  state.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/quickaddons/configmodule.cpp
  src/quickaddons/configmodule.h
  src/quickaddons/managedconfigmodule.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25070: Make KCModuleQml conform to the defaulted() signal

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This new signal got introduced in KCModule, so we need to emit it at the
  right time depending on the ConfigModule state.
  
  This depends on: https://phabricator.kde.org/D25069

REPOSITORY
  R295 KCMUtils

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

AFFECTED FILES
  src/kcmoduleqml.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25072: Disable the restore defaults button if the KCModule says so

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REPOSITORY
  R295 KCMUtils

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

AFFECTED FILES
  src/kcmultidialog.cpp

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25071: Have KCModuleProxy take care of the defaulted state

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart, davidedmundson, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Now that KCModule exposes a defaulted() signal, consume it and keep our
  own defaulted state up to date. This is exposed for further consumption
  in the GUIs integrating KCModuleProxy.
  
  This depends on: https://phabricator.kde.org/D25069

REPOSITORY
  R295 KCMUtils

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

AFFECTED FILES
  src/kcmoduleproxy.cpp
  src/kcmoduleproxy.h
  src/kcmoduleproxy_p.h

To: ervin, #plasma, #frameworks, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25069: Adjust KCModule to also channel information about defaults

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Frameworks, Plasma, mart, davidedmundson, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  We'd probably do things differently nowadays (including the signal
  name...) but I tried to stay as close as possible to KCModule current
  structure.

REPOSITORY
  R265 KConfigWidgets

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

AFFECTED FILES
  src/kcmodule.cpp
  src/kcmodule.h

To: ervin, #frameworks, #plasma, mart, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25068: Adjust QtQuickSettings KCM to use KConfigXT in full

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, mart, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This removes quite a lot of uneeded code and fixes the automatic default
  handling from KConfigXT point of view (this was the wrong integration
  scheme with combo boxes).
  
  It also gives me a nice testing ground for a widget based KCM usable
  with kcmshell. :-)

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/qtquicksettings/kcmqtquicksettings.cpp
  kcms/qtquicksettings/kcmqtquicksettings.h
  kcms/qtquicksettings/kcmqtquicksettingswidget.ui
  kcms/qtquicksettings/renderer.kcfg

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

D25061: kconfig_compiler: Move the KSharedConfig::Ptr when using them

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  LGTM, maybe give some times to others to chip in.

REPOSITORY
  R237 KConfig

BRANCH
  master

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

To: aacid, ervin
Cc: kde-frameworks-devel, ervin, apol, LeGast00n, GB_2, michaelh, ngraham, bruns

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added a comment.


  Good start, there's one case still missing... which looks hard to reach, I 
wonder how to get there.

INLINE COMMENTS

> ComboBox.qml:92
>  if (indexUnderMouse > -1) {
>  controlRoot.currentIndex = indexUnderMouse;
>  controlRoot.activated(indexUnderMouse);

What about that one? :-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

ervin added a comment.


  Style wise, functions come before children objects. Other than that looks 
good to me.
  
  Through Kai comments, the question also becomes, wouldn't it be better to fix 
the root cause? It's likely we'll encounter that ComboBox breakage at other 
places.

REPOSITORY
  R119 Plasma Desktop

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

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

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

ervin added a comment.


  In D24916#553958 , @broulik wrote:
  
  > > Ah somehow I thought we had a third more "clever" case of currentIndex 
moving.
  >
  > Heh, yea, there's also some more elaborate "change index while moving the 
mouse" going on :(
  >  So best hack would be to have a c++ thing that sets the property without 
going through QML's binding system :D
  
  
  So we're back on the "but it's not going to be pretty" department. ;-)

REPOSITORY
  R119 Plasma Desktop

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

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

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

ervin added a comment.


  In D24916#553919 , @broulik wrote:
  
  > > AFAICT it won't be pretty though. :-)
  >
  > I think it's pretty straightforward
  >  1.) the delegate must not explicitly set a `currentIndex` because Qt does 
it for us, see D18299 
  >  2.) the wheel handler must be changed to call `decrementCurrentIndex()` 
and `incrementCurrentIndex()` (hopefully they wrap...) instead of changing the 
index manually, so it doesn't break the binding.
  
  
  Ah somehow I thought we had a third more "clever" case of currentIndex 
moving. Then indeed if that's just those two we should be fine.

REPOSITORY
  R119 Plasma Desktop

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

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

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

ervin added a comment.


  In D24916#553886 , @broulik wrote:
  
  > Hm I think qqc2-desktop-style breaks the binding on `currentIndex`, 
otherwise this would not be neccessary...
  
  
  Good point... What about fixing that? AFAICT it won't be pretty though. :-)

REPOSITORY
  R119 Plasma Desktop

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

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

D24917: KCM Style port to KConfigXT

[Kevin Ottens]

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

INLINE COMMENTS

> kcmstyle.cpp:317
> +
> +m_model->setSelectedStyle(m_settings->widgetStyle());
>  

That line and the metaenum wrangling could likely be factored out somehow and 
shared with load(). Especially the enum part is uncommon enough that we 
probably want to hide it behind a nicer name (also would help a bit having a 
more consistent level of abstraction in the code of those functions).

> kcmstyle.h:48
>  Q_PROPERTY(StylesModel *model READ model CONSTANT)
> -
> -Q_PROPERTY(bool iconsOnButtons READ iconsOnButtons WRITE 
> setIconsOnButtons NOTIFY iconsOnButtonsChanged)
> -Q_PROPERTY(bool iconsInMenus READ iconsInMenus WRITE setIconsInMenus 
> NOTIFY iconsInMenusChanged)
> +Q_PROPERTY(StyleSettings* styleSettings READ styleSettings CONSTANT)
>  Q_PROPERTY(ToolBarStyle mainToolBarStyle READ mainToolBarStyle WRITE 
> setMainToolBarStyle NOTIFY mainToolBarStyleChanged)

Please have the space before the * and not after to follow the style of that 
file (see the Q_PROPERTY just above).

> kcmstyle.h:66
>  
> -bool iconsOnButtons() const;
> -void setIconsOnButtons(bool enable);
> -Q_SIGNAL void iconsOnButtonsChanged();
> -
> -bool iconsInMenus() const;
> -void setIconsInMenus(bool enable);
> -Q_SIGNAL void iconsInMenusChanged();
> +StyleSettings* styleSettings() const;
>  

ditto

REPOSITORY
  R119 Plasma Desktop

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

To: crossi, ervin, mart, bport, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24916: KCM style : fix combobox that weren't updated after user made change

[Kevin Ottens]

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

INLINE COMMENTS

> EffectSettingsPopup.qml:72
> +onMainToolBarStyleChanged: {
> +mainToolBarStyleCombo.currentIndex = 
> mainToolBarStyleCombo.model.findIndex(function (item) {
> +return item.value === kcm.mainToolBarStyle

To de-duplicate a bit, because now that's 4 times (almost) the same piece of 
code. What about having a function resetIndex() declared in that combo and 
another one in the other combobox which would do the currentIndex + findIndex 
dance.

This way in both the code would become in both cases:

  currentIndex: resetIndex()
  ...
  onFooStyleChanged: fooStyleCombo.resetIndex()

REPOSITORY
  R119 Plasma Desktop

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

To: crossi, #plasma, ervin, mart, bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24846: Port kcm icons to kconfigxt

[Kevin Ottens]

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

INLINE COMMENTS

> broulik wrote in main.cpp:141
> Does this `KIconTheme` instance need caching? I recall creating those parses 
> a tonne of files and directories and is quite slow

Agreed, it'll hit the disk quite a bit, this might be a problem in a function 
which takes part in bindings (more than one which will change around the same 
time).

> main.cpp:176
> +{
> +   return m_model->selectedTheme() != KIconTheme::current();
> +}

Indentation looks wrong here.

> bport wrote in IconSizePopup.qml:47-53
> I wiil look if I can simplify that

I like the intent of trying to have names instead of just indices everywhere... 
but that might not be feasible without extra complexity we'll want to avoid 
indeed.

I think the more fundamental problem is that the settings are based on those 
names, while the invokables (most notably availableIconSizes) are based on 
indices. Probably something to address, like have names everywhere and just a 
function to map from index to name in the iconTypeList.

REPOSITORY
  R119 Plasma Desktop

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

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

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a comment.


  In D24847#552665 , @bport wrote:
  
  > I will update this patch to allow to delete current selected theme but 
remove bug
  >  So if we have A B C, A selected
  >
  > 1. we delete B : B pending deletion A still selected
  > 2. we delete A : A and B pending deletion C selected
  >
  >   What happen if we delete the last one ?
  
  
  It's a bit theoretical in practice you always have at least one coming from 
the system which can't be deleted. Anyway, assuming you'd end up in such a 
situation: if it's the last one I'd say: don't allow deletion in the first 
place.

REPOSITORY
  R119 Plasma Desktop

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

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

Re: D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

Hello,


On Tuesday, 22 October 2019 20:17:15 CEST Nathaniel Graham wrote:
>   Does this patch have unmarked dependencies? It doesn't apply for me:

I'd expect it also needs D24845 and D24846. They got all submitted together.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a subscriber: bport.
ervin added a comment.


  Hello,
  
  - F7654931: signature.asc 

REPOSITORY
  R119 Plasma Desktop

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

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

D24822: Port the desktoptheme kcm to ManagedConfigModule

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:331c25893d06: Port the desktoptheme kcm to 
ManagedConfigModule (authored by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24822?vs=68409=68547

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

AFFECTED FILES
  kcms/desktoptheme/kcm.cpp
  kcms/desktoptheme/kcm.h

To: ervin, mart, bport, crossi, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24821: Add ManagedConfigModule

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:c8b95b07c53b: Add ManagedConfigModule (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68530=68545

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin updated this revision to Diff 68530.
ervin added a comment.


  Address David's comments

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68411=68530

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin marked an inline comment as done.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin marked 4 inline comments as done.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24848: fix kcm fonts "typo" on connect

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.


  In D24848#551863 , @broulik wrote:
  
  > Interesting, I thought Qt would/could static_assert that
  
  
  Well, it turns out the signatures were compatible in fact (getters connected 
to a function which takes no parameter, signatures are fine). But since it is 
done by the C++ compiler, this one has no idea if the second parameter of the 
connect is indeed a signal (this is a completely alien concept to it). This is 
the one blind spot of the new style connects.

REPOSITORY
  R119 Plasma Desktop

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

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

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin added a comment.


  In D24821#551179 , @davidedmundson 
wrote:
  
  > Nice!
  >
  > I'm not super sold on magically doing findChildren to get the config 
skeletons over an explicit
  >  registerSettings(KCoreConfigSkeleton*).
  >
  > I'm not against it either, but could you expand on the rationale.
  
  
  You mean here to sell it to you or in the doc or something else?
  
  Indeed the alternative is registerSettings(), I went for the less explicit 
"children settings" mechanism mostly because it was closer to the older system 
around KCModule which was very much working by convention.

INLINE COMMENTS

> bport wrote in managedconfigmodule.h:210
> I think we need to set this value to true by default, because if we don't 
> override it we assume value are not the default one

Right, makes sense for the future code.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin updated this revision to Diff 68411.
ervin added a comment.


  Addresses bport's comment

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68408=68411

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport
Cc: bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24821: Add ManagedConfigModule

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This is a new type of ConfigModule which will manage the state of
  "apply" and "defaults" by itself depending on the state of the KConfigXT
  objects used within the module.
  
  This is sligthly uncomplete on the "defaults" side due to missing
  facilities in ConfigModule, but at least it shows the approach works.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24822: Port the desktoptheme kcm to ManagedConfigModule

[Kevin Ottens]

ervin created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This depends on https://phabricator.kde.org/D24821

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/desktoptheme/kcm.cpp
  kcms/desktoptheme/kcm.h

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