broulik added inline comments. INLINE COMMENTS
> CMakeLists.txt:1 > +add_definitions(-DTRANSLATION_DOMAIN=\"plasmauser-kcm\") > + This must match the `KAboutData` name (which is `kcm_user`) and what `Messages.sh` is actually extracting into. > ChangePassword.qml:29 > + > +Kirigami.OverlaySheet { > + function openAndClear() { You might want to add a `FocusScope` and then add some key handlers for return and what not > ChangePassword.qml:31 > + function openAndClear() { > + verifyField.text = passwordField.text = "" > + this.open() One assignment per line, please > ChangePassword.qml:35 > + > + property variant account > + `variant` is obsolete, use `var`. Perhaps use more specific `QtObject` type > ChangePassword.qml:67 > + user.password = passwordField.text > + ChangePassword.window.hide() > + } How does that work? > CreateUser.qml:51 > + } > + TextField { > + id: passwordField There's a `Kirigami.PasswordField` > UserDetailsPage.qml:50 > + > + Menu { > + id: imagePickerMenu No user picture gallery anymore? > UserDetailsPage.qml:69 > + > + FileDialog { > + id: fileDialog Start in pictures folder? I think you can set that by doing folder: shortcuts.pictures > UserDetailsPage.qml:89 > + > + Image { > + source: user.face Set a `sourceSize` to keep it from loading a full size image when user choses an absurdly large image > UserDetailsPage.qml:110 > + text: user.realName > + placeholderText: i18n("John Doe") > + Add context `i18nc("Example name", "John Doe")` > UserDetailsPage.qml:112 > + > + onTextChanged: { > + kcm.needsSave = true Use `onEditingFinished` > UserDetailsPage.qml:132 > + > + model: [ > + i18n("Standard"), I'd prefer if you did textRole: "label" model: [ {type: "standard", label: i18n("Standard")}, {type: "admin", label: i18n("Administrator")} ] and then in the place where you check admin check for `type` rather than `index === 1` > UserDetailsPage.qml:140 > + currentIndex: user.administrator ? 1 : 0 > + property bool isAdmin: false > + Looks unused > UserDetailsPage.qml:153 > + text: user.email > + placeholderText: i18n("john....@kde.org") > + Kirigami.FormData.label: i18n("Email address:") Add `i18nc` > main.qml:34 > + > +KCM.SimpleKCM { > + id: root How about using a `ScrollViewKCM` instead since all you get as main item is a scrollvivew > main.qml:53 > + id: userList > + property int idx: -1; > + model: UserModel `ListView` has a `currentIndex` concept > main.qml:54 > + property int idx: -1; > + model: UserModel > + spacing: Kirigami.Units.largeSpacing Can you please namespace most of your imports, I'm having hard time figuring out where all those types come from. > main.qml:87 > + Label { > + text: name > + opacity: 0.8 Access via `model` everywhere, please, i.e. `model.name` > main.qml:115 > + Item { > + Layout.alignment: Qt.AlignLeft > + Layout.fillWidth: true Not needed? > main.qml:120 > + Button { > + id: addConnectionButton > + Layout.alignment: Qt.AlignRight Connection? :) Also, this `id` is unused, remove > main.qml:125 > + > + text: i18n("Add new user") > + Needs title capitalization > kcm.cpp:53 > + > + qmlRegisterSingletonType<UserModel>("org.kde.userng", 1, 0, "UserModel", > [](QQmlEngine *engine, QJSEngine *scriptEngine) -> QObject* { > + Q_UNUSED(engine) This doesn't need to be exposed as a full-fledged QML Type. Just make this a `CONSTANT` `Q_PROPERTY` on this class and access via `kcm.userModel` > kcm.cpp:60 > + }); > + qmlRegisterSingletonType<UserController>("org.kde.userng", 1, 0, > "UserController", [](QQmlEngine *engine, QJSEngine *scriptEngine) -> QObject* > { > + Q_UNUSED(engine) Also doesn't need to be a fully-fledged qml type. Just make `createUser` and `deleteUser` `Q_INVOKABLE` on this class and call them via `kcm.createUser` > user.cpp:22 > + > +#include <QDebug> > +#include <user.h> Unused? > user.cpp:42 > + mUid = value; > + uidChanged(value); > +} Write `emit` or `Q_EMIT` before signal calls > user.cpp:91 > + > + QString face = QUrl(value).toLocalFile(); > + Why not use `QUrl` as property type then? > user.cpp:127 > +void User::setPath(const QDBusObjectPath &path) { > + m_dbusIface = new > OrgFreedesktopAccountsUserInterface(QStringLiteral("org.freedesktop.Accounts"), > path.path(), QDBusConnection::systemBus(), this); > + You're leaking the old one, if this method is called multiple times > user.cpp:129 > + > + if (!m_dbusIface->isValid() || m_dbusIface->lastError().isValid() || > m_dbusIface->systemAccount()) { > + return; `m_dbusIface->systemAccount()` may block waiting for a reply, not sure how severe that is > user.cpp:137 > + mFace = m_dbusIface->iconFile(); > + mFaceValid = QFile::exists(mFace); > + mRealName = m_dbusIface->realName(); Use `QFileInfo::exists` > user.cpp:189 > +{ > + m_dbusIface->SetEmail(mEmail).waitForFinished(); > + m_dbusIface->SetRealName(mRealName).waitForFinished(); Can you make this async? Make an `ApplyJob`, maybe? You're doing four blocking calls in a row, which may also spawn a polkit prompt? > user.h:32 > + > + Q_PROPERTY(int uid READ uid > + WRITE setUid Put all in one line > user.h:59 > + > + Q_PROPERTY(QString password WRITE setPassword) > + Clever, not having a `READ` but I think it's mandatory? > user.h:61 > + > + Q_PROPERTY(bool loggedIn READ loggedIn) > + Either add `NOTIFY` signal or declare `CONSTANT` > user.h:74 > +public: > + explicit User(QObject* parent = nullptr); > + Please clean up coding style in both this header and implementation > user.h:129 > +private: > + int mUid; > + QString mName; Those all seem to be uninitialized > usercontroller.cpp:32 > +{ > + m_dbusInterface = new > OrgFreedesktopAccountsInterface(QStringLiteral("org.freedesktop.Accounts"), > QStringLiteral("/org/freedesktop/Accounts"), QDBusConnection::systemBus(), > this); > +} Move to initializer list. Perhaps also add `parent` argument, even if it's defaulted to `nullptr` > usercontroller.cpp:38 > + QDBusPendingReply<QDBusObjectPath> reply = > m_dbusInterface->CreateUser(name, realName, 0); > + reply.waitForFinished(); > + if (reply.isValid()) { No. > usercontroller.h:35 > + explicit UserController(); > + virtual ~UserController() = default; > + `~UserController() override = default` > usermodel.cpp:32 > + : QAbstractListModel(parent) > + , m_dbusInterface(nullptr) > +{ Initialize here > usermodel.cpp:51 > + connect(m_dbusInterface, &OrgFreedesktopAccountsInterface::UserDeleted, > this, [this](const QDBusObjectPath &path) { > + for (User* user: m_userList) { > + if (user->path() == path) { Use algorithms, `std::remove_if` (which despite its name just moves the items you want to remove to the end of the list) in conunction with `erase` which then actually removes them. But surely don't mutate a list while you're iterating it. However, you can only ever have one match in this instance, right? So perhaps `std::find_if` and then just remove that one item. > usermodel.cpp:62 > + > + QDBusPendingReply <QList <QDBusObjectPath > > reply = > m_dbusInterface->ListCachedUsers(); > + reply.waitForFinished(); `auto` is your friend :D > usermodel.cpp:63 > + QDBusPendingReply <QList <QDBusObjectPath > > reply = > m_dbusInterface->ListCachedUsers(); > + reply.waitForFinished(); > + Use `QDBusPendingReplyWatcher` > usermodel.cpp:96 > +UserModel::~UserModel() > +{ > +} You're leaking all the users. `qDeleteAll(m_userList)` > usermodel.cpp:103 > + if (user->loggedIn()) { > + return user; > + } Mind that you're returning a parent-less `QObject` from a `Q_INVOKABLE` which means the QML gc will adopt it and delete it as it sees fit yielding funny results. Either parent the `User` objects to `this` (then you also don't have to delete them manually in the destructor), or `QQmlEngine::setObjectOwnership(user, QQmlEngine::CppOwnership)` (However, this doesn't seem to be used?) > usermodel.cpp:111 > +{ > + if (!index.isValid() > + || index.row() < 0 There's `checkIndex` > usermodel.cpp:118 > + > + User *user = m_userList[index.row()]; > + Prefer `.at()`. (Doesn't really matter here since the method is `const`) > usermodel.cpp:139 > + return user->loggedIn(); > + default: > + return QVariant(); No `default` case, so we notice when we add new stuff. Just add a `return QVariant()` at the end of the method > usermodel.h:37 > + enum ModelRoles { > + UidRole, > + NameRole, Start at `Qt::UserRole` > usermodel.h:39 > + NameRole, > + RealNameRole, > + EmailRole, This one should probably be `Qt::DisplayRole` > usermodel.h:41 > + EmailRole, > + FaceRole, > + FaceValidRole, This could perhaps be `Qt::DecorationRole` > usermodel.h:60 > +Q_SIGNALS: > + void rowsChanged(); > + What's this for? > usersessions.h:33 > + > +typedef QList<UserInfo> UserInfoList; > +Q_DECLARE_METATYPE(UserInfoList) `using`? > usersessions.h:43 > + explicit UserSession(QObject* parent = nullptr); > + virtual ~UserSession(); > + `~UserSession() override;` REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28154 To: cblack, #plasma, #vdg, ngraham Cc: broulik, filipf, ngraham, nicolasfella, zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart