D27502: Create ConfigView an unmanaged ConfigWidget
This revision was automatically updated to reflect the committed changes. Closed by commit R246:12d1e200f081: Create ConfigView an unmanaged ConfigWidget (authored by bport). REPOSITORY R246 Sonnet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27502?vs=77269=78296 REVISION DETAIL https://phabricator.kde.org/D27502 AFFECTED FILES autotests/test_settings.cpp src/core/CMakeLists.txt src/core/languagefilter.cpp src/core/loader.cpp src/core/loader_p.h src/core/settings.cpp src/core/settings.h src/core/settings_p.h src/core/settingsimpl.cpp src/core/settingsimpl_p.h src/core/speller.cpp src/ui/CMakeLists.txt src/ui/configui.ui src/ui/configview.cpp src/ui/configview.h src/ui/configwidget.cpp src/ui/dialog.cpp src/ui/highlighter.cpp To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
ervin accepted this revision. This revision is now accepted and ready to land. REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D27502 To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport updated this revision to Diff 77269. bport added a comment. rename check to skip and add missing negation for default REPOSITORY R246 Sonnet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27502?vs=76517=77269 REVISION DETAIL https://phabricator.kde.org/D27502 AFFECTED FILES autotests/test_settings.cpp src/core/CMakeLists.txt src/core/languagefilter.cpp src/core/loader.cpp src/core/loader_p.h src/core/settings.cpp src/core/settings.h src/core/settings_p.h src/core/settingsimpl.cpp src/core/settingsimpl_p.h src/core/speller.cpp src/ui/CMakeLists.txt src/ui/configui.ui src/ui/configview.cpp src/ui/configview.h src/ui/configwidget.cpp src/ui/dialog.cpp src/ui/highlighter.cpp To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> settings.cpp:62
>
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setSkipUppercase(bool check)
> {
Rename the parameter to skip as well please
> settingsimpl.cpp:260
> //same defaults are in the default filter (filter.cpp)
> -d->checkUppercase = settings.value(QStringLiteral("checkUppercase"),
> true).toBool();
> -d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"),
> true).toBool();
> +d->checkUppercase = settings.value(QStringLiteral("checkUppercase"),
> Settings::defaultSkipUppercase()).toBool();
> +d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"),
> Settings::defauktSkipRunTogether()).toBool();
I'd expect a negation here... if by default skip is false, check in
settingsimpl should be true (which was the old default).
REPOSITORY
R246 Sonnet
REVISION DETAIL
https://phabricator.kde.org/D27502
To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport updated this revision to Diff 76517. bport added a comment. rename check uppercase to skip uppercase REPOSITORY R246 Sonnet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27502?vs=76384=76517 REVISION DETAIL https://phabricator.kde.org/D27502 AFFECTED FILES autotests/test_settings.cpp src/core/CMakeLists.txt src/core/languagefilter.cpp src/core/loader.cpp src/core/loader_p.h src/core/settings.cpp src/core/settings.h src/core/settings_p.h src/core/settingsimpl.cpp src/core/settingsimpl_p.h src/core/speller.cpp src/ui/CMakeLists.txt src/ui/configui.ui src/ui/configview.cpp src/ui/configview.h src/ui/configwidget.cpp src/ui/dialog.cpp src/ui/highlighter.cpp To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> settings.cpp:62
>
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setCheckUppercase(bool check)
> {
Please rename it to "skip uppercase"...
Otherwise it does the opposite of its name, it's just confusing
> settings.cpp:67
>
> bool Settings::checkUppercase() const
> {
Likewise should be renamed to "skip"
> configview.cpp:43
> +
> +void ConfigViewPrivate::slotSelectionChanged() {
> +
> ui.removeButton->setEnabled(!ui.ignoreListWidget->selectedItems().isEmpty());
Break the line before { please
REPOSITORY
R246 Sonnet
REVISION DETAIL
https://phabricator.kde.org/D27502
To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport updated this revision to Diff 76384. bport added a comment. Rework, not expose anymore settings and loader but expose a new settings class REPOSITORY R246 Sonnet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27502?vs=75997=76384 REVISION DETAIL https://phabricator.kde.org/D27502 AFFECTED FILES autotests/test_settings.cpp src/core/CMakeLists.txt src/core/languagefilter.cpp src/core/loader.cpp src/core/loader_p.h src/core/settings.cpp src/core/settings.h src/core/settings_p.h src/core/settingsimpl.cpp src/core/settingsimpl_p.h src/core/speller.cpp src/ui/CMakeLists.txt src/ui/configui.ui src/ui/configview.cpp src/ui/configview.h src/ui/configwidget.cpp src/ui/dialog.cpp src/ui/highlighter.cpp To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport added inline comments. INLINE COMMENTS > ervin wrote in settings.cpp:109 > This is an odd bit of logic, isn't it? Yes indeed, but what we want is to save false if checked and true if not checked. I moved the logic there in order to work with kconfigxt and with current implementation > ervin wrote in configview.cpp:33 > I'm kind of surprised at this extra widget, can't we get rid of it? I kept same widget logic as ConfigWidget. Ididn't want to change that to ensure we have the same baheviour REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D27502 To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> loader.h:6
> */
> -#ifndef SONNET_LOADER_P_H
> -#define SONNET_LOADER_P_H
> +#ifndef SONNET_LOADER_H
> +#define SONNET_LOADER_H
What's the reason for loader becoming public? If that's mostly out of
convenience (it looks like it is), I'd advise not turning it public, Settings
and ConfigView can use it internally but let's not increase the API surface
even more with Loader.
> settings.cpp:13
> #include
> -#include
> #include
Obviously will reappear here ;-)
> settings.cpp:109
> {
> -if (d->checkUppercase != check) {
> +if (d->checkUppercase != !check) {
> d->modified = true;
This is an odd bit of logic, isn't it?
> settings.cpp:119
> {
> -return d->checkUppercase;
> +return !d->checkUppercase;
> }
Ditto
> settings.h:9
> +
> +#include
> +#include
Please remove that include (it'll become more obvious why you'll be able to do
it with one of the comments below) ;-)
> settings.h:33
> +
> +public:
> +~Settings() override;
Will need a ctor (see other comments) in the form of `explicit Settings(QObject
*parent = nullptr);`
> settings.h:36
> +
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;
This is likely useless now, since we inherit from QObject and QObject is
non-copyable
> settings.h:37
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;
> +
Ditto
> settings.h:79
> +// A static list of KDE specific words that we want to recognize
> +static QStringList defaultIgnoreList()
> +{
Move all the implementations of those static methods to the cpp file.
> settings.h:136
> +private:
> +void readIgnoreList();
> +bool setQuietIgnoreList(const QStringList );
Time to move those to the SettingsPrivate class.
> settings.h:141
> +friend class Loader;
> +explicit Settings(Loader *loader);
> +private:
Might end up disappearing completely, unsure but to be evaluated
> speller.cpp:216
> case CheckUppercase:
> -d->settings->setCheckUppercase(b);
> +d->settings->setCheckUppercase(!b);
> break;
And that negation again? We're negating twice now...
> speller.cpp:232
> case CheckUppercase:
> -return d->settings->checkUppercase();
> +return !d->settings->checkUppercase();
> case SkipRunTogether:
And again...
I wonder what's happening there, it's highly confusing.
> configview.cpp:33
> +layout->setObjectName(QStringLiteral("SonnetConfigUILayout"));
> +d->wdg = new QWidget(this);
> +d->ui.setupUi(d->wdg);
I'm kind of surprised at this extra widget, can't we get rid of it?
> configview.h:17
> +}
> +#include "sonnetui_export.h"
> +
Please move this line just after the `#include `
> configview.h:23
> +{
> +Q_OBJECT
> +public:
You might want to declare a bunch of Q_PROPERTY there
> configview.h:25
> +public:
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();
clients should be a const reference (although I suspect it'll disappear)
parent should be the last parameter and default to nullptr
> configview.h:26
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();
> +
override
> configview.h:30
> +
> +void setPreferredLanguages(const QStringList );
> +QStringList preferredLanguages() const;
All the setters there would make for good public slots
> configview.h:34
> +QString language() const;
> +void setIgnoreList(const QStringList& ignoreList);
> +QStringList ignoreList() const;
Space before & not after
> configview.h:39
> +void setBackgroundCheckingButtonShown(bool);
> +protected Q_SLOTS:
> +void slotIgnoreWordRemoved();
Are they directly called by a subclass outside the library? If not move them to
the dpointer and use Q_PRIVATE_SLOT
> configview.h:42
> +void slotIgnoreWordAdded();
> +private Q_SLOTS:
> +void slotUpdateButton(const QString );
Move to the d pointer and use Q_PRIVATE_SLOT for those
> configview.h:55
> +ConfigViewPrivate *const d;
> +QStringList m_ignoreList;
> +};
Should be in the d pointer
REPOSITORY
R246 Sonnet
REVISION DETAIL
https://phabricator.kde.org/D27502
To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
meven edited the summary of this revision. REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D27502 To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport added a dependent revision: D27503: [KCM Spellchecking] port to KPropertySkeletonItem. REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D27502 To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27502: Create ConfigView an unmanaged ConfigWidget
bport created this revision. bport added reviewers: Plasma, ervin, crossi, meven. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. bport requested review of this revision. REVISION SUMMARY - Allow to manage controller externally - Compatible (for checkbox) with KConfigDialogManager (usage of kcfg_ in widget name) - Expose loader and settings in order to allow tham managed by a KCModule TEST PLAN - automated test - Checked integration with Kate - ConfigView tested with a plasma-desktop patch REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D27502 AFFECTED FILES autotests/test_settings.cpp src/core/CMakeLists.txt src/core/guesslanguage.cpp src/core/languagefilter.cpp src/core/loader.cpp src/core/loader.h src/core/loader_p.h src/core/settings.cpp src/core/settings.h src/core/settings_p.h src/core/speller.cpp src/ui/CMakeLists.txt src/ui/configui.ui src/ui/configview.cpp src/ui/configview.h src/ui/configwidget.cpp src/ui/configwidget.h src/ui/dialog.cpp src/ui/highlighter.cpp To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
