D28154: Add users KCM

[Kai Uwe Broulik]

broulik added a comment.


  Pretty cool

INLINE COMMENTS

> UserDetailsPage.qml:36
> +
> +    property variant user
> +    property bool overrideImage: false

Use `property var`, or even `property QtObject` since `User` is a `QObject`

> UserDetailsPage.qml:82
> +        id: fileDialog
> +        title: "Choose a picture"
> +        folder: shortcuts.pictures

`i18n`

> UserDetailsPage.qml:96
> +            Layout.alignment: Qt.AlignHCenter
> +            QQC2.RoundButton {
> +                id: userPfp

Is this a `RoundButton` bug that it doesn't indicate keyboard focus when I tab to it?

> UserDetailsPage.qml:97
> +            QQC2.RoundButton {
> +                id: userPfp
> +

Pfp?

> UserDetailsPage.qml:99
> +
> +                property int size: 6 * Kirigami.Units.gridUnit
> +

`readonly property`

> UserDetailsPage.qml:111
> +                    visible: usersDetailPage.user.faceValid || usersDetailPage.overrideImage
> +                    sourceSize: Qt.size(parent.size, parent.size)
> +                    cache: false

I *think* one needs to multiple that with `Screen.devicePixelRatio`

> UserDetailsPage.qml:217
> +
> +    Kirigami.OverlaySheet {
> +        id: picturesSheet

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;
  }

> UserDetailsPage.qml:219
> +        id: picturesSheet
> +        header: RowLayout {
> +            Kirigami.Heading {

Is this `RowLayout` needed?

> UserDetailsPage.qml:287
> +                        model: [
> +                            "Artist Konqi.png",
> +                            "Bookworm Konqi.png",

Can we not hardcode the list of avatars, please.
Also, you probably want to use a proper `GridView` for all of this, otherwise you end up creating every single delegate immediately on opening.

> UserDetailsPage.qml:342
> +
> +                    QQC2.Button {
> +                        Layout.preferredHeight: Kirigami.Units.gridUnit * 6

A tooltip and `Accessible.name` with the name of the avatar would be nice

> UserDetailsPage.qml:346
> +
> +                        ColumnLayout {
> +                            anchors.centerIn: parent

Is this `ColumnLayout` needed?

> UserDetailsPage.qml:361
> +
> +                    Repeater {
> +                        model: [

What's all of this? I don't see anything in the UI.
Anyway, we probably want to make all of this into a proper `QAbstractListModel`

> UserDetailsPage.qml:399
> +                            onClicked: {
> +                                colourRectangle.grabToImage(function(result) {
> +                                    picturesSheet.close()

Not sure if clever or mad :) Can we not just generate a colored rectangle on C++ side?

> usermodel.cpp:112
> +{
> +    if (!checkIndex(index))
> +    {

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 :/

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D28154

REVISION DETAIL
  https://phabricator.kde.org/D28154

To: cblack, #plasma, #vdg, ngraham
Cc: ltoscano, 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200513/28c35e0e/attachment-0001.htm>