Add users KCM
Changes PlannedPublic

Authored by cblack on Fri, Mar 20, 12:51 AM.

Details

Reviewers
ngraham
Group Reviewers
Plasma
VDG
Summary

This patch introduces a new users KCM based off of the AccountsService.

Co-authored-by: Nicolas Fella <nicolas.fella@gmx.de>

Test Plan



Diff Detail

Repository
R119 Plasma Desktop
Branch
cblack/userkcm-ng (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24174
Build 24192: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

I'm strongly in favor of removing the autologin option from this KCM. The option only applies to SDDM so it should be in SDDM's settings.

I'm strongly in favor of removing the autologin option from this KCM. The option only applies to SDDM so it should be in SDDM's settings.

πŸ‘

Also, were we going to add a UI to modify a user's group membership here? I seem to recall seeing that on a rough early version of @nicolasfella's version. That's the most urgently needed feature.

Modifiying groups isn't part of the accounts service, and implementing that would require substantial code with the potential for tons of security issuesβ€”I'd rather not try and wade into that territory. Plus, modifying groups is too technical for the kind of user that's using a simple graphical interface like this.

I seem to recall seeing that on a rough early version of @nicolasfella's version. That's the most urgently needed feature.

I'm pretty sure I did no such thing and I also think it's not super important

Couple of nitpicks, mostly coding style and most of that probably comes from me.
Can't say much otherwise since I wrote quite a bit of it

kcms/users/package/contents/ui/main.qml
101

remove prints

kcms/users/src/kcm.cpp
54

We should probably drop the 'ng'

kcms/users/src/kcm.h
28

copy pasta?

46

remove

kcms/users/src/user.cpp
179

newline before {, also elsewhere

kcms/users/src/usercontroller.cpp
53

Not needed I think

kcms/users/src/usermodel.cpp
52

qAsConst(m_userList)

63

Coding style: no space before '>'

The coding style does not specify this, but IMHO it makes sense to apply the rules for parentheses here

67

qCDebug

73

remove or qCDebug

88

If we decide to not have autologin in the KCM then we don't this I guess?

102

const + qAsConst

kcms/users/src/usersessions.h
43

Coding style: *parent
Also elsewhere

broulik added inline comments.
kcms/users/CMakeLists.txt
2

This must match the KAboutData name (which is kcm_user) and what Messages.sh is actually extracting into.

kcms/users/package/contents/ui/ChangePassword.qml
30

You might want to add a FocusScope and then add some key handlers for return and what not

32

One assignment per line, please

36

variant is obsolete, use var.
Perhaps use more specific QtObject type

68

How does that work?

kcms/users/package/contents/ui/CreateUser.qml
52

There's a Kirigami.PasswordField

kcms/users/package/contents/ui/UserDetailsPage.qml
51

No user picture gallery anymore?

70

Start in pictures folder?
I think you can set that by doing

folder: shortcuts.pictures
90

Set a sourceSize to keep it from loading a full size image when user choses an absurdly large image

111

Add context i18nc("Example name", "John Doe")

113

Use onEditingFinished

133

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

141

Looks unused

154

Add i18nc

kcms/users/package/contents/ui/main.qml
35

How about using a ScrollViewKCM instead since all you get as main item is a scrollvivew

54

ListView has a currentIndex concept

55

Can you please namespace most of your imports, I'm having hard time figuring out where all those types come from.

88

Access via model everywhere, please, i.e. model.name

116

Not needed?

121

Connection? :) Also, this id is unused, remove

126

Needs title capitalization

kcms/users/src/kcm.cpp
54

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

61

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

kcms/users/src/user.cpp
23

Unused?

43

Write emit or Q_EMIT before signal calls

92

Why not use QUrl as property type then?

128

You're leaking the old one, if this method is called multiple times

130

m_dbusIface->systemAccount() may block waiting for a reply, not sure how severe that is

138

Use QFileInfo::exists

190

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?

kcms/users/src/user.h
33

Put all in one line

60

Clever, not having a READ but I think it's mandatory?

62

Either add NOTIFY signal or declare CONSTANT

75

Please clean up coding style in both this header and implementation

130

Those all seem to be uninitialized

kcms/users/src/usercontroller.cpp
33

Move to initializer list.
Perhaps also add parent argument, even if it's defaulted to nullptr

39

No.

kcms/users/src/usercontroller.h
36

~UserController() override = default

kcms/users/src/usermodel.cpp
33

Initialize here

52

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.

63

auto is your friend :D

64

Use QDBusPendingReplyWatcher

97

You're leaking all the users.
qDeleteAll(m_userList)

104

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?)

112

There's checkIndex

119

Prefer .at(). (Doesn't really matter here since the method is const)

140

No default case, so we notice when we add new stuff. Just add a return QVariant() at the end of the method

kcms/users/src/usermodel.h
38

Start at Qt::UserRole

40

This one should probably be Qt::DisplayRole

42

This could perhaps be Qt::DecorationRole

61

What's this for?

kcms/users/src/usersessions.h
34

using?

44

~UserSession() override;

Since the new KCM's name (kcm_user) does natch the name of the old one (user_manager) this will require a change in System Settings to actually make it appear. Also for some reason it does not show up to kcmshell list:

The names need to match, otherwise we break any setup that has a manual shortcut to user_manager

I can't see any saving to "~/.face.icon"

this is needed for legacy compatibility with other systems that don't use accounts service.

For the lack of text on Full Name, is there any chance some hint text could appear in the full name text box when the textbox value is empty saying something along the lines of "Full Name" or something?

Also on the subject of Autologin I'm also in favour of removing that checkbox from this KCM and having it only in the SDDM KCM since it only affects SDDM.

cblack updated this revision to Diff 78135.Sat, Mar 21, 3:55 AM
cblack marked 53 inline comments as done.

Address some feedback

cblack added inline comments.Sat, Mar 21, 5:37 AM
kcms/users/package/contents/ui/ChangePassword.qml
68

Holdover from an older version. You can look through the history of https://invent.kde.org/cblack/userkcm-ng/ to see when it actually worked.

kcms/users/src/user.cpp
190

I don't really know how to do async stuff in this context, you would probably have to show me how/link examples.

kcms/users/src/user.h
60

QML doesn't seem to complain, soo.... Β―\_(ツ)_/Β―

crossi added a subscriber: crossi.Mon, Mar 23, 2:38 PM

Regarding groups, obviously if it isn't a part of this patch, then it isn't a part of this patch. I don't think it makes sense to WONTFIX the idea of it though. I not-infrequenrly wish for a GUI method of changing my user's group membership. Yes, I know how to do it using the CLI, but I find it easier and faster to use a GUI most of the time.

Regarding removing the auto-login checkbox, I understand that this isn't the most technically appropriate place for it and it's already available somewhere else. I won't block the patch on this, but I do think it's nice to have it here too. This is IMO a pretty logical place for people to look for the setting.


anywaybacktothepatchitself...

Thanks for using a more conventional list style. I'm still not super happy about the list item for the current user being bigger though. It just feels kinda weird IMO:

The emergingstandard we're moving towards is making the text bold fur the current/default item in a list, and I think this qualifies.

Also as you can see, the list item for my account doesn't display the user picture that's visible on the account page for some reason.

I also noticed that the width threshold for switching to two-column view is so close to the minimum size of System Settings' window that some weird effects are produced:

The threshold should probably be reduced a bit so that you always see the two-column view in the System Settings app itself when the app is using its default/minimum window size. Same for when opened in KCMShell, or else it opens really small and my name gets cut off:

Various other KCMs set a more appropriate default size in KCMShell by setting their own implicitWidth and implicitHeight values on the root item.

Oh and check this out:

One last UI regression I noticed: you can no longer set the account type during account creation; you now need to create the account, then select it, and change the type there. You should still be able to choose the type during account creation

kcms/users/package/metadata.desktop
5

Don't change the icon. The old was appropriately using preferences-system-users

kcms/users/user_manager.desktop
4

ditto

cblack updated this revision to Diff 78364.Tue, Mar 24, 2:44 PM

Use better icon

cblack updated this revision to Diff 78387.Tue, Mar 24, 6:14 PM
cblack marked 6 inline comments as done.

Address feedback and fix issues

cblack updated this revision to Diff 78394.Tue, Mar 24, 7:23 PM

Add boilerplate code for a KAuth helper to prepare for adding groups support

cblack planned changes to this revision.Tue, Mar 24, 7:24 PM

FWIW deleting a user doesn't work for me. When I click the Delete User button, in the console I see:

file:///home/nate/kde/usr/share/kpackage/kcms/user_manager/contents/ui/UserDetailsPage.qml:177: TypeError: Cannot read property 'uid' of undefined

Also I notice that the users list no longer indicates what type of account the user has; that information must now be found by clicking on each user and looking at their details page. A third line could probably be used to re-add this information. It would make the list items taller, but that's fine, as long as *all* of them are taller. :)