D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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

INLINE COMMENTS

> autostart.cpp:167
>  {
> -KOpenWithDialog owdlg( this );
> -if (owdlg.exec() != QDialog::Accepted)
> -return;
> +KOpenWithDialog* owdlg = new KOpenWithDialog(this);
> +connect(owdlg, ::finished, this, [this, owdlg] (int result) {

Space before * not after, or just use auto

> autostart.cpp:186
>  {
> -AddScriptDialog * addDialog = new AddScriptDialog(this);
> -int result = addDialog->exec();
> -if (result == QDialog::Accepted) {
> -if (addDialog->symLink())
> -KIO::link(addDialog->importUrl(), 
> QUrl::fromLocalFile(m_paths[0]));
> -else
> -KIO::copy(addDialog->importUrl(), 
> QUrl::fromLocalFile(m_paths[0]));
> -
> -ScriptStartItem * item = new ScriptStartItem( m_paths[0] + 
> addDialog->importUrl().fileName(), m_scriptItem,this );
> -addItem( item,  addDialog->importUrl().fileName(), 
> addDialog->importUrl().fileName(),ScriptStartItem::START );
> -}
> -delete addDialog;
> +AddScriptDialog* addDialog = new AddScriptDialog(this);
> +connect(addDialog, ::finished, this, [this, addDialog] (int 
> result) {

ditto

> autostart.cpp:214
>  {
> +Q_UNUSED(parent)
> +Q_UNUSED(last)

Actually I think I'd Q_ASSERT(!parent.isValid())

> autostart.cpp:215
> +Q_UNUSED(parent)
> +Q_UNUSED(last)
>  

This Q_UNUSED is now used ;-)

> autostart.cpp:276
> +
> +KPropertiesDialog* dlg = new KPropertiesDialog( kfi, this );
> +connect(dlg, ::finished, this, [this, index, fileName, 
> desktopItem, dlg] (int result) {

Space before * not after, or use auto
Also drop the spaces between the parentheses

> autostart.cpp:296
>  {
> -if ( widget->listCMD->currentItem() == nullptr )
> +if ( widget->listCMD->currentItem() == nullptr ) {
>  return;

Since you touched the line, please drop the spaces between the parentheses

> autostart.cpp:304
>  {
> -if ( widget->listCMD->currentItem() == nullptr )
> +if ( widget->listCMD->currentItem() == nullptr ) {
>  return;

Since you touched the line, please drop the spaces between the parentheses

> autostart.cpp:321
> +
> +QModelIndex Autostart::indexFromWidget(QTreeWidgetItem* widget) const {
> +int index = m_programItem->indexOfChild(widget);

Since you touched the line please fix the * and { positions

> ervin wrote in autostart.h:64
> Still needs being addressed

Still needs being addressed. :-)

> autostartmodel.cpp:130
> +
> +AutostartModel::AutostartModel(QWidget* parent)
> +: QAbstractListModel(parent)

Space before * not after

> ervin wrote in autostartmodel.cpp:177
> Makes me realize: are we sure it will always be the first entry? I'm not sure 
> we can guarantee that over time.

Still needs being addressed.

> autostartmodel.h:49
> +public:
> +explicit AutostartModel(QWidget* parent = nullptr);
> +

Since you touched the line, please drop the spaces between the parentheses

> autostartmodel.h:81
> +QVector m_entries;
> +QWidget* m_window;
> +};

Space before * not after

REPOSITORY
  R119 Plasma Desktop

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

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

D28462: [KCM] Add color state probe to allow system settings to display current default state

[Kevin Ottens]

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

INLINE COMMENTS

> colorsstateprobe.cpp:29
> +: KCModuleStateProbe(parent, args)
> +, m_settings(new ColorsSettings(this))
> +{

I'm not sure I see the point of doing this... You don't need that member 
variable anyway. You could just have in the ctor body the following:
`registerSetting(new ColorsSettings(this));`

REPOSITORY
  R119 Plasma Desktop

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

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

D28461: In sidebar mode show if a module is in default state or not

[Kevin Ottens]

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

INLINE COMMENTS

> MenuItem.h:160
> +
> +void updateDefault();
> +

I don't like the name much, I think it could be confused with updating actual 
default value or such... updateIsDefault() is not great either though... :-/

> SidebarMode.cpp:117
> +{
> +QHash roleNames;
> +roleNames.insert(Qt::DisplayRole, "display");

This is generally implemented by calling the roleNames() of the parent class 
and then tune the returned hash. This way you ensure you keep the support for 
the standard roles. In your case that'd give something like:

  auto roleNames = QStandardItemModel::roleNames();
  roleNames.insert(MenuModel::IsDefaultRole, "default"); // yeah... QML doesn't 
have the isFoo convention, go figure
  return roleNames;

> SidebarMode.cpp:588
> +QModelIndex categoryIdx = 
> d->categorizedModel->index(d->activeCategoryRow, 0);
> +auto item = categoryIdx.data(Qt::UserRole).value();
> +// If subcategory exist update from subcategory

I'd feel better with a Q_ASSERT(item)

> SidebarMode.cpp:590
> +// If subcategory exist update from subcategory
> +if (!item->children().empty()) {
> +item = item->child(d->activeSubCategoryRow);

Technically correct, we tend to favor the use of `isEmpty()` though.

> CategoriesPage.qml:206
> +id: defaultMarker
> +radius: width*0.5
> +width: Kirigami.Units.largeSpacing

Missing spaces around *

> SubCategoryPage.qml:195
> +
> +Rectangle {
> +id: defaultMarker

This is twice the same Rectangle item, what about we make a reusable element 
and use it at both places to reduce code duplication?

> SubCategoryPage.qml:197
> +id: defaultMarker
> +radius: width*0.5
> +width: Kirigami.Units.largeSpacing

Spaces missing around *

REPOSITORY
  R124 System Settings

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

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

D28460: Add KCModuleStateProbe as base class for plugin

[Kevin Ottens]

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

INLINE COMMENTS

> kcmoduleloader.cpp:161
> +if (!mod.service() || mod.service()->noDisplay() || 
> mod.library().isEmpty())
> +{
> +return true;

Curly brace should be on the previous line

> kcmoduleloader.cpp:167
> +args2.reserve(args.count());
> +for (const QString  : args) {
> +args2 << arg;

Wouldn't it be better to initialize args2 with arg iterators?
i.e. `QVariantList args2(arg.cbegin(), arg.cend());`

> kcmodulestateprobe.cpp:47
> +
> +KCModuleStateProbe::~KCModuleStateProbe() {
> +delete d;

Curly brace on the next line

> kcmodulestateprobe.cpp:55
> +
> +void KCModuleStateProbe::registerSkeleton(KCoreConfigSkeleton *skeleton) {
> +if (!skeleton || d->_skeletons.contains(skeleton)) {

Curly brace on the next line

> kcmodulestateprobe.h:44
> +
> +virtual void virtual_hook(int id, void *data);
> +

Should be protected not public

> broulik wrote in kcmodulestateprobe.h:39
> Please add a `virtual_hook` so we can extend this class in the future without 
> breaking ABI should we have the need to extract more data out of a settings 
> module:
> 
>   virtual void virtual_hook(int id, void *data)

I'd slightly disagree here though, if that inherits from QObject anyway I'd 
just rely on meta call dispatching. But OK, let's go virtual_hook.

REPOSITORY
  R295 KCMUtils

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

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

D28331: KCM/mouse KCM/touchpad: Add a Scroll speed setting for wayland

[Kevin Ottens]

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


  LGTM

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D28331

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

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

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin added a comment.


  LGTM but @dfaure concerns need to be addressed.

REPOSITORY
  R237 KConfig

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

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

D28128: Add force save behavior to KEntryMap

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

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

To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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

INLINE COMMENTS

> broulik wrote in autostart.cpp:182
> Yes.
> It then also needs to set a transient parent and window modality manual I 
> think

Yes, otherwise you wouldn't get a modal dialog.

> meven wrote in autostart.cpp:231
> ?

Yes, similar situation to rowInserted here.

> broulik wrote in autostart.cpp:241
> Ok

Yes, it used to also inherit QObject (which wasn't a great idea) but not 
anymore.

> autostart.cpp:260
> +if (desktopItem)
>  {
> +const QModelIndex index = indexFromWidget(ent);

This curly brace needs to be on the previous line.

> broulik wrote in autostart.cpp:102
> Coding style:
> 
>   if (enabled) {
>   ...
>   }

I think Kai's point was also that the curly braces are required for the if and 
the else

> broulik wrote in autostart.cpp:210
> rows inserted gives you a range of [first,last] which was added

Good point, this still needs to be addressed.

> broulik wrote in autostart.h:64
> Capitalize `Changed`

Still needs being addressed

> autostartmodel.cpp:177
> +// add scripts, skips first value of listPath
> +// .config/autostart that is not compatible with scripts
> +for (int i = 1; i < listPath().length(); i++) {

Makes me realize: are we sure it will always be the first entry? I'm not sure 
we can guarantee that over time.

REPOSITORY
  R119 Plasma Desktop

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in autostartmodel.h:52
> I see, I meant my code to have all Roles used explicitly declared here rather 
> than relying on the developer knowing about Qt::DisplayRole.

Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making your 
own model. :-)

REPOSITORY
  R119 Plasma Desktop

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

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

Re: New Framework Review: KDAV

[Kevin Ottens]

Hello,

On Saturday, 9 November 2019 12:33:54 CEST Volker Krause wrote:
> during Akademy there was a request to promote KDAV from KDE PIM to
> Frameworks for use by Plasma Mobile. KDAV is a framework that implements
> the CalDav/ CardDav/GroupDav protocol on top of KIO's WebDav support. It
> would be classified as a functional tier 3 framework.
> 
> So far we have fixed a number of obvious ABI-compatibility issues, removed
> QtXml[Patterns] usage from the public interface and relicensed GPL parts
> (apart from a bit of test code) to LGPL. The next step would be a more
> thorough review to identify changes necessary before becoming a Framework.
> 
> To avoid the last minute invasive changes we ended up doing for
> KCalendarCore, I'd propose the following timeline:
> 
> - identify and implement all necessary changes to the API and ABI until the
> 20.04 Application release (that includes the still necessary move to the KF5
> library namespace).

I'm likely late to the party, but here is what I found by looking at KDAV 
master today (first day of the KDE PIM sprint):
 * There's a few private methods or Q_SLOTS that I'd hide completely by moving 
them to the d-pointer, for the slots we're using type safe connects so they 
don't even need to be marked as slots at all;
 * Is it worth making DavCollection moveable? It's only copyable right now;
 * We might want to do something about "ctag" in DavCollection it's a bit 
obscure as a name (and the API doc doesn't help), also it seems to not be an 
official standard (while being widely supported) and there's the sync-token 
mechanism which has a RFC (RFC6578);
 * Why isn't DavCollectionModifyJob using DavCollection somehow? (might just 
be my ignorance but I find it surprising that it is solely based on a property 
mechanism);
 * DavCollections(Multi)FetchJob has a mysterious "protocol" parameter on its 
collectionDiscovered signal, is it really necessary? if it has to stay, 
shouldn't be at least documented? or at least a safer type than int?
 * DavCollectionsMultiFetchJob is inconsistent as it's not using 
Q_DECLARE_PRIVATE;
 * KDAV::Error would benefit from more apidox;
 * Is it worth making DavItem moveable? It's only copyable right now;
 * Same comment about etag for DavItem than the ctag one for DavCollection
 * I'd be tempted to move all the protected methods of DavJobBase on its d-
pointer, the job subclasses would have access to them anyway, it'd make sense 
to put them protected in the header only if we expect subclasses outside of 
the lib (and I doubt this is actually supported);
 * It needs to decide between Qt smart pointers or STL ones I think, found a 
bit of both so far (I'd lean toward STL ones but maybe that's just me);
 * Make DavUrl moveable?
 * EtagCache probably shouldn't have anything protected, also, why is it a 
QObject at all?
 * Are we sure we want to return a QLatin1String in ProtocolInfo? this strike 
me as an odd choice.

Overall apidox would likely need a big pass of cleanups as well.

I think that's it from me.

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.

Re: New Framework Review: KDAV

[Kevin Ottens]

Hello,

On Saturday, 9 November 2019 12:33:54 CEST Volker Krause wrote:
> during Akademy there was a request to promote KDAV from KDE PIM to
> Frameworks for use by Plasma Mobile. KDAV is a framework that implements
> the CalDav/ CardDav/GroupDav protocol on top of KIO's WebDav support. It
> would be classified as a functional tier 3 framework.
> 
> So far we have fixed a number of obvious ABI-compatibility issues, removed
> QtXml[Patterns] usage from the public interface and relicensed GPL parts
> (apart from a bit of test code) to LGPL. The next step would be a more
> thorough review to identify changes necessary before becoming a Framework.
> 
> To avoid the last minute invasive changes we ended up doing for
> KCalendarCore, I'd propose the following timeline:
> 
> - identify and implement all necessary changes to the API and ABI until the
> 20.04 Application release (that includes the still necessary move to the KF5
> library namespace).

I'm likely late to the party, but here is what I found by looking at KDAV 
master today (first day of the KDE PIM sprint):
 * There's a few private methods or Q_SLOTS that I'd hide completely by moving 
them to the d-pointer, for the slots we're using type safe connects so they 
don't even need to be marked as slots at all;
 * Is it worth making DavCollection moveable? It's only copyable right now;
 * We might want to do something about "ctag" in DavCollection it's a bit 
obscure as a name (and the API doc doesn't help), also it seems to not be an 
official standard (while being widely supported) and there's the sync-token 
mechanism which has a RFC (RFC6578);
 * Why isn't DavCollectionModifyJob using DavCollection somehow? (might just 
be my ignorance but I find it surprising that it is solely based on a property 
mechanism);
 * DavCollections(Multi)FetchJob has a mysterious "protocol" parameter on its 
collectionDiscovered signal, is it really necessary? if it has to stay, 
shouldn't be at least documented? or at least a safer type than int?
 * DavCollectionsMultiFetchJob is inconsistent as it's not using 
Q_DECLARE_PRIVATE;
 * KDAV::Error would benefit from more apidox;
 * Is it worth making DavItem moveable? It's only copyable right now;
 * Same comment about etag for DavItem than the ctag one for DavCollection
 * I'd be tempted to move all the protected methods of DavJobBase on its d-
pointer, the job subclasses would have access to them anyway, it'd make sense 
to put them protected in the header only if we expect subclasses outside of 
the lib (and I doubt this is actually supported);
 * It needs to decide between Qt smart pointers or STL ones I think, found a 
bit of both so far (I'd lean toward STL ones but maybe that's just me);
 * Make DavUrl moveable?
 * EtagCache probably shouldn't have anything protected, also, why is it a 
QObject at all?
 * Are we sure we want to return a QLatin1String in ProtocolInfo? this strike 
me as an odd choice.

Overall apidox would likely need a big pass of cleanups as well.

I think that's it from me.

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.

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

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


  As advised by Kai and David on D27840 , 
switch to using tool buttons and fix RTL handling.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=78468=78703

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a reviewer: broulik.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin updated this revision to Diff 78698.
ervin marked 10 inline comments as done.
ervin added a comment.


  Addresses Kai and David comments.

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=78469=78698

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

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

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin marked 15 inline comments as done.
ervin added inline comments.

INLINE COMMENTS

> broulik wrote in SettingStateBinding.qml:58
> This description didn't really help me in understanding what it does until I 
> read the code further down.

Went for something much more verbose, but at least it says what it does.

> broulik wrote in SettingStateBinding.qml:75
> Can you explain this.
> Can't you
> 
>   indicatorComponent.createObject(target, {
>   settingsState: settingsState
>   });

Good point.

> broulik wrote in SettingStateBinding.qml:91
> We could also bind `parent: target` instead?

Tried it and QQuickItem was very angry at me, I didn't push further. :-)

> broulik wrote in SettingStateBinding.qml:97
> Please check if this works with right-to-left languages (run with `-reverse` 
> argument to test)

Again that's one of those comments I'd have appreciated on the widgets version 
a little while ago. ;-)

> davidedmundson wrote in SettingStateIndicator.qml:30
> This is a faux-button.
> 
> Which means it needs all the relevant:
> Accessible.blah
> 
> roles to be manually added
> 
> Should it be in the tab focus chain?
> 
> (or we could just inherit ToolButton)

Note this is the case for the QtWidgets implementation as well. I switched to 
ToolButton here but I likely need to do something similar in my other patch... 
would have been nice to have this comment earlier there (it's been ready a bit 
longer), you guys have a bias against widgets based patches. ;-)

I think it shouldn't temper with focus at all though, this is almost impossible 
to get correct in that context I think, so it'll simply reject focus (not too 
much of an issue in that context IMHO).

> broulik wrote in SettingStateIndicator.qml:40
> Perhaps bind to `icon.valid`?

Done completely differently with ToolButton anyway.

> broulik wrote in SettingStateIndicator.qml:48
> You could make the `root` `Item` a `MouseArea` instead.
> Also, I think this should get some kind of hover and/or pressed indication?
> Also, what about `Accessible` and keyboard focus? Should this be a proper 
> `ToolButton` control instead?

See my reply to @davidedmundson above, I switched to ToolButton.

> broulik wrote in settingstatebindingprivate.cpp:30
> Odd. Wouldn't we just get `QQuickRowLayout` et al? Or is that if you do a 
> custom item with a `Layout` as a base?

Or if you make something like Kirigami.FormLayout which has Item as base... 
obviously it's mostly an heuristic so can't be 100% perfect.

> broulik wrote in settingstatebindingprivate.cpp:82
> Too bad `QQuickItemChangeListener` is private.
> 
> Also, does any of this need event compression?

That really didn't feel like it required event compression to be honest at 
least I couldn't perceive any visible impact.

> broulik wrote in settingstatebindingprivate.cpp:90
> Check if it actually changed before emitting

Well it will have changed in 90% of the cases and checking means doing the 
computation anyway, which will happen when the getter is called. We'd pay the 
price twice.

An option is of course to cache the result of the getter to the price of higher 
complexity. Again I didn't perceive any visible impact on use. Should I go for 
it anyway?

Note I'm not at all denying this implementation is in some way inefficient, I'm 
just pointing out the trade off in term of readability and maintainability of 
the system.

> broulik wrote in settingstatebindingprivate.cpp:107
> Check if it actually changed before emitting

Same answer than the previous such case.

REPOSITORY
  R296 KDeclarative

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

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

D28331: KCM/mouse KCM/touchpad: Add a Scroll speed setting for wayland

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> main.qml:333
> +15,
> +20];
> +

nitpick: I'd put the closing square bracket on the next line and you can drop 
the semicolumn

> main.qml:343
> +value = index;
> +}
> +

You can drop the semicolumn in there

> main.qml:346
> +onMoved: {
> +device.scrollFactor = values[value];
> +root.changeSignal()

ditto

> touchpad.qml:631
> +15,
> +20];
> +

Same comment applies than for mouse

> touchpad.qml:638
> +if (index === -1) {
> +index = values.indexOf(1);
> +}

ditto

> touchpad.qml:644
> +onMoved: {
> +touchpad.scrollFactor = values[value];
> +root.changeSignal()

ditto

REPOSITORY
  R119 Plasma Desktop

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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


  Damn turns out I still missed a few, so what @broulik says (when it still 
applies).
  
  @broulik : are you sure about all your comments? It looks like something odd 
happened, some of you comments seems to target and older version of the patch 
or not being on the right line for some reason.

REPOSITORY
  R119 Plasma Desktop

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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


  Thanks for your patience :-)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  D26934

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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


  One last nitpick

INLINE COMMENTS

> autostartmodel.cpp:121
> +case PlasmaStart:
> +return static_cast (index);
> +default:

Please remove the spurious space before (

REPOSITORY
  R119 Plasma Desktop

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

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

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.

INLINE COMMENTS

> kconfigskeletontest.cpp:25
>  
> +static inline QString kdeGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/kdeglobals";

{ should be on its own line

> kconfigskeletontest.cpp:29
> +
> +static inline QString kdeSystemGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/system.kdeglobals";

ditto

> kconfigskeletontest.cpp:190
> +{
> +// This test ensure we don't override default value when this one came 
> from a file
> +// system.kdeglobals is a global file and can provide default

s/ensure/ensures/

> kconfigskeletontest.cpp:218
> +QCOMPARE(item->value(), 30);
> +}

More comments welcome in that test as well.

> kcoreconfigskeleton.cpp:13
>  #include 
> +#include 
>  

Looks like this include is unused

REPOSITORY
  R237 KConfig

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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

INLINE COMMENTS

> autostartitem.cpp:60
> +m_comboBoxStartup->addItem( AutostartModel::listPathName()[1], 
> AutostartEntrySource::PlasmaShutdown);
> +m_comboBoxStartup->addItem( AutostartModel::listPathName()[2], 
> AutostartEntrySource::PlasmaStart);
> +

Remove the space after the opening parenthesis on those three lines.

It's much better like this BTW, the loop wasn't indeed buying us much for just 
three values.

> autostartmodel.cpp:116
> +{
> +switch (index) {
> +case XdgAutoStart: return XdgAutoStart;

Heh, so we're playing who's the most stubborn here? I still find that "switch 
not saying it's a static_cast" really ugly and non idiomatic.

> autostartmodel.cpp:122
> +}
> +Q_UNREACHABLE();
> +return XdgAutoStart;

I'd still advise getting rid of the switch but if it doesn't disappear at least 
move that to the "default:" case to silence potential warning on type's non 
exhaustion.

But really... static_cast needs to appear please. Alternatively to the if I was 
proposing you could also do:

  switch (index) {
  case XdgAutoStart:
  case XdgScripts:
  case PlasmaShutdown:
  case PlasmaStart:
  return static_cast(index);
  default:
  Q_UNREACHABLE();
  }
  
  return XdgAutoStart;

At least it'd be DRY and it'd be more obvious we got a cast...

REPOSITORY
  R119 Plasma Desktop

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

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

D28128: Add force save behavior to KEntryMap

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> kconfigtest.cpp:1953
> +
> +void KConfigTest::testKdeglobalsVSDefault()
> +{

Seeing how this test confuses everyone (including me and I knew the problem 
before hand...), I think it'd benefit greatly from getting comments at the most 
important points in its execution (similarly to other tests in that file).

> kconfigtest.h:76
>  
> +void testKdeglobalsVSDefault();
> +

nitpick: should be "Vs"

> kconfigdata.cpp:105
> +if (e.bGlobal && !(options & EntryGlobal) && !k.bDefault)
> +{
> +e.bOverridesGlobal = true;

Should be at the end of the previous line

> kconfigdata.h:63
> +/**
> + * Entry will need to be written on a not global file even if it matches 
> default value
> + */

nitpick: I think I'd write "non global" rather than "not global"

REPOSITORY
  R237 KConfig

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

To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Kevin Ottens]

nt and copy it to kwin (likely 
with a rename to avoid symbol clashes);
 6) mark all of kwaylandserver API deprecated and freeze it;
 7) when KF6 gets branched drop all the deprecated APIs.

Again, I'm no Wayland expert so I might be missing some things. In any case: 
yes, this is quite some work and might incur some pain and trade-offs... it's 
unfortunately a price to pay to get out of what seems like a difficult 
situation which will lead to increasing pains over time.

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.

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#629380 , @ervin wrote:
  
  > In D27540#629362 , @ndavis wrote:
  >
  > > Is it possible to align all of the reset buttons like a column?
  >
  >
  > Why I'm not surprised. ;-)
  >
  > Honestly with enough code it might be possible, but that'd be expensive in 
term of effort... I'll need to keep track of all widgets in the page. I'll give 
it a shot but don't hold your breath.
  
  
  Alright, turns out I managed to do it both for QtWidgets and QtQuick cases, 
so you can breath again. ;-)
  
  Reviews on all my patches very welcome.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

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


  Have the indicators line up vertically automatically when applicable

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=77848=78469

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

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

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

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


  Have the indicators vertically line up automatically

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=77847=78468

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27063: Fix disabeling of autolock timeout

[Kevin Ottens]

ervin added a comment.


  Ah didn't know! I assumed you could push... But yeah, you authored a few 
patches now, time to apply for a developer account. You can put my name indeed.

REPOSITORY
  R133 KScreenLocker

BRANCH
  fix_disabled_state

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

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

D27063: Fix disabeling of autolock timeout

[Kevin Ottens]

ervin added a comment.


  Well, I already accepted it, I thought you pushed long ago (and I suspect it 
got overlooked by the other potential reviewers). :-)

REPOSITORY
  R133 KScreenLocker

BRANCH
  fix_disabled_state

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

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

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#629362 , @ndavis wrote:
  
  > Is it possible to align all of the reset buttons like a column?
  
  
  Why I'm not surprised. ;-)
  
  Honestly with enough code it might be possible, but that'd be expensive in 
term of effort... I'll need to keep track of all widgets in the page. I'll give 
it a shot but don't hold your breath.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

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


  Take feedback about the GUI into account

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=76952=77848

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

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

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the summary of this revision.
ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

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


  Take feedback about the GUI into account

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=76362=77847

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#627618 , @ndavis wrote:
  
  > Some extra rules I thought of:
  >
  > - With the checkable label example in the mockup above, it should reset 
both the label and the checkbox.
  
  
  Just for the record, this will unlikely to be enforceable on the framework 
side of the code, likely to be taken care of on a case by case basis when we 
encounter those in the modules themselves. Just managing expectations on what 
the code can easily do at that level of abstraction. So don't look for it in 
the patches I submit around the topic. ;-)
  
  The rest should be doable I'll update my patches shortly.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

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

To: crossi, ervin, dfaure, #frameworks
Cc: apol, meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27971: Solid-device-automounter/kcm: correctly update automountOn

[Kevin Ottens]

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

INLINE COMMENTS

> DeviceAutomounterKCM.cpp:66
>  
> -auto emitChanged = [this] {
> -
> m_devices->setAutomaticMountOnLogin(kcfg_AutomountOnLogin->isChecked());
> -
> m_devices->setAutomaticMountOnPlugin(kcfg_AutomountOnPlugin->isChecked());
> -emit markAsChanged();
> -};
> -
> -connect(m_devices, ::dataChanged, this, emitChanged);
> +connect(kcfg_AutomountOnLogin, ::stateChanged, [this](int 
> state) {
> +m_devices->setAutomaticMountOnLogin(state == Qt::Checked);

Missing this as third parameter

> DeviceAutomounterKCM.cpp:69
> +});
> +connect(kcfg_AutomountOnPlugin, ::stateChanged, [this](int 
> state) {
> +m_devices->setAutomaticMountOnPlugin(state == Qt::Checked);

ditto

REPOSITORY
  R119 Plasma Desktop

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

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

D27956: [Various KCMs] Notify about changes in GTK related settings

[Kevin Ottens]

ervin added a comment.


  In D27956#625419 , @ngraham wrote:
  
  > I'm finding myself wondering why we don't just make everything notify by 
default.
  
  
  IMHO it's mostly a question of limiting the chatter on the bus.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  notify-about-changes-in-gtk-related-settings (branched from master)

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

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

D27724: Syncronise setNeedsSave between KCModule and ConfigModule in both directions

[Kevin Ottens]

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


  What @meven said about the typo, otherwise LGTM indeed.

REPOSITORY
  R295 KCMUtils

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

To: davidedmundson, ervin
Cc: ervin, meven, iasensio, crossi, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27944: KCM Colors fix apply button always disabled

[Kevin Ottens]

ervin added a comment.


  I marked it accepted, but of course this is assuming David's patch wouldn't 
make it quickly and we'd need the fix here ASAP.

REPOSITORY
  R119 Plasma Desktop

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

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

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

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

REPOSITORY
  R246 Sonnet

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

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

INLINE COMMENTS

> autostart.h:52
> +void updateDesktopStartItem(DesktopStartItem *item, const QString , 
> const QString , bool disabled, const QString );
> +void updateScriptStartItem(ScriptStartItem *item, const QString , 
> const QString& command, AutostartEntrySource type, const QString );
>  

& is misplaced for command

> autostart.h:67
>  private:
> -QTreeWidgetItem *m_programItem, *m_scriptItem;
> -QString m_desktopPath;
> -QStringList m_paths;
> -QStringList m_pathName;
> +QModelIndex indexFromWidget(QTreeWidgetItem* widget);
> +

- is misplaced

> autostartitem.cpp:57
> +for (int i = 0; i < AutostartModel::listPathName().size(); i++) {
> +m_comboBoxStartup->addItem( AutostartModel::listPathName()[i], i + 1 
> /* +1 to skip first path that is not selectable for scripts */);
> +}

This is not obvious at all even with the comment... I guess you expect 
listPathName indices to almost line up with the enum values? That sounds very 
fragile as well then. Looks like it needs to be reworked a bit. Maybe listing 
the path names isn't enough you need to provide some extra context from the 
model.

> autostartmodel.cpp:56
> +{
> +
> this->append(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation)
>  + QStringLiteral("/autostart/"));
> +
> this->append(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation)
>  + QStringLiteral("/autostart-scripts/"));

We don't do "this->"

Beside I find using Q_GLOBAL_STATIC_WITH_ARGS and a factory function more 
readable and less boilerplaty than inheritance like you did.

It'd give something like:

  QStringList autostartPaths()
  {
  return {
  
QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
QStringLiteral("/autostart/"),
  ...
  };
  }
  Q_GLOBAL_STATIC_WITH_ARGS(QStringList, s_paths, autostartPaths())

> autostartmodel.cpp:69
> +{
> +this->append(i18n("Startup"));
> +this->append(i18n("Logout"));

ditto

> autostartmodel.cpp:101
> +const bool enabled = !(hidden ||
> +notShowList.contains(QLatin1String("KDE")) ||
> +(!onlyShowList.isEmpty() && 
> !onlyShowList.contains(QLatin1String("KDE";

is it me or indentation is wrong here? I'd expect one less space (same on the 
line below)

> autostartmodel.cpp:114
> +onlyInPlasma
> +});
> +}

Indentation seems off here as well, Note that the AutostartEntry() here is 
merely redundant. I think I'd write it like this:

  return {
  name,
  command,
  
  };

> autostartmodel.cpp:343
> +if (role == Qt::EditRole) {
> +emit dataChanged(index, index, {DisplayRole});
> +}

I think I'd avoid emitting twice but have one or two roles as third parameter.

Also, since I don't know how anyone else being that careful of specifying the 
dirty roles in dataChanged there's still the valid option of just emitting 
dataChanged with only two parameters.

> autostartmodel.cpp:421
> +
> +AutostartEntry entry = AutostartEntry{
> +destinationScript.fileName(),

I'd drop the redundant AutostartEntry (or use auto). Also could be const'd.

> autostartmodel.h:28
> +XdgAutoStart = 0,
> +XdgScripts= 1,
> +PlasmaShutdown = 2,

Missing space before =

> autostartmodel.h:60
> +
> +static AutostartEntrySource sourceFromInt(int index)
> +{

I'd still move the implementation in the cpp file. Makes sense to keep static 
now though.

> autostartmodel.h:68
> +}
> +return XdgAutoStart;
> +}

I still don't buy the safety argument for not changing this implementation 
which is very not "DRY".

index is an int so the argument of the enum-checked by the compiler is 
completely moot, it will never be checked (some compilers might even give a 
warning because of the lack of default in the switch which means it doesn't 
exhaust all possible values for index).
The cannot return "bad value" argument is debatable it means than an infinity 
of values (from int perspective which is obviously not really all natural 
numbers) will fallback to XdgAutostart

So it's really a conversion with no real check, whatever the way you paint it. 
I'd advocate making it look like what it is and with less repetition.

From your description I think I'd even go for the implementation I advocated 
earlier + Q_UNREACHABLE before the fallback return (or a Q_ASSERT about index 
being in the expected range, if I understood you correctly we're not supposed 
to hit that return ever).

REPOSITORY
  R119 Plasma Desktop

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

To: meven, mlaurent, ervin, #plasma, broulik, bport, crossi
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#617485 , @ervin wrote:
  
  > And now you got a screenshot as well. Waiting for further feedback now.
  
  
  Ping? This patch was first created three weeks ago now.
  
  Note that look in D27840  and D27841 
 is pretty much the same than in here for 
which I provided screenshot

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

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


  LGTM, please fix the typo in the docs before pushing though

INLINE COMMENTS

> README.dox:372
>  
> +Since 5.68, if present the value attribute will be used used as the 
> choice value written to the backend 
> +instead of the name, allowing to write text incompatible with enum 
> naming.

typo: it says "used used"

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

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

REPOSITORY
  R237 KConfig

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

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

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin added a comment.


  In D27840#622937 , @ngraham wrote:
  
  > This patch doesn't apply on top of KDeclarative for me:
  >
  >   This diff is against commit 3d8757d5dfea2360304e2c8e7d0d575d04b00735, 
but
  >   the commit is nowhere in the working copy. Try to apply it against the
  >   current working copy state? (a1282da765c1b909d03d6c94eb77fd99e4374d74)
  >   [Y/n] y
  >  
  >   Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.h...
  >   Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp...
  >   Checking patch src/qmlcontrols/kcmcontrols/qml/qmldir...
  >   Checking patch 
src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml...
  >   Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml...
  >   Checking patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp...
  >   Checking patch src/qmlcontrols/kcmcontrols/CMakeLists.txt...
  >   error: while searching for:
  >  
  >   set(kcmcontrols_SRCS
  >   kcmcontrolsplugin.cpp
  >   )
  >  
  >   add_library(kcmcontrolsplugin SHARED ${kcmcontrols_SRCS})
  >  
  >   error: patch failed: src/qmlcontrols/kcmcontrols/CMakeLists.txt:2
  >   Hunk #2 succeeded at 12 (offset -1 lines).
  >   Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.h cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/qmldir cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml 
cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml 
cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp cleanly.
  >   Applying patch src/qmlcontrols/kcmcontrols/CMakeLists.txt with 1 reject...
  >   Rejected hunk #1.
  >   Hunk #2 applied cleanly.
  >  
  >Patch Failed! 
  >
  
  
  Ah right, sorry it depends on D27839 .

REPOSITORY
  R296 KDeclarative

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

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

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a comment.


  In D27811#622916 , @ahmadsamir 
wrote:
  
  > In D27811#622885 , @ervin wrote:
  >
  > > This styleName thing is an endless amount of fun... The patch LGTM, I'll 
let others weight in though since it can have ramifications I might miss.
  >
  >
  > Well this one is the tip of the ice berg, the berg itself was 
a2774ff5b41987c3919a9e 
 :)
  
  
  Yeah I know but we still need to account for this Qt "addition" at other 
places. It wasn't exactly transparent.

REPOSITORY
  R237 KConfig

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

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

D27833: Add an accessor to get the last loaded value for KConfigSkeletonItem

[Kevin Ottens]

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


  In D27833#622353 , @apol wrote:
  
  > What's the use-case?
  
  
  Having an idea of the patch you want to build on top of this would indeed 
help.

INLINE COMMENTS

> kcoreconfigskeleton.cpp:210
> +Q_D(KConfigSkeletonItem);
> +d->mLoadedValueImpl= impl;
> +}

Missing space before =

> kcoreconfigskeleton.cpp:226
> +Q_D(const KPropertySkeletonItem);
> +return QVariant::fromValue(d->mLoadedValue);
> +});

This is already a QVariant

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a comment.


  This styleName thing is an endless amount of fun... The patch LGTM, I'll let 
others weight in though since it can have ramifications I might miss.

REPOSITORY
  R237 KConfig

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

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

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

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

INLINE COMMENTS

> kcoreconfigskeleton.cpp:581
> +// HACK for BC concerns
> +// TODO KF6: remove KCoreConfigSkeletonPrivate::mValues and add a value 
> field to KCoreConfigSkeleton::ItemEnum::Choice
> +const auto inHash = d_ptr->mValues.value(name);

You mean KConfigSkeletonItemPrivate aren't you? (instead of 
KCoreConfigSkeletonPrivate)

> meven wrote in kcoreconfigskeleton.h:788
> I expect those to be found through grep, and I had comments in the past to 
> put it somewhere where it could not get in documentation, in cpp guarantees 
> this.

Well, regular comments don't go in the docs ;-)
(you need the triple slash or the double start at start of comment for it to be 
picked up by doxygen)

But cpp, why not.

> KConfigCommonStructs.h:60
> +
> +QString value() const {
> +return !val.isEmpty() ? val : name;

New line before opening curly brace please

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a reviewer: bport.

REPOSITORY
  R237 KConfig

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

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

D27155: libnotificationmanager : add app-specific kconfig settings

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.


  Although might become "parentGroupName" depending on what you do about my 
comments on the review introducing sub-group handling.

REPOSITORY
  R120 Plasma Workspace

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

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

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

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

INLINE COMMENTS

> kconfigcompiler_test.cpp:80
>  "test_signal",
> +"test_notifiers",
>  "test_translation_kde",

This seems unrelated... So we had this test case available but it was in fact 
never run?

> test_subgroups.kcfg:10
> +
> +
> +

Now that I see it, I think I'd go for "parentGroupName" since this is not 
referential and really about the name (like the name parameter)

> test_subgroups.kcfg:20
> +
> +

Probably worth also having cases with:

- the group name being parameterized but not the parentGroup
- both name and parentGroup not be parameterized

Just to have all the possible combinations.

> KConfigSourceGenerator.cpp:354
>  
> +// TODO : Some compiler option won't work or generate bogus settings file.
> +// * Does not manage properly Notifiers=true kcfgc option for parameterized 
> entries :

Unrelated right, this is not due to your patch, or am I confused?

> KConfigXmlParser.cpp:502
>  
> +QString parentGroup = e.attribute(QStringLiteral("parentGroup"));
> +

I'd const auto it, or at least const.

REPOSITORY
  R237 KConfig

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

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

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.cpp:580
> You mean I should prepare this and put it in KF6 waiting for merge queue ?

No I meant the comment needs to be adjusted due to the changes (fields changing 
place and such) sorry if I was unclear.

> meven wrote in kcoreconfigskeleton.h:788
> I have one in `QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString 
> name) const`

Yeah, noticed only later... I wonder if it's more obvious in the header or the 
cpp...

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

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

INLINE COMMENTS

> kcoreconfigskeleton.cpp:580
> +{
> +// TODO KF6 move value to ItemEnum::Choice and remove 
> KCoreConfigSkeleton::ItemEnum::mValues
> +const auto inHash = mValues.value(name);

Will need an update

> kcoreconfigskeleton.cpp:586
> +/**
> + * Stores a choice value for name
> + */

This comment should go away

> kcoreconfigskeleton.h:788
> + */
> +QString valueForChoice(QString name) const;
> +

const QString 

Probably worth adding a KF6 comment somewhere as well, because your first 
attempt felt more natural, we're going for this weird construct only for BC 
concerns.

> kcoreconfigskeleton.h:793
> + */
> +void setValueForChoice(QString name, QString valueForChoice);
> +

const ref for the parameters

> kcoreconfigskeleton.h:797
>  QList mChoices;
> +QHash mValues;
>  };

Nope, can't be here, this still breaks binary compatibility. As I mentioned in 
my last comment it should be buried all the way into the d-pointer inherited 
from KConfigSkeletonItem... This is inefficient in term of memory load but it's 
the only option to keep BC.

> KConfigCommonStructs.h:108
>  Choices choices;
> +QHash enumValues;
>  QList signalList;

Well, on that side you should have kept the more natural value() method IMO.

> KConfigXmlParser.cpp:193
>  QList chlist;
> +const QRegularExpression choiceNameRegex = 
> QRegularExpression(QStringLiteral("\\w+"));
> +

nitpick: I'd const auto that one, but it's your call

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin added a dependent revision: D27841: Port desktoptheme, icons and 
workspace KCMs to SettingStateBinding.

REPOSITORY
  R296 KDeclarative

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

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

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

[Kevin Ottens]

ervin edited the summary of this revision.
ervin added reviewers: Frameworks, Plasma, VDG.
ervin added a dependency: D27840: Introduce SettingState* elements to ease KCM 
writing.

REPOSITORY
  R119 Plasma Desktop

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

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

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin edited the summary of this revision.
ervin added reviewers: Frameworks, Plasma.

REPOSITORY
  R296 KDeclarative

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

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

D27839: Properly name the content of the kcmcontrols project

[Kevin Ottens]

ervin added reviewers: Frameworks, Plasma.

REPOSITORY
  R296 KDeclarative

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

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

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

[Kevin Ottens]

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

TEST PLAN
  Has been tested with the three modules mentioned in the commit message

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/desktoptheme/package/contents/ui/main.qml
  kcms/icons/package/contents/ui/IconSizePopup.qml
  kcms/icons/package/contents/ui/main.qml
  kcms/workspaceoptions/package/contents/ui/main.qml

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

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

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

REVISION SUMMARY
  This is the QML based counterpart of D27540 
. Unlike with the KCModule
  case, this is not automatically propagated to the ConfigModules, they
  will all have to be adapted to make use of it.
  
  I will provide another patch which ports a few ConfigModule to see
  how it looks.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27839: Properly name the content of the kcmcontrols project

[Kevin Ottens]

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

REVISION SUMMARY
  Looks like it was copied from dragandrop, finish to properly rename
  everything.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27717: fix min/max entries with dpointer

[Kevin Ottens]

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

REPOSITORY
  R237 KConfig

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

To: hchain, meven, ervin
Cc: kde-frameworks-devel, aacid, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> bport wrote in spellchecking.cpp:88
> Yes, or I can probably also use item like before, and that will be handled 
> for me

That would create other issues I think (not fully sure TBH). Well in any case 
that's an extra level of indirection we don't really need (and it's already 
very confusing as is). Adding the missing usrRead() in the skeleton ctor is 
clearly easier IMO.

> bport wrote in spellchecking.cpp:118
> no because we hold loaded data if we do that, apply button will be 
> deactivated after clicking default

*sigh* OK... KConfigDialogManager is so confusing each time...

REPOSITORY
  R119 Plasma Desktop

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

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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> bport wrote in spellchecking.cpp:88
> no load only do findItem for managed widget on the skeleton

Sure but still, the skeleton is expected to be already be in a "loaded" state 
at that point, that's why it works for the items (otherwise a findItem wouldn't 
be enough, it'd have to call readConfig on them or load on the whole skeleton, 
and it doesn't). I think the problem is more that in the ctor of the skeleton 
you initialize the items just fine but you don't initialize the extra 
properties you need a call to usrRead() at the end of that ctor.

> bport wrote in spellcheckingskeleton.cpp:51
> Yes is necessary no code is run after that so nobody will take care of saving 
> this skeleton.

Ah right it does the writeConfig calls first... there's really some 
"interesting" design choices in KCoreConfigSkeleton...

REPOSITORY
  R119 Plasma Desktop

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

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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

[Kevin Ottens]

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

INLINE COMMENTS

> ervin wrote in spellchecking.cpp:55
> What about calling toSet() on those? Not overly efficient but would compress 
> a bit the resulting code. Alternatively, couldn't we ensure those lists are 
> always kept sorted?

Not what I had in mind at all, now it's even less readable and less performant 
than before... I had something in mind like:
`const auto currentIgnoreSet = m_configWidget->ignoreList().toSet()`

I know they advertise the new range ctor, but it wouldn't buy us anything in 
that context. At least the `toSet()` call even though less performant buys us 
some readability.

> spellchecking.cpp:43
> +m_skeleton = new SpellCheckingSkeleton(this);
> +m_configWidget = new Sonnet::ConfigView(this);
> +
> m_configWidget->setNoBackendFoundVisible(m_skeleton->clients().isEmpty());

I notice it only now, but those two `new` would be better done in the ctor 
initialization list.

> spellchecking.cpp:88
> +// Load unmanaged widget
> +m_skeleton->load();
> +m_configWidget->setIgnoreList(m_skeleton->ignoreList());

Are you sure this is necessary? I'd expect KCModule::load() to call load() on 
m_skeleton

> spellchecking.cpp:118
> +
> m_configWidget->setPreferredLanguages(Sonnet::Settings::defaultPreferredLanguages());
> +m_configWidget->setLanguage(Sonnet::Settings::defaultDefaultLanguage());
>  }

I'd expect this to be the same block than the one in load() now. Since 
`KCModule::defaults()` will reset the skeleton to defaults, ignoreList, 
preferredLanguages and defaultLanguage will hold the default values.

> spellcheckingskeleton.cpp:27
> +
> +SpellCheckingSkeleton::SpellCheckingSkeleton(QObject *parent)
> +: KCoreConfigSkeleton(QStringLiteral(""), parent), m_store(new 
> Sonnet::Settings(this))

I'd tend to name that SpellCheckingSettings I think and m_settings instead of 
m_skeleton. I understand this is debatable because of the proximity with 
Sonnet::Settings but you hold it in a m_store member so it reduces chances of 
confusion I think.

> spellcheckingskeleton.cpp:51
> +m_store->setDefaultLanguage(m_defaultLanguage);
> +m_store->save();
> +return KCoreConfigSkeleton::usrSave();

I have a doubt, is it really necessary? I'd expect it to work without that line.

REPOSITORY
  R119 Plasma Desktop

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

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

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

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

INLINE COMMENTS

> settings.cpp:62
>  
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setSkipUppercase(bool check)
>  {

Rename the parameter to skip as well please

> settingsimpl.cpp:260
>  //same defaults are in the default filter (filter.cpp)
> -d->checkUppercase = settings.value(QStringLiteral("checkUppercase"), 
> true).toBool();
> -d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"), 
> true).toBool();
> +d->checkUppercase = settings.value(QStringLiteral("checkUppercase"), 
> Settings::defaultSkipUppercase()).toBool();
> +d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"), 
> Settings::defauktSkipRunTogether()).toBool();

I'd expect a negation here... if by default skip is false, check in 
settingsimpl should be true (which was the old default).

REPOSITORY
  R246 Sonnet

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

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

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

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

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.h:764
> Oh right, stupid me, this obviously breaks binary compatibility, we need to 
> find another way to store this.

Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer 
(likely a hash associating vals with names)... it'd be unused by most item 
types of course which sucks but at least it'd be safe BC wise.

> KConfigXmlParser.cpp:202
>  std::cerr << "Tag  requires attribute 'name'." << 
> std::endl;
> +} else if 
> (!(QRegularExpression(QStringLiteral("\\w+"))).match(choice.name).hasMatch()) 
> {
> +std::cerr << "Tag  attribute 'name' must be compatible 
> with Enum naming. name was '" << qPrintable(choice.name) << "'. You can use 
> attribute 'value' to pass any string as the choice value." << std::endl;

We should move the QRegularExpression out of the loop, otherwise it'll get 
compiled over and over for each choice (premature pessimisation imo). We could 
go one step further and have it as a member of the parser to have it compiled 
only once of course, but maybe we'd get in premature optimization territory.

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.h:764
> Something I have noticed while testing this.
> Since it changes the memory of a very common data struct in a very common 
> lib, it creates a lot of crashes if apps are not compiled with the installed 
> version.
> So all local builds should be rebuild or reinstalled, once this has landed, 
> or you end up with a lot of crashes.

Oh right, stupid me, this obviously breaks binary compatibility, we need to 
find another way to store this.

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> autostart.cpp:87
>  
> +setMinimumHeight(300);
> +setMinimumWidth(400);

Unrelated right? Where do this come from?

> autostart.cpp:112
>  
> -void Autostart::addItem( DesktopStartItem* item, const QString& name, const 
> QString& run, const QString& command, bool disabled )
> +void Autostart::setDesktopStartItem( DesktopStartItem* item, const QString 
> , const QString , bool disabled, const QString )
>  {

Please remove the space after (, and space should be before * not after

Shouldn't that be called "updateDesktopStartItem" or similar? It's not setting 
anything on "this" after all, but changing the state of the item.

> autostart.cpp:183
>  {
>  AddScriptDialog * addDialog = new AddScriptDialog(this);
> +if (addDialog->exec() != QDialog::Accepted) {

It'd be better to have this dialog on the stack, or reachable via a pointer 
available between calls to this slot. Currently it'd instantiate a new dialog 
each time this slot is invoked and it'd never be freed until the KCModule 
itself is freed.

> autostart.cpp:243
> +QTreeWidgetItem *item = m_scriptItem->child(topLeft.row() - 
> m_programItem->childCount());
> +ScriptStartItem *scriptEntry = dynamic_cast( item 
> );
> +setScriptStartItem(scriptEntry, name, command, source, fileName);

qobject_cast is deemed faster (since it turns out those items inherit from 
QObject which gives me the creeps somehow)

> autostart.cpp:250
> +QTreeWidgetItem *item = m_programItem->child(topLeft.row());
> +DesktopStartItem *desktopItem = dynamic_cast( 
> item );
> +setDesktopStartItem(desktopItem, name, command, !enabled, fileName);

ditto

> autostart.cpp:254
> +
> +// const int index = 
> widget->listCMD->indexOfTopLevelItem(widgetItem->parent()) + 
> widgetItem->parent()->indexOfChild(widgetItem);
> +}

I guess you didn't meant to commit this

> autostart.cpp:288
>  return;
> -slotEditCMD( (AutoStartItem*)widget->listCMD->currentItem() );
> +slotEditCMD( 
> dynamic_cast(widget->listCMD->currentItem()) );
>  }

ditto

> autostart.h:48
>  public Q_SLOTS:
> -void slotChangeStartup( ScriptStartItem* item, int index );
> +void slotChangeStartup( ScriptStartItem *item, int index );
>  

Since you touched the line anyway you could as well remove the spaces between 
the parentheses

> autostartmodel.cpp:1
> +#include "autostartmodel.h"
> +

Copyright header missing

> autostartmodel.cpp:31
> +// must match enum AutostartEntrySource
> +const static QStringList s_paths = QStringList()
> +<< 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> QStringLiteral("/autostart/")

Urgh, no, use Q_GLOBAL_STATIC with an initializer

> autostartmodel.cpp:37
> +
> +const QStringList s_pathName = QStringList() << i18n("Startup")
> + << i18n("Logout")

Ditto

> autostartmodel.cpp:66
> +{
> +this->beginResetModel();
> +

We don't do "this->"

> autostartmodel.cpp:74
> +
> +autostartdir.setFilter( QDir::Files );
> +

Remove the spaces between the parentheses

> autostartmodel.cpp:76
> +
> +for (QFileInfo fi : autostartdir.entryInfoList()) {
> +QString filename = fi.fileName();

const auto , and qAsConst (have no idea if the list is cached somewhere and 
thus could detach)

> autostartmodel.cpp:79
> +bool desktopFile = filename.endsWith(QLatin1String(".desktop"));
> +if ( desktopFile ) {
> +AutostartEntry entry = loadDesktopEntry(fi.absoluteFilePath());

Drop the spaces

> autostartmodel.cpp:100
> +
> +dir.setFilter( QDir::Files );
> +

Drop the spaces

> autostartmodel.cpp:102
> +
> +for (QFileInfo fi : dir.entryInfoList()) {
> +

const auto , and qAsConst (have no idea if the list is cached somewhere and 
thus could detach)

> autostartmodel.cpp:121
> +
> +this->endResetModel();
> +}

s/this->//

> autostartmodel.cpp:126
> +{
> +Q_UNUSED(parent)
> +

Should return 0 if the parent is valid

> autostartmodel.cpp:131
> +
> +bool AutostartModel::checkEntry(const AutostartEntry )
> +{

Shouldn't that be at least const? Actually could probably even be static or in 
an anonymous namespace at the top of this file

> autostartmodel.cpp:145
> +
> +AutostartEntry AutostartModel::loadDesktopEntry(const QString )
> +{

Ditto

> autostartmodel.cpp:195
> +
> +switch (role) {
> +case DisplayRole : return entry.name;

at least Qt::DisplayRole should be handled

> autostartmodel.cpp:196
> +switch (role) {
> +case DisplayRole : return entry.name;
> +case Command : return entry.command;

No space before :

> autostartmodel.cpp:217
> +switch (role) {
> +case DisplayRole : {
> +if (entry.name == value.toString()) {

I think you meant Qt::EditRole here. Also no space before :

> autostartmodel.cpp:303
> +if (dirty) {
> +emit dataChanged(index, index, {role});
> +}

There might be a trick in the 

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

[Kevin Ottens]

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

INLINE COMMENTS

> spellchecking.cpp:55
> +
> +auto referenceValue = m_settings->currentIgnoreList();
> +auto currentValue = m_configWidget->ignoreList();

What about calling toSet() on those? Not overly efficient but would compress a 
bit the resulting code. Alternatively, couldn't we ensure those lists are 
always kept sorted?

> spellchecking.cpp:61
> +defaultValue.sort();
> +if (currentValue != referenceValue) {
> +unmanagedChangeState = true;

this is:

  unmanagedChangeState |= currentValue != referenceValue;

> spellchecking.cpp:64
> +}
> +if (currentValue != defaultValue) {
> +unmanagedDefaultState = false;

this is:

  unmanagedDefaultState &= currentValue == defaultValue

> spellchecking.cpp:74
> +defaultValue.sort();
> +if (currentValue != referenceValue) {
> +unmanagedChangeState = currentValue != referenceValue;

see above

> spellchecking.cpp:77
> +}
> +if (currentValue != defaultValue) {
> +unmanagedDefaultState = false;

see above

> spellchecking.cpp:82
> +
> +if (m_settings->defaultLanguage() != m_configWidget->language()) {
> +unmanagedChangeState = true;

see above

> spellchecking.cpp:85
> +}
> +if (m_configWidget->language() != 
> Sonnet::Settings::defaultDefaultLanguage()) {
> +unmanagedDefaultState = false;

see above

> ervin wrote in spellchecking.h:56
> nitpick: we usually find methods before variables

This still applies

> spellcheckingskeleton.cpp:38
> +
> +void SpellCheckingSkeleton::usrRead()
> +{

I'd expect this to update m_preferredLanguages, m_ignoreList and 
m_defaultLanguage otherwise we could have situations where things get out of 
sync. Also I'd expect to see usrSetDefaults() to be overridden as well.

REPOSITORY
  R119 Plasma Desktop

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

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

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

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

INLINE COMMENTS

> settings.cpp:62
>  
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setCheckUppercase(bool check)
>  {

Please rename it to "skip uppercase"...

Otherwise it does the opposite of its name, it's just confusing

> settings.cpp:67
>  
>  bool Settings::checkUppercase() const
>  {

Likewise should be renamed to "skip"

> configview.cpp:43
> +
> +void ConfigViewPrivate::slotSelectionChanged() {
> +
> ui.removeButton->setEnabled(!ui.ignoreListWidget->selectedItems().isEmpty());

Break the line before { please

REPOSITORY
  R246 Sonnet

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

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

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#617589 , @alexde wrote:
  
  > Does this patch also fix indirectly #274629?
  
  
  I doubt it, this bug report seems confused BTW... it was never broken on the 
kdelibs side but *some* dialogs have bugs which break the feature for them. 
Unrelated to this patch we'd been doing a big sweep on the systemsettings 
module to address among other thing the problems they might have in that 
department.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin added a comment.


  In D27133#617577 , @dfaure wrote:
  
  > I really have zero experience with this stuff, but you're asking for my 
opinion nonetheless, so you'll get it, as crappy as it might be :-)
  >
  > Your comment refers to "group separator" as if it was an existing notion in 
KConfig. Can you point me where? Grepping for '241' or for 'separator' gives 
nothing.
  >  Do KConfig files use this already? Or is this all kconfig_compiler 
specific?
  >  I told you, I'm clueless :-)
  
  
  Right, on the C++ side you'd have to look for \x1d

REPOSITORY
  R237 KConfig

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

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

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

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

REPOSITORY
  R237 KConfig

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

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

ervin edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27380: [GTK Config] Construct font style by hand instead of relying on Qt function

[Kevin Ottens]

ervin added a comment.


  LGTM, now requires swift testing to not miss the next bugfix release

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

To: gikari, #plasma, ervin, bport, meven, davidedmundson
Cc: chauvin, davidre, davidedmundson, cfeck, ngraham, IlyaBizyaev, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615160 , @ervin wrote:
  
  > In D27540#615156 , @ngraham 
wrote:
  >
  > > Please add screenshots to the Test Plan section for patches that change 
the UI. :) Also it would be nice to indicate how someone can test this. Which 
apps? System Settings? Where? How?
  >
  >
  > Well, second part I pointed out a few kcms you can test with. ;-)
  
  
  And now you got a screenshot as well. Waiting for further feedback now.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

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


  Handle visibility of the tracked widget, setters become slots, and add plural 
to "indicatorWidgets".

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=76089=76362

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615091 , @davidre wrote:
  
  > The editmarks should probably respect the visibility of the associated 
widget. In this picture I use a invisble lineedit to manage the settings of the 
QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)
  >  F8117874: grafik.png 
  
  
  For the record, next iteration of that patch will honor the visibility of the 
associated widget (got some more changes to do locally before uploading though, 
stay tuned).
  
  That being said, what you did is a very naughty hack you know. Why not fixing 
KConfigDialogManager or go through a QGroupBox (which is handled in some way). 
;-)

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

[frameworks-kconfig] [Bug 418146] kconfig_compiler with GenerateProperties creates uncompilable/wrong code

[Kevin Ottens]

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

--- Comment #2 from Kevin Ottens  ---
Could you test https://phabricator.kde.org/D27497 ? I think it will address
your issue.

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

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

ervin added a comment.


  A few smallish issues only, otherwise LGTM.

INLINE COMMENTS

> KConfigSourceGenerator.cpp:316
>  {
> -   stream() << "  " << itemPath(entry, cfg()) << " = "
> +QString innerItemVarStr(innerItemVar(entry, cfg()));
> +if (!entry->signalList.isEmpty()) {

I'd const it and also use the = style of initialization which I find more 
readable.

> KConfigSourceGenerator.cpp:354
> +QString argBracket = QStringLiteral("[%1]").arg(i);
> +QString innerItemVarStr = innerItemVar(entry, cfg()) + argBracket;
>  

const those please

> KConfigSourceGenerator.cpp:365
> +
> +QString itemVarStr(itemPath(entry, cfg()) + argBracket);
> +

ditto

> kconfig_compiler.cpp:397
>  {
> -if (cfg.itemAccessors) {
> -return QString();
> +QString type = cfg.inherits + "::Item" + itemType(e->type);
> +

const

> kconfig_compiler.cpp:401
> +fCap[0] = fCap[0].toUpper();
> +QString argSuffix = (!e->param.isEmpty()) ? 
> (QStringLiteral("[%1]").arg(e->paramMax + 1)) : QString();
> +QString result;

const

> kconfig_compiler.cpp:462
>  
> -QString newItem(const CfgEntry* entry, const QString , const QString& 
> defaultValue,
> +QString newItemInner(const CfgEntry *entry, const QString , const 
> QString ,
>  const KConfigParameters , const QString ) {

Should be named newInnerItem

REPOSITORY
  R237 KConfig

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

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

ervin requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R237 KConfig

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

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

[Kevin Ottens]

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

INLINE COMMENTS

> spellchecking.cpp:30
> +#include 
> +#include 
> +#include 

Good opportunity to switch to the camel case includes?

> spellchecking.cpp:40
> +SonnetSpellCheckingModule::SonnetSpellCheckingModule(QWidget *parent, const 
> QVariantList &)
> +: KCModule(parent)
> +, m_loader(Sonnet::Loader::openLoader())

Either phab display is stupid of the indentation on that line and the next is 
wrong

> spellchecking.cpp:52
> +
> +void SonnetSpellCheckingModule::stateChanged()
> +{

Code in here feels rather inelegant, my brain is too mushy right now to propose 
something though... maybe once it reboots...

> spellchecking.cpp:101
> +{
> +m_skeleton->load();
> +KCModule::load();

This warrants a comment, because with the addConfig call one would expect this 
to be unnecessary.

> spellchecking.cpp:107
>  {
> -m_configWidget->save();
> +if (!m_managedConfig->hasChanged()) {
> +m_skeleton->save();

Ditto

> spellchecking.cpp:115
>  {
> -m_configWidget->slotDefault();
> +m_configWidget->setLanguage(Sonnet::Settings::defaultDefaultLanguage());
> +
> m_configWidget->setPreferredLanguages(Sonnet::Settings::defaultPreferredLanguages());

Ditto

> spellchecking.h:56
> +
> +void stateChanged();
>  

nitpick: we usually find methods before variables

> spellcheckingskeleton.cpp:43
> +
> +SpellCheckingSkeleton::~SpellCheckingSkeleton()
> +{}

If this needs to be kepts for some reason, please use `= default` instead

> spellcheckingskeleton.h:38
> +public:
> +SpellCheckingSkeleton(Sonnet::ConfigView *widget, Sonnet::Settings 
> *settings, QObject *parent = nullptr);
> +~SpellCheckingSkeleton() override;

Passing the widget in here feels very much wrong (layer violation and all 
that). I'd also expect it to not receive the settings object but to create it 
internally (although that's a less major issue).

> spellcheckingskeleton.h:39
> +SpellCheckingSkeleton(Sonnet::ConfigView *widget, Sonnet::Settings 
> *settings, QObject *parent = nullptr);
> +~SpellCheckingSkeleton() override;
> +bool usrSave() override;

Probably unnecessary

REPOSITORY
  R119 Plasma Desktop

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

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

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

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

INLINE COMMENTS

> loader.h:6
>   */
> -#ifndef SONNET_LOADER_P_H
> -#define SONNET_LOADER_P_H
> +#ifndef SONNET_LOADER_H
> +#define SONNET_LOADER_H

What's the reason for loader becoming public? If that's mostly out of 
convenience (it looks like it is), I'd advise not turning it public, Settings 
and ConfigView can use it internally but let's not increase the API surface 
even more with Loader.

> settings.cpp:13
>  #include 
> -#include 
>  #include 

Obviously will reappear here ;-)

> settings.cpp:109
>  {
> -if (d->checkUppercase != check) {
> +if (d->checkUppercase != !check) {
>  d->modified = true;

This is an odd bit of logic, isn't it?

> settings.cpp:119
>  {
> -return d->checkUppercase;
> +return !d->checkUppercase;
>  }

Ditto

> settings.h:9
> +
> +#include 
> +#include 

Please remove that include (it'll become more obvious why you'll be able to do 
it with one of the comments below) ;-)

> settings.h:33
> +
> +public:
> +~Settings() override;

Will need a ctor (see other comments) in the form of `explicit Settings(QObject 
*parent = nullptr);`

> settings.h:36
> +
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;

This is likely useless now, since we inherit from QObject and QObject is 
non-copyable

> settings.h:37
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;
> +

Ditto

> settings.h:79
> +// A static list of KDE specific words that we want to recognize
> +static QStringList defaultIgnoreList()
> +{

Move all the implementations of those static methods to the cpp file.

> settings.h:136
> +private:
> +void readIgnoreList();
> +bool setQuietIgnoreList(const QStringList );

Time to move those to the SettingsPrivate class.

> settings.h:141
> +friend class Loader;
> +explicit Settings(Loader *loader);
> +private:

Might end up disappearing completely, unsure but to be evaluated

> speller.cpp:216
>  case CheckUppercase:
> -d->settings->setCheckUppercase(b);
> +d->settings->setCheckUppercase(!b);
>  break;

And that negation again? We're negating twice now...

> speller.cpp:232
>  case CheckUppercase:
> -return d->settings->checkUppercase();
> +return !d->settings->checkUppercase();
>  case SkipRunTogether:

And again...

I wonder what's happening there, it's highly confusing.

> configview.cpp:33
> +layout->setObjectName(QStringLiteral("SonnetConfigUILayout"));
> +d->wdg = new QWidget(this);
> +d->ui.setupUi(d->wdg);

I'm kind of surprised at this extra widget, can't we get rid of it?

> configview.h:17
> +}
> +#include "sonnetui_export.h"
> +

Please move this line just after the `#include `

> configview.h:23
> +{
> +Q_OBJECT
> +public:

You might want to declare a bunch of Q_PROPERTY there

> configview.h:25
> +public:
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();

clients should be a const reference (although I suspect it'll disappear)

parent should be the last parameter and default to nullptr

> configview.h:26
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();
> +

override

> configview.h:30
> +
> +void setPreferredLanguages(const QStringList );
> +QStringList preferredLanguages() const;

All the setters there would make for good public slots

> configview.h:34
> +QString language() const;
> +void setIgnoreList(const QStringList& ignoreList);
> +QStringList ignoreList() const;

Space before & not after

> configview.h:39
> +void setBackgroundCheckingButtonShown(bool);
> +protected Q_SLOTS:
> +void slotIgnoreWordRemoved();

Are they directly called by a subclass outside the library? If not move them to 
the dpointer and use Q_PRIVATE_SLOT

> configview.h:42
> +void slotIgnoreWordAdded();
> +private Q_SLOTS:
> +void slotUpdateButton(const QString );

Move to the d pointer and use Q_PRIVATE_SLOT for those

> configview.h:55
> +ConfigViewPrivate *const d;
> +QStringList m_ignoreList;
> +};

Should be in the d pointer

REPOSITORY
  R246 Sonnet

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

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

D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

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

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

D27482: Update kdeglobals config file for Breeze widgetStyle

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> bport wrote in style_widgetstyle_default_breeze.pl:8
> This will fix only for Breeze theme
> This will fix existing case but we also need to fix the root cause (i.e. how 
> we end up with a lowercase name).

Sure but that would be a different  patch isn't it? (both should be rolled out 
together though)

Now I wonder if there are more than "breeze" that we'd like to fix in the 
process? Just to avoid having several config updates to deploy.

REPOSITORY
  R119 Plasma Desktop

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

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

D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

[Kevin Ottens]

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


  LGTM, please don't forget to address dfaure's comment before pushing though.

REPOSITORY
  R237 KConfig

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

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

D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

[Kevin Ottens]

ervin added a comment.


  In D27059#612205 , @dfaure wrote:
  
  > Well, I'm not the KConfig maintainer, mdawson is :-)
  
  
  Well, my understanding is that mdawson is MIA, so I rely on the old "dfaure 
as default maintainer" model. ;-)

REPOSITORY
  R237 KConfig

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

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

D27380: [GTK Config] Construct font style by hand instead of relying on Qt function

[Kevin Ottens]

ervin added a comment.


  A minor style nitpick, otherwise LGTM. As mentioned by the author, this 
likely require extensive testing before going in.

INLINE COMMENTS

> configvalueprovider.cpp:53
>  
> +QString ConfigValueProvider::fontStyleHelper(const QFont& font) const
> +{

Space should be before & not after

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

To: gikari, #plasma, ervin, bport, meven, davidedmundson
Cc: chauvin, davidre, davidedmundson, cfeck, ngraham, IlyaBizyaev, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27024: Solid-device-automounter/kcm: show disconnected known device when disconnecting it

[Kevin Ottens]

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


  I guess extra care will need to be taken with your other patch moving away 
from the singleton (dunno which one is based on which, I might review out of 
order).

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D27024

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

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

D27480: Solid-device-automounter/kcm: Get rid of singleton for AutomounterSettings

[Kevin Ottens]

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

INLINE COMMENTS

> DeviceAutomounterKCM.cpp:44
>  : KCModule(parent, args)//DeviceAutomounterKCMFactory::componentData(), 
> parent)
> -  , m_devices(new DeviceModel(this))
> +  , m_settings(new AutomounterSettings(this)), m_devices(new 
> DeviceModel(this, m_settings))
>  {

please break the line before ", m_devices"

> DeviceModel.cpp:34
> +DeviceModel::DeviceModel(QObject *parent, AutomounterSettings *m_settings)
> +: QAbstractItemModel(parent), m_settings(m_settings)
>  {

m_settings on its own line?

> DeviceModel.h:37
>  public:
> -explicit DeviceModel(QObject *parent = nullptr);
> +explicit DeviceModel(QObject *parent, AutomounterSettings *m_settings);
>  ~DeviceModel() override = default;

parent should come last and keep the = nullptr

> DeviceAutomounter.cpp:111
>  Solid::Device dev(udi);
> -automountDevice(dev, AutomounterSettings::Attach);
> -AutomounterSettings::self()->save();
> +automountDevice(dev, m_settings->Attach);
> +m_settings->save();

This is rather odd. A too eager search and replace? ;-)

> AutomounterSettings.cpp:23
>  
> +AutomounterSettings::AutomounterSettings(QObject *parent) :
> +AutomounterSettingsBase(parent)

I guess you could have used `using 
AutomounterSettingsBase::AutomounterSettingsBase;` in the header instead.

> AutomounterSettings.h:31
>  public:
> +AutomounterSettings( QObject *parent = nullptr );
>  enum AutomountType {

No space between the parenthesis

REPOSITORY
  R119 Plasma Desktop

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

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

D27127: Solid-device-automounter/kcm: Use KConfigXT in ui

[Kevin Ottens]

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

INLINE COMMENTS

> DeviceAutomounterKCM.cpp:44
>  : KCModule(parent, args)//DeviceAutomounterKCMFactory::componentData(), 
> parent)
> +  , m_devices(new DeviceModel(this))
>  {

This is wrongly indented

> ervin wrote in DeviceAutomounterKCM.cpp:114
> I'd store the result of the automountEnabled call in an intermediate 
> variable, just to make things more readable.

Comment has been marked done but I don't see it changed here. Wrong status of 
the review?

REPOSITORY
  R119 Plasma Desktop

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

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

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

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

INLINE COMMENTS

> kcoreconfigskeleton.h:766
> +
> +public:
> +QString value() const {

It's a struct you can drop the public: here.

> kcoreconfigskeleton.h:767
> +public:
> +QString value() const {
> +return val.isEmpty() ? name : val;

There should be a new line before the opening curly brace.

I also wonder if it wouldn't be better with the implementation being moved to 
the cpp file, otherwise it risks being inlined which might not be the most 
convenient for longer term management of the change (if for some reason the 
implementation needs to be changed).

> KConfigCommonStructs.h:61
> +public:
> +QString value() const{
> +return val.isEmpty() ? name : val;

New line before opening curly brace please.

> KConfigXmlParser.cpp:203
>  }
> +else if (choice.name.contains(QLatin1Char(' ')) || 
> choice.name.contains(QLatin1Char('/')) || 
> choice.name.contains(QLatin1Char('.')) || 
> choice.name.contains(QLatin1Char(':'))) {
> +std::cerr << "Tag  attribute 'name' must be compatible 
> with Enum naming. name was '" << qPrintable(choice.name) << "'. You can use 
> attribute 'value' to pass any string as the choice value." << std::endl;

else if should be just after the closing curly brace

As for checking the valid characters for an enum name this is "easy" it can 
only be alphabetical, numerical and underscore characters (technically 
shouldn't start with a digit). Any other character will be rejected by the 
compiler, the current filter is thus not discriminating enough at all and I 
think its logic should be reversed (the blacklist being potentially infinite it 
should be white list based).

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615132 , @davidre wrote:
  
  > They stick around because as you said the settings differ from the defaults.
  
  
  Right, so "not a bug" (as in that's what's the patch is intended to do so 
far).
  
  > The pen for me conveys the meaning that something was edited that's the 
reason I would only show it for unsaved changes. But let's others weigh in what 
they think
  
  Right, I picked visual indicators which came to mind, I'm totally open to 
suggestions on which would be better suited.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615156 , @ngraham wrote:
  
  > Please add screenshots to the Test Plan section for patches that change the 
UI. :) Also it would be nice to indicate how someone can test this. Which apps? 
System Settings? Where? How?
  
  
  Well, second part I pointed out a few kcms you can test with. ;-)

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615105 , @davidre wrote:
  
  > In D27540#615092 , @ervin wrote:
  >
  > > In D27540#615061 , @davidre 
wrote:
  > >
  > > > How does it look? Will this be opt in for other users of 
KConfigDialogManager?
  > >
  > >
  > > I'd say you should try it. ;-)
  > >  I really need wider feedback on how it behaves in different context. 
Currently the patch makes it mandatory for everyone.
  >
  >
  > I use KConfigDialogManager to manage the settings in Spectacle MainWindow 
which are instant apply by 
  >  `connect(mConfigManager, ::widgetModified, 
mConfigManager, ::updateSettings);` and I don't like it 
that they appear there too
  
  
  You mean they appear and stick around? Or they appear/disappear immediately?
  If they stick around I'd indeed have to check why the state isn't recomputed 
on the updateSettings call...
  
  > F8117881: grafik.png 
  > 
  >> a setting which is currently dirty or which differs from default value.
  > 
  > What is your idea behind this? After first seeing this patch my intuition 
was that it would show for unsaved changes. But then I was confused when I open 
a SettingsDialog that there were already marks even though I changed nothing! I 
would expect them only for unsaved changes, I don't know how other platforms 
handle this if they have something similiar?
  
  Currently it helps the user find out the unsaved changes (the least useful in 
some way, or your immediate memory is not good) but it also helps the user find 
out which of her settings differ from the default values (and that's indeed 
something you will forget over time and wonder "why the hell is the defaults 
button enabled").
  
  Hope the extra context helps.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615091 , @davidre wrote:
  
  > The editmarks should probably respect the visibility of the associated 
widget. In this picture I use a invisble lineedit to manage the settings of the 
QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)
  >  F8117874: grafik.png 
  
  
  Very good point!

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns