D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

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 <QMap>
> -#include <QLocale>
>  #include <QSettings>

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 <QLocale>
> +#include <QStringList>

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 &operator=(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 &operator=(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 &ignores);

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 <QWidget>`

> 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 &ignoreList);
> +    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 &text);

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