This patch introduces a new users KCM based off of the AccountsService.
Co-authored-by: Nicolas Fella <nicolas.fella@gmx.de>
Closes T7246
ngraham |
Plasma | |
VDG |
This patch introduces a new users KCM based off of the AccountsService.
Co-authored-by: Nicolas Fella <nicolas.fella@gmx.de>
Closes T7246
No Linters Available |
No Unit Test Coverage |
Buildable 26413 | |
Build 26431: arc lint + arc unit |
Changing a user's avatar and clicking Reset instead of Apply makes the avatar display revert to the default avatar (which I remember you telling me isn't actually possible), not the previous one. This seems inaccurate because the avatar does in fact remain at the last one. It's just the display that's wrong.
Nice, that fixes the last of the avatar issues that I can find.
After you add icons to the items in the keep/delete menu and make it close it with a click outside the menu, I think I'll be done reviewing the UI--other than the multi-column sizing bug that I'd like @mart to take a look at (video #1 in D28154#663205). Nice work.
Great. From my perspective this is ready to go! I'd like a Plasma person to review too, and for @mart or another Plasma person to see if they can reproduce and help figure out the issue described in the first video at D28154#663205. But that's not a hard blocker IMO.
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 +++
kcms/users/src/kcm.cpp | ||
---|---|---|
95 | what's this for? | |
98 | This is an odd method name. I don't really understand what it's doing, some sort of title casing? | |
122 | so who cleans this up? | |
kcms/users/src/user.cpp | ||
97 | use url.path and url.protocol for this not string manipulation | |
kcms/users/src/usermodel.cpp | ||
73 | why are we resetting the whole model instead of just dataChanged() on the relevant row | |
79 | 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. | |
97 | this is marked as done, yet I can't see where ? |
kcms/users/src/kcm.cpp | ||
---|---|---|
95 | used as a hook for QML to know when the KCM needs to apply | |
98 | taking the initials of a user's real name | |
122 | whoever is normally cleaning up tempfiles | |
kcms/users/src/usermodel.cpp | ||
73 | i couldn't figure out how to use those correctly. some help would be appreciated. | |
79 |
loggedIn compares the UID of the user to the UID of the currently running program, which means only one user can be logged in at a time by our metric. | |
97 | users are parented to the model, which will use normal QObject destructor behaviour to clean them |
kcms/users/src/kcm.cpp | ||
---|---|---|
122 | Normally the app creating a temp file would clean it up. A quick fix would be It's sort of leaking the object, but on exit it'll tidy everything up. |
kcms/users/src/kcm.cpp | ||
---|---|---|
98 | Then it should have a different name, like "getInitials", else the next person will be just as confused |
kcms/users/src/kcm.cpp | ||
---|---|---|
122 | that looks like it could possibly run into race conditions, as the applyjob is executed asynchronously and IIRC is able to still be running when everything else, including this, has destructed and removed one of the files it could be using |
kcms/users/src/kcm.cpp | ||
---|---|---|
122 | new QTemporaryFile(qApp) then KJobs have an QEventLoopLocker that will stop qApp from quitting until they finish so you can be sure your applyjob is done. |
I would like to get this into 5.19, which would mean moving the code back into the user-manager repo, or removing it as a dependency for Plasma 5.19 by requesting an exception to the repo freeze.
Please do not get me wrong (I'm fine with the new designs and updating the code and this new KCM is very good), but it would be good to hear some words from @ltoscano on the catalog (and KCM) naming.
It is also evident that all the new KCMs do not have the "Help" button. Is it the new policy?
Thanks in advance for your comments.
Technically kcm should be called kcm<Plasma_version>_<name>. So far the Plasma_version part has been ignored, but at least kcm_ should be there.
I'm not sure about the lack of the button.
I objected to changing the name to avoid breaking compatibility with users and software that have the old name hardcoded, but maybe that was too inflexible. If everyone else is okay with changing the name to the more common style, I'll rescind my objection.
Pretty cool
kcms/users/package/contents/ui/UserDetailsPage.qml | ||
---|---|---|
37 | Use property var, or even property QtObject since User is a QObject | |
83 | i18n | |
97 | Is this a RoundButton bug that it doesn't indicate keyboard focus when I tab to it? | |
98 | Pfp? | |
100 | readonly property | |
112 | I *think* one needs to multiple that with Screen.devicePixelRatio | |
218 | Can you add some way to make Escape close the sheet but not the entire KCM, when run standalone through kcmshell? Hopefully the following is sufficient, otherwise you'd have to mess with FocusScope and the like: Keys.onEscapePressed: { close(); event.accepted = true; } | |
220 | Is this RowLayout needed? | |
288 | Can we not hardcode the list of avatars, please. | |
343 | A tooltip and Accessible.name with the name of the avatar would be nice | |
347 | Is this ColumnLayout needed? | |
362 | What's all of this? I don't see anything in the UI. | |
400 | Not sure if clever or mad :) Can we not just generate a colored rectangle on C++ side? | |
kcms/users/src/usermodel.cpp | ||
113 | I just learned that you need to pass QAbstractItemModel::CheckIndesOptions::IndexIsValid otherwise it only checks for blatant out of bounds indices but not invalid "Null" ones :/ |
kcms/users/package/contents/ui/UserDetailsPage.qml | ||
---|---|---|
218 | FWIW KCMs opened through KRunner are now opened in System Settings so KCMShell is something we're going to have to deal with much less. :) |
kcms/users/package/contents/ui/UserDetailsPage.qml | ||
---|---|---|
218 | Yes, but it's still something to consider. Right now Esc does nothing at all, and neither do arrow keys. This thing needs some FocusScope treatment |
Great, that works!
BTW I think you can change the KCM name to kcm_users as you originally wanted. Sorry for saying otherwise.
So next step would be to shadow user_manager ?
A naive ripgrep gave :
$ rg user_manager .. ../kxmlgui/src/kbugreport.cpp 352: m_process->start(QStringLiteral("kcmshell5"), QStringList() << QStringLiteral("user_manager")); ../systemsettings/sidebar/SidebarMode.cpp 130: item->setData(QUrl(QStringLiteral("kcm:user_manager.desktop")), ResultModel::ResourceRole); ../plasma-desktop/applets/kickoff/package/contents/ui/Header.qml 128: KCMShell.open("user_manager") 130: visible: KCMShell.authorize("user_manager.desktop").length > 0