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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200227/c3aeacf5/attachment.html>