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 <KConfigDialogManager>
> +#include <sonnet/configview.h>
> +#include <sonnet/loader.h>

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