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