KConfigDialogManager: get change signal from metaObject or special property
ClosedPublic

Authored by kossebau on Jan 13 2017, 9:24 PM.

Details

Summary

KConfigDialogManager was possibly designed when there was no way to
access the change signal of properties. So maintaining the signals
and properties to use in separate global user-accessable tables might
have been done as there was no other option.

But this has flaws:

  • different libs & plugins might do conflicting global settings
  • prevents per-widget-instance settings (for signals)
  • requires explicit setup even for signals already set via NOTIFY on the property

With modern Qt metaobject API instead the change signal can be
fetched and used automatically. And if needing a custom signal, e.g. for
classes without a notify signal set on the property, that can be set via a
special new dynamic property "kcfg_propertyNotify", like it is done
to select a custom property for a widget instance.

For backward-compatibility in KF5 those will be only a new options,
the global maps should be removed on next ABI breakage.

Test Plan

Old and new tests do not fail.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau updated this revision to Diff 10158.Jan 13 2017, 9:24 PM
kossebau retitled this revision from to Let KConfigDialogManager get a property's change signal from the metaObject.
kossebau updated this object.
kossebau edited the test plan for this revision. (Show Details)
kossebau added a reviewer: Frameworks.
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 13 2017, 9:24 PM

I see a problem with this patch, it seems it breaks custom widgets that are using properties without a NOTIFY signal.

In Ark we are using a plain QTreeWidget in the config dialog, and we have the following code:

// Set the custom property that KConfigDialogManager will use to update the settings.
kcfg_disabledPlugins->setProperty("kcfg_property", QByteArray("disabledPlugins"));
// Tell KConfigDialogManager to monitor the itemChanged signal for a QTreeWidget instance in the dialog.
KConfigDialogManager::changedMap()->insert(QString::fromLatin1(QTreeWidget::staticMetaObject.className()),
                                           SIGNAL(itemChanged(QTreeWidgetItem*,int)));

If I remove the changedMap() line in ark and apply your patch, the Apply button in the config dialog is no longer enabled whenever the items in the QTreeWidget change.

This could be solved by subclassing QTreeWidget, but that would be annoying :p

kossebau added a comment.EditedJan 14 2017, 11:40 PM

Right, I forgot I wanted to check all the hits in lxr.kde.org first, but seems I called arc too early.
And checking the QWidgets also should have been done, at least QCalendarWidget also has no NOTIFY on interesting properties. Meh.

So what about this

To fix the issue with possibly conflicting global settings of the property to use via the propertyMap() or indirectly the "kcfg_property" (which then is also stored into that global property map), we introduce a new additional way to control per widget instance what property to use (if not the USER one). One would store the name in the dynamic property "kconfig_property". Or alternatively "kcfg_property" could be kept used, and another dynamic property would mark that this one is only for this widget, e.g. "kcfg_propertyForWidget" = true.
The second variant might be nicer, as it allows simply forward compatibility, so existing code could start adding that property for all the widget instances, would still work with old KF versions and for KF >=5.31 it would start working as intended, and no longer result in possibly conflicting registrations (none known to me, but there is a chance with apps with many settings from plugins, like System Settings, KDevelop, etc.).
For KF6, where the global maps would be dropped, then the "kcfg_propertyForWidget" would be useless, but there could be a warning check in debug builds pointing out to remove this then deprecated flag from ones code.

Update: I was confused when I wrote that. "kcfg_property" is not stored into that global property map, it is already only valid for widget it is set on. So no need to add new property keys for selecting the value property of the widget. The only things needed is to deprecate the use of propertyMap(), to avoid conflicting settings by independent code.

And together with that there would be another new property "kconfig_propertyNotify" or similar, via which for a given widget instance the signal to use would be passed (and only for this instance, not all of the class type). So with Ark's code it would be:

// Set the custom property that KConfigDialogManager will use to update the settings.
kcfg_disabledPlugins->setProperty("kcfg_property", QByteArray("disabledPlugins"));
// Tell KConfigDialogManager to monitor the itemChanged signal for a QTreeWidget instance in the dialog.
kcfg_disabledPlugins->setProperty("kcfg_propertyNotify", SIGNAL(itemChanged(QTreeWidgetItem*,int)));

What do you think?

@kossebau That looks good, way more easier :)

kossebau updated this revision to Diff 10262.Jan 17 2017, 12:41 AM

Add option to set signal via kcfg_propertyNotify

kossebau updated this revision to Diff 10263.Jan 17 2017, 1:02 AM

update KF6 comment

kossebau retitled this revision from Let KConfigDialogManager get a property's change signal from the metaObject to KConfigDialogManager: get change signal from metaObject or special property.Jan 17 2017, 2:49 PM
kossebau updated this object.
kossebau updated this revision to Diff 10285.Jan 17 2017, 3:34 PM

small fix in unit test

kossebau updated this revision to Diff 10874.Feb 2 2017, 9:03 PM

rebase to latest msater, also prepare for rather KF 5.32

If no-one objects or has further comments, I would commit this soon after 5.31 release.

This revision was automatically updated to reflect the committed changes.