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
