D27502: Create ConfigView an unmanaged ConfigWidget

2020-03-23 Thread Benjamin Port
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

2020-03-16 Thread Kevin Ottens
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

2020-03-09 Thread Benjamin Port
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

2020-02-27 Thread Kevin Ottens
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

2020-02-26 Thread Benjamin Port
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

2020-02-25 Thread Kevin Ottens
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

2020-02-25 Thread Benjamin Port
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

2020-02-25 Thread Benjamin Port
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

2020-02-24 Thread 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 
> -#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

2020-02-19 Thread Méven Car
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

2020-02-19 Thread Benjamin Port
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

2020-02-19 Thread Benjamin Port
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