Details

Reviewers

meven
crossi
ervin

Group Reviewers

Plasma

Commits

R119:38a874ab82aa: [KCM Spellchecking] port to KPropertySkeletonItem

Diff Detail

Repository

R119 Plasma Desktop

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 22994
Build 23012: arc lint + arc unit

bport created this revision.Feb 19 2020, 2:03 PM

Restricted Application added a project: Plasma. · View Herald TranscriptFeb 19 2020, 2:03 PM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

bport requested review of this revision.Feb 19 2020, 2:03 PM

Harbormaster completed remote builds in B22681: Diff 75998.Feb 19 2020, 2:03 PM

bport added a dependency: D27502: Create ConfigView an unmanaged ConfigWidget.Feb 19 2020, 2:04 PM

ervin requested changes to this revision.Feb 24 2020, 3:53 PM

ervin added inline comments.

kcms/spellchecking/spellchecking.cpp
37	Good opportunity to switch to the camel case includes?
38	Either phab display is stupid of the indentation on that line and the next is wrong
61	Code in here feels rather inelegant, my brain is too mushy right now to propose something though... maybe once it reboots...
86	This warrants a comment, because with the addConfig call one would expect this to be unnecessary.
96–109	Ditto
114–118	Ditto
kcms/spellchecking/spellchecking.h
55	nitpick: we usually find methods before variables
kcms/spellchecking/spellcheckingskeleton.cpp
44	If this needs to be kepts for some reason, please use `= default` instead
kcms/spellchecking/spellcheckingskeleton.h
38	Passing the widget in here feels very much wrong (layer violation and all that). I'd also expect it to not receive the settings object but to create it internally (although that's a less major issue).
39	Probably unnecessary

This revision now requires changes to proceed.Feb 24 2020, 3:53 PM

Take change from sonnet patch in consideration and other ervin feedback

Harbormaster completed remote builds in B22916: Diff 76385.Feb 25 2020, 3:52 PM

ervin requested changes to this revision.Feb 25 2020, 5:08 PM

ervin added inline comments.

kcms/spellchecking/spellchecking.cpp
55 ↗	(On Diff #76385)	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?
61 ↗	(On Diff #76385)	this is: unmanagedChangeState \|= currentValue != referenceValue;
64 ↗	(On Diff #76385)	this is: unmanagedDefaultState &= currentValue == defaultValue
74 ↗	(On Diff #76385)	see above
77 ↗	(On Diff #76385)	see above
82 ↗	(On Diff #76385)	see above
85 ↗	(On Diff #76385)	see above
kcms/spellchecking/spellchecking.h
55	This still applies
kcms/spellchecking/spellcheckingskeleton.cpp
38 ↗	(On Diff #76385)	I'd expect this to update m_preferredLanguages, m_ignoreList and m_defaultLanguage otherwise we could have situations where things get out of sync. Also I'd expect to see usrSetDefaults() to be overridden as well.

This revision now requires changes to proceed.Feb 25 2020, 5:08 PM

cleanup

Harbormaster completed remote builds in B22994: Diff 76516.Feb 27 2020, 7:37 AM

ervin requested changes to this revision.Feb 27 2020, 9:00 AM

ervin added inline comments.

kcms/spellchecking/spellchecking.cpp
43		I notice it only now, but those two `new` would be better done in the ctor initialization list.
88		Are you sure this is necessary? I'd expect KCModule::load() to call load() on m_skeleton
118		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.
55 ↗	(On Diff #76385)	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.
kcms/spellchecking/spellcheckingskeleton.cpp
27	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.
51	I have a doubt, is it really necessary? I'd expect it to work without that line.

This revision now requires changes to proceed.Feb 27 2020, 9:00 AM

bport added inline comments.Feb 27 2020, 10:52 AM

kcms/spellchecking/spellchecking.cpp
88	no load only do findItem for managed widget on the skeleton
kcms/spellchecking/spellcheckingskeleton.cpp
51	Yes is necessary no code is run after that so nobody will take care of saving this skeleton.

ervin added inline comments.Feb 27 2020, 11:13 AM

kcms/spellchecking/spellchecking.cpp
88	Sure but still, the skeleton is expected to be already be in a "loaded" state at that point, that's why it works for the items (otherwise a findItem wouldn't be enough, it'd have to call readConfig on them or load on the whole skeleton, and it doesn't). I think the problem is more that in the ctor of the skeleton you initialize the items just fine but you don't initialize the extra properties you need a call to usrRead() at the end of that ctor.
kcms/spellchecking/spellcheckingskeleton.cpp
51	Ah right it does the writeConfig calls first... there's really some "interesting" design choices in KCoreConfigSkeleton...

bport added inline comments.Feb 27 2020, 11:49 AM

kcms/spellchecking/spellchecking.cpp
88	Yes, or I can probably also use item like before, and that will be handled for me
118	no because we hold loaded data if we do that, apply button will be deactivated after clicking default

ervin added inline comments.Feb 27 2020, 1:00 PM

kcms/spellchecking/spellchecking.cpp
88	That would create other issues I think (not fully sure TBH). Well in any case that's an extra level of indirection we don't really need (and it's already very confusing as is). Adding the missing usrRead() in the skeleton ctor is clearly easier IMO.
118	sigh OK... KConfigDialogManager is so confusing each time...

fix

Harbormaster completed remote builds in B23454: Diff 77267.Mar 9 2020, 12:06 PM

ervin accepted this revision.Mar 16 2020, 3:27 PM

This revision is now accepted and ready to land.Mar 16 2020, 3:27 PM

alex mentioned this in D26904: Use Qt5 syntax for signal, rename files.Mar 23 2020, 7:34 AM

Closed by commit R119:38a874ab82aa: [KCM Spellchecking] port to KPropertySkeletonItem (authored by bport). · Explain WhyMar 23 2020, 5:13 PM

This revision was automatically updated to reflect the committed changes.

Looks like this failed in the CI.

[2020-03-23T17:18:47.106Z] /home/jenkins/workspace/Plasma/plasma-desktop/kf5-qt5 SUSEQt5.12/kcms/spellchecking/spellcheckingskeleton.cpp:24:10: fatal error: Sonnet/ConfigView: No such file or directory
[2020-03-23T17:18:47.106Z] 24 | #include <Sonnet/ConfigView>
[2020-03-23T17:18:47.106Z] | ^~~~~~~~~~~~~~~~~~~

This failed to build on the CI https://build.kde.org/job/Plasma/job/plasma-desktop/, I guess you'll need to bump the min required version of KF5 to 5.69.0.

Edit: @ngraham sorry I didn't see your post.

Not just include problem but also got a problem about new Qt version :

[ 64%] Linking CXX shared module ../../bin/kcm_notifications.so
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp: In member function ‘void SonnetSpellCheckingModule::stateChanged()’:
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:58:74: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

58 |     QSet<QString> refIgnoreSet(refIgnoreList.begin(), refIgnoreList.end());
   |                                                                          ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:59:86: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

59 |     QSet<QString> currentIgnoreSet(currentIgnoreList.begin(), currentIgnoreList.end());
   |                                                                                      ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:60:86: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

60 |     QSet<QString> defaultIgnoreSet(defaultIgnoreList.begin(), defaultIgnoreList.end());
   |                                                                                      ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:67:107: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

67 |     QSet<QString> refPreferredLanguages(refPreferredLanguagesList.begin(), refPreferredLanguagesList.end());
   |                                                                                                           ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:68:119: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

68 |     QSet<QString> currentPreferredLanguages(currentPreferredLanguagesList.begin(), currentPreferredLanguagesList.end());
   |                                                                                                                       ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:69:119: error: no matching function for call to ‘QSet<QString>::QSet(QList<QString>::iterator, QList<QString>::iterator)’

69 |     QSet<QString> defaultPreferredLanguages(defaultPreferredLanguagesList.begin(), defaultPreferredLanguagesList.end());
   |                                                                                                                       ^

In file included from /usr/include/qt5/QtCore/QSet:1,

from /home/usta/kde/src/plasma-desktop/kcms/spellchecking/spellchecking.cpp:27:

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate: ‘QSet<T>::QSet(std::initializer_list<_Tp>) [with T = QString]’

61 |     inline QSet(std::initializer_list<T> list)
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:61:12: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:59:12: note: candidate: ‘QSet<T>::QSet() [with T = QString]’

59 |     inline QSet() Q_DECL_NOTHROW {}
   |            ^~~~

/usr/include/qt5/QtCore/qset.h:59:12: note: candidate expects 0 arguments, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(const QSet<QString>&)’

54 | class QSet
   |       ^~~~

/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate: ‘QSet<QString>::QSet(QSet<QString>&&)’
/usr/include/qt5/QtCore/qset.h:54:7: note: candidate expects 1 argument, 2 provided
gmake[2]: * [kcms/spellchecking/CMakeFiles/kcmspellchecking.dir/build.make:89: kcms/spellchecking/CMakeFiles/kcmspellchecking.dir/spellchecking.cpp.o] Error 1
gmake[1]: * [CMakeFiles/Makefile2:7508: kcms/spellchecking/CMakeFiles/kcmspellchecking.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

Should be fixed by D28232.

		Path
M		kcms/spellchecking/CMakeLists.txt (2 lines)
M		kcms/spellchecking/spellchecking.h (27 lines)
M		kcms/spellchecking/spellchecking.cpp (89 lines)
A	M	kcms/spellchecking/spellcheckingskeleton.h (59 lines)
A	M	kcms/spellchecking/spellcheckingskeleton.cpp (90 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	75998	4dfaffd		Feb 19 2020, 2:03 PM	★	★
Diff 2	76385	2a6abf7	Take change from sonnet patch in consideration and other ervin feedback	Feb 25 2020, 3:52 PM	★	★
Diff 3	76516	7af4b04	cleanup	Feb 27 2020, 7:37 AM	★	★
Diff 4	77267	d673153	fix	Mar 9 2020, 12:06 PM	★	★
Diff 5	78297	fff5c04	R119:38a874ab82aac7666377b06234788f909991cf1c	Mar 23 2020, 5:12 PM	★	★

[KCM Spellchecking] port to KPropertySkeletonItem
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 76516

kcms/spellchecking/CMakeLists.txt

kcms/spellchecking/spellchecking.h

kcms/spellchecking/spellchecking.cpp

kcms/spellchecking/spellcheckingskeleton.h

kcms/spellchecking/spellcheckingskeleton.cpp

[KCM Spellchecking] port to KPropertySkeletonItemClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 76516

kcms/spellchecking/CMakeLists.txt

kcms/spellchecking/spellchecking.h

kcms/spellchecking/spellchecking.cpp

kcms/spellchecking/spellcheckingskeleton.h

kcms/spellchecking/spellcheckingskeleton.cpp

[KCM Spellchecking] port to KPropertySkeletonItem
ClosedPublic
Actions

Revision Contents
Changeset List