davidedmundson added a comment.
Which release?
We need to remove user-manager. We're technically in a repo freeze, and this
would involve changing (removing) a repo a breakage of that.
--------
Generally this looks quite neat and tidy. A big simplification +++
INLINE COMMENTS
> kcm.cpp:94
> + KQuickAddons::ConfigModule::save();
> + Q_EMIT apply();
> +}
what's this for?
> kcm.cpp:97
> +
> +QString KCMUser::initializeString(const QString& stringToInitial)
> +{
This is an odd method name.
I don't really understand what it's doing, some sort of title casing?
> kcm.cpp:121
> + QTemporaryFile file;
> + file.setAutoRemove(false);
> + if (file.open()) {
so who cleans this up?
> user.cpp:96
> + }
> + QString trueVal = value.toString();
> + if (value.toString().startsWith(QStringLiteral("file://"))) {
use url.path and url.protocol for this not string manipulation
> usermodel.cpp:72
> + connect(user, &User::dataChanged, [=]() {
> + beginResetModel();
> + endResetModel();
why are we resetting the whole model instead of just dataChanged() on the
relevant row
> usermodel.cpp:78
> + std::sort(m_userList.begin(), m_userList.end(), [](User *lhs, User *) {
> + return lhs->loggedIn();
> + });
The ones not logged in will be sorted randomly.
Also note that if the intention is to have your user at the top, this check
won't suffice as you can have 2 things logged in.
> broulik wrote in usermodel.cpp:96
> You're leaking all the users.
> `qDeleteAll(m_userList)`
this is marked as done, yet I can't see where ?
REPOSITORY
R119 Plasma Desktop
BRANCH
arcpatch-D28154
REVISION DETAIL
https://phabricator.kde.org/D28154
To: cblack, #plasma, #vdg, ngraham
Cc: mart, yurchor, iasensio, meven, crossi, The-Feren-OS-Dev, davidedmundson,
broulik, filipf, ngraham, nicolasfella, zzag, plasma-devel, Orage, LeGast00n,
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
