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
