D28154: Add users KCM
Kai Uwe Broulik
noreply at phabricator.kde.org
Fri Mar 20 22:11:04 GMT 2020
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.doe at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200320/4bd5156d/attachment-0001.html>
More information about the Plasma-devel
mailing list