Details
- Reviewers
meven crossi ervin - Group Reviewers
Plasma - Commits
- R119:38a874ab82aa: [KCM Spellchecking] port to KPropertySkeletonItem
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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. | |
95–103 | Ditto | |
108–112 | 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 |
kcms/spellchecking/spellchecking.cpp | ||
---|---|---|
67 | 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? | |
73 | this is: unmanagedChangeState |= currentValue != referenceValue; | |
76 | this is: unmanagedDefaultState &= currentValue == defaultValue | |
86 | see above | |
89 | see above | |
94 | see above | |
97 | see above | |
kcms/spellchecking/spellchecking.h | ||
55 | This still applies | |
kcms/spellchecking/spellcheckingskeleton.cpp | ||
39 | 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. |
kcms/spellchecking/spellchecking.cpp | ||
---|---|---|
43 | I notice it only now, but those two new would be better done in the ctor initialization list. | |
67 | 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: 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. | |
88 | Are you sure this is necessary? I'd expect KCModule::load() to call load() on m_skeleton | |
112 | 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. | |
kcms/spellchecking/spellcheckingskeleton.cpp | ||
28 | 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. | |
52 | I have a doubt, is it really necessary? I'd expect it to work without that line. |
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 | ||
52 | Ah right it does the writeConfig calls first... there's really some "interesting" design choices in KCoreConfigSkeleton... |
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. | |
112 | *sigh* OK... KConfigDialogManager is so confusing each time... |
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....