Differential D28192 Diff 78191 runners/dictionary/dictionaryrunner_config.cpp

Changeset View

Standalone View

View Options

runners/dictionary/dictionaryrunner_config.cpp

1/* 1​/*
2 * Copyright (C) 2010, 2012 Jason A. Donenfeld <Jason@zx2c4.com> 2​ * Copyright (C) 2010, 2012 Jason A. Donenfeld <Jason@zx2c4.com>
3​ * Copyright (C) 2020 Alexander Lohnau <alexander.lohnau@gmx.de>
3 */ 4​ */
4 5​
5#include "dictionaryrunner_config.h" 6​#include "dictionaryrunner_config.h"
7​#include "config_keys.h"
6#include <KRunner/AbstractRunner> 8​#include <KRunner/AbstractRunner>
7#include <QFormLayout> 9​#include <QFormLayout>
8#include <QLineEdit> 10​#include <QLineEdit>
9#include <KSharedConfig> 11​#include <KSharedConfig>
10#include <klocalizedstring.h> 12​#include <KLocalizedString>
11#include <KPluginFactory> 13​#include <KPluginFactory>
12 14​
13K_PLUGIN_FACTORY(DictionaryRunnerConfigFactory, 15​K_PLUGIN_FACTORY(DictionaryRunnerConfigFactory,
14 registerPlugin<DictionaryRunnerConfig>(QStringLiteral("kcm_krunner_dictionary"));) 16​ registerPlugin<DictionaryRunnerConfig>(QStringLiteral("kcm_krunner_dictionary"));)
15 17​
16DictionaryRunnerConfig::DictionaryRunnerConfig(QWidget* parent, const QVariantList& args) : 18​DictionaryRunnerConfig::DictionaryRunnerConfig(QWidget* parent, const QVariantList& args) :
17 KCModule(parent, args) 19​ KCModule(parent, args)
18{ 20​{
19 QFormLayout *layout = new QFormLayout; 21​ auto *layout = new QFormLayout;
20 m_triggerWord = new QLineEdit; 22​ m_triggerWord = new QLineEdit;
21 layout->addRow(i18nc("@label:textbox", "Trigger word:"), m_triggerWord); 23​ layout->addRow(i18nc("@label:textbox", "Trigger word:"), m_triggerWord);
22 setLayout(layout); 24​ setLayout(layout);
23 connect(m_triggerWord, SIGNAL(textChanged(QString)), this, SLOT(changed())); 25​ connect(m_triggerWord, &QLineEdit::textChanged, this, &DictionaryRunnerConfig::markAsChanged);
24 load(); ​
sitterUnsubmitted
Not Done

I wouldn't remove this without good reason.

Without this blocking load() call the UI gets set up and shown and only then the data gets loaded in a kinda async fashion. That is cool, but only iff the KCM is actually equipped for handling the intermediate time frame with a "I'm still loading" UI state (busyindicator or something; or is completely empty at least). If that is not the case then relying on async load will result in the KCM appearing in an incomplete state until the load() is run which may take a noticeable amount of time depending on system performance. So, when load() is doing trivial/cheap work it makes a whole lot of sense to call it blockingly form the ctor.

sitter: I wouldn't remove this without good reason. Without this blocking load() call the UI gets set…
alexAuthorUnsubmitted
Not Done

Thanks for the detailed explanation.
Calling the method twice seemed weird to me but as you have said the load method does only cheap work and it should not matter that much.

alex: Thanks for the detailed explanation. Calling the method twice seemed weird to me but as you…
25} 26​}
26 27​
27void DictionaryRunnerConfig::load() 28​void DictionaryRunnerConfig::load()
28{ 29​{
29 KCModule::load(); 30​ KConfigGroup grp = KSharedConfig::openConfig(QStringLiteral("krunnerrc"))->group("Runners").group("Dictionary");
sitterUnsubmitted
Not Done

Could you elaborate on why you removed the baseclass calls for load/save/defaults please?

sitter: Could you elaborate on why you removed the baseclass calls for load/save/defaults please?
alexAuthorUnsubmitted
Not Done

The baseclass implementations for these methods handle the KConfigDialogManager widgets and the corresponding changed signals.
I removed them, because the KConfigDialogManager class is not used in this kcm and the signals are manually handled.

alex: The baseclass implementations for these methods handle the KConfigDialogManager widgets and the…
sitterUnsubmitted
Not Done

That's an implementation detail though, is it not? From the outside we shouldn't make assumption about what the implementation does unless the documentation says what we can assume.
Today the baseclass may be useless, in 10 years it may not be.

Long-winded way of saying that I would leave the base class calls in. If nothing else it's at least better form in terms of API contracts.

sitter: That's an implementation detail though, is it not? From the outside we shouldn't make…
alexAuthorUnsubmitted
Not Done

That makes sense but one question: The doc says: ...However, if you for some reason reimplement it and also are using KConfigXT, you must call this function, does this mean we can assume that the base class is not needed?

PS: In this case it is not very relevant but I would like to understand concept for future patches 😃.

alex: That makes sense but one question: The doc says: `...However, if you for some reason…
sitterUnsubmitted
Not Done

That line makes no assertions about what to do when you are not using KConfigXT. We don't really have consistent language for docs unfortunately but generally unless the docs explicitly say you must or you must not, it's usually best to assume that you should go with best practice.

In this particular case the fact that the docs say you must call the base when using kconfigxt could just mean that someone forgot about forwarding the call and then stumbled over bugs and took the time to add a warning for future generations to not repeat their mistake. It doesn't necessarily mean that the list of musts is comprehensive and exhaustive. It certainly doesn't mean that implicit requirements will remain the same over time. And along that line, consider the two scenarios:

in 10 years

  • a new requirement is added where you must forward the call to the base. say a sound plays on every save and that is implemented inside KCModule. if you do not forward the call this singular KCM will be misbehaving and someone has to file a bug and a dev has to go figure out that the calls aren't being forwarded
  • a new requirement is added where you must not forward the call (terribly unlikely) it's easy for the baseclass to know when that requirement was violated and Q_ASSERT or print a qwarning. at that point you'll still have a bug on your hand but the library can make provisions for that bug to not be actually a problem

or simply put: it's easier to deal with useless/unintended calls than with entirely missing calls. if you don't call a library it can't tell you about failed runtime assertions and the like.

sitter: That line makes no assertions about what to do when you are not using KConfigXT. We don't…
30 KSharedConfig::Ptr cfg = KSharedConfig::openConfig(QLatin1String("krunnerrc")); ​
31 KConfigGroup grp = cfg->group("Runners"); ​
32 grp = KConfigGroup(&grp, "Dictionary"); ​
33 m_triggerWord->setText(grp.readEntry(CONFIG_TRIGGERWORD, i18nc("Trigger word before word to define", "define"))); 31​ m_triggerWord->setText(grp.readEntry(CONFIG_TRIGGERWORD, i18nc("Trigger word before word to define", "define")));
34 emit changed(false); 32​ emit changed(false);
35} 33​}
36 34​
37void DictionaryRunnerConfig::save() 35​void DictionaryRunnerConfig::save()
38{ 36​{
39 KCModule::save(); 37​ KConfigGroup grp = KSharedConfig::openConfig(QStringLiteral("krunnerrc"))->group("Runners").group("Dictionary");
40 KSharedConfig::Ptr cfg = KSharedConfig::openConfig(QLatin1String("krunnerrc")); ​
41 KConfigGroup grp = cfg->group("Runners"); ​
42 grp = KConfigGroup(&grp, "Dictionary"); ​
43 grp.writeEntry(CONFIG_TRIGGERWORD, m_triggerWord->text()); 38​ grp.writeEntry(CONFIG_TRIGGERWORD, m_triggerWord->text());
44 emit changed(false); 39​ emit changed(false);
45} 40​}
46 41​
47void DictionaryRunnerConfig::defaults() 42​void DictionaryRunnerConfig::defaults()
48{ 43​{
49 KCModule::defaults(); ​
50 m_triggerWord->setText(i18nc("Trigger word before word to define", "define")); 44​ m_triggerWord->setText(i18nc("Trigger word before word to define", "define"));
51 emit changed(true); 45​ emit changed(true);
52} 46​}
53 47​
54#include "dictionaryrunner_config.moc" 48​#include "dictionaryrunner_config.moc"