D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-03-24 Thread Ahmad Samir 
ahmadsamir added a comment.


  Should be fixed by D28232 .

REPOSITORY
  R119 Plasma Desktop

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

To: bport, #plasma, meven, crossi, ervin
Cc: usta, ahmadsamir, 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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-03-23 Thread Ömer Fadıl Usta 
usta added a comment.


  Not just include problem but also got a problem about new Qt version :
  
  [ 64%] Linking CXX shared module ../../bin/kcm_notifications.so
  /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp: In 
member function ‘void SonnetSpellCheckingModule::stateChanged()’:
  /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:58:74: 
error: no matching function for call to 
‘QSet::QSet(QList::iterator, QList::iterator)’
  
58 | QSet refIgnoreSet(refIgnoreList.begin(), 
refIgnoreList.end());
   |
  ^
  
  In file included from /usr/include/qt5/QtCore/QSet:1,
  
from 
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:
  
  /usr/include/qt5/QtCore/qset.h:61:12: note: candidate: 
‘QSet::QSet(std::initializer_list<_Tp>) [with T = QString]’
  
61 | inline QSet(std::initializer_list list)
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:61:12: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet::QSet() [with 
T = QString]’
  
59 | inline QSet() Q_DECL_NOTHROW {}
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:59:12: note:   candidate expects 0 arguments, 
2 provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(const QSet&)’
  
54 | class QSet
   |   ^~~~
  
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(QSet&&)’
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:59:86: 
error: no matching function for call to 
‘QSet::QSet(QList::iterator, QList::iterator)’
  
59 | QSet currentIgnoreSet(currentIgnoreList.begin(), 
currentIgnoreList.end());
   |
  ^
  
  In file included from /usr/include/qt5/QtCore/QSet:1,
  
from 
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:
  
  /usr/include/qt5/QtCore/qset.h:61:12: note: candidate: 
‘QSet::QSet(std::initializer_list<_Tp>) [with T = QString]’
  
61 | inline QSet(std::initializer_list list)
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:61:12: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet::QSet() [with 
T = QString]’
  
59 | inline QSet() Q_DECL_NOTHROW {}
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:59:12: note:   candidate expects 0 arguments, 
2 provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(const QSet&)’
  
54 | class QSet
   |   ^~~~
  
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(QSet&&)’
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:60:86: 
error: no matching function for call to 
‘QSet::QSet(QList::iterator, QList::iterator)’
  
60 | QSet defaultIgnoreSet(defaultIgnoreList.begin(), 
defaultIgnoreList.end());
   |
  ^
  
  In file included from /usr/include/qt5/QtCore/QSet:1,
  
from 
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:
  
  /usr/include/qt5/QtCore/qset.h:61:12: note: candidate: 
‘QSet::QSet(std::initializer_list<_Tp>) [with T = QString]’
  
61 | inline QSet(std::initializer_list list)
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:61:12: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet::QSet() [with 
T = QString]’
  
59 | inline QSet() Q_DECL_NOTHROW {}
   |^~~~
  
  /usr/include/qt5/QtCore/qset.h:59:12: note:   candidate expects 0 arguments, 
2 provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(const QSet&)’
  
54 | class QSet
   |   ^~~~
  
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  /usr/include/qt5/QtCore/qset.h:54:7: note: candidate: 
‘QSet::QSet(QSet&&)’
  /usr/include/qt5/QtCore/qset.h:54:7: note:   candidate expects 1 argument, 2 
provided
  
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:67:107: 
error: no matching function for call to 
‘QSet::QSet(QList::iterator, QList::iterator)’
  
67 | QSet 
refPreferredLanguages(refPreferredLanguagesList.begin(), 
refPreferredLanguagesList.end());
   |

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-03-23 Thread Ahmad Samir 
ahmadsamir added a comment.


  This failed to build on the CI 
https://build.kde.org/job/Plasma/job/plasma-desktop/, I guess you'll need to 
bump the min required version of KF5 to 5.69.0.

REPOSITORY
  R119 Plasma Desktop

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

To: bport, #plasma, meven, crossi, ervin
Cc: ahmadsamir, 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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-03-23 Thread Nathaniel Graham 
ngraham added a comment.


  Looks like this failed in the CI.
  
[2020-03-23T17:18:47.106Z] 
/home/jenkins/workspace/Plasma/plasma-desktop/kf5-qt5 
SUSEQt5.12/kcms/spellchecking/spellcheckingskeleton.cpp:24:10: fatal error: 
Sonnet/ConfigView: No such file or directory
[2020-03-23T17:18:47.106Z] 24 | #include 
[2020-03-23T17:18:47.106Z] | ^~~

REPOSITORY
  R119 Plasma Desktop

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

To: bport, #plasma, meven, crossi, ervin
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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-03-23 Thread Benjamin Port 
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:38a874ab82aa: [KCM Spellchecking] port to 
KPropertySkeletonItem (authored by bport).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27503?vs=77267=78297

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

AFFECTED FILES
  kcms/spellchecking/CMakeLists.txt
  kcms/spellchecking/spellchecking.cpp
  kcms/spellchecking/spellchecking.h
  kcms/spellchecking/spellcheckingskeleton.cpp
  kcms/spellchecking/spellcheckingskeleton.h

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

2020-03-09 Thread Benjamin Port 
bport updated this revision to Diff 77267.
bport added a comment.


  fix

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27503?vs=76516=77267

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

AFFECTED FILES
  kcms/spellchecking/CMakeLists.txt
  kcms/spellchecking/spellchecking.cpp
  kcms/spellchecking/spellchecking.h
  kcms/spellchecking/spellcheckingskeleton.cpp
  kcms/spellchecking/spellcheckingskeleton.h

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

2020-02-27 Thread 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

2020-02-27 Thread Benjamin Port 
bport added inline comments.

INLINE COMMENTS

> ervin wrote in spellchecking.cpp:88
> 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.

Yes, or I can probably also use item like before, and that will be handled for 
me

> ervin wrote in spellchecking.cpp:118
> 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.

no because we hold loaded data if we do that, apply button will be deactivated 
after clicking default

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

2020-02-27 Thread 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

2020-02-27 Thread Benjamin Port 
bport added inline comments.

INLINE COMMENTS

> ervin wrote in spellchecking.cpp:88
> Are you sure this is necessary? I'd expect KCModule::load() to call load() on 
> m_skeleton

no load only do findItem for managed widget on the skeleton

> ervin wrote in spellcheckingskeleton.cpp:51
> I have a doubt, is it really necessary? I'd expect it to work without that 
> line.

Yes is necessary no code is run after that so nobody will take care of saving 
this skeleton.

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

2020-02-27 Thread 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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-02-26 Thread Benjamin Port 
bport updated this revision to Diff 76516.
bport marked 19 inline comments as done.
bport added a comment.


  cleanup

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27503?vs=76385=76516

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

AFFECTED FILES
  kcms/spellchecking/CMakeLists.txt
  kcms/spellchecking/spellchecking.cpp
  kcms/spellchecking/spellchecking.h
  kcms/spellchecking/spellcheckingskeleton.cpp
  kcms/spellchecking/spellcheckingskeleton.h

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

2020-02-25 Thread 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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-02-25 Thread Benjamin Port 
bport updated this revision to Diff 76385.
bport added a comment.


  Take change from sonnet patch in consideration and other ervin feedback

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27503?vs=75998=76385

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

AFFECTED FILES
  kcms/spellchecking/CMakeLists.txt
  kcms/spellchecking/spellchecking.cpp
  kcms/spellchecking/spellchecking.h
  kcms/spellchecking/spellcheckingskeleton.cpp
  kcms/spellchecking/spellcheckingskeleton.h

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

2020-02-24 Thread 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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

2020-02-19 Thread Benjamin Port 
bport created this revision.
bport added reviewers: Plasma, meven, crossi, ervin.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bport requested review of this revision.

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/spellchecking/CMakeLists.txt
  kcms/spellchecking/spellchecking.cpp
  kcms/spellchecking/spellchecking.h
  kcms/spellchecking/spellcheckingskeleton.cpp
  kcms/spellchecking/spellcheckingskeleton.h

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