[Differential] [Requested Changes To] D3738: [Task Manager] Tooltips redesign

[hein (Eike Hein)]

hein requested changes to this revision.
hein added a reviewer: hein.
hein added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ContextMenu.qml:59
>  
> +    function get(modelProp) {
> +        return tasksModel.data(modelIndex, modelProp)

Maybe s/get/data to be more conventional, but could you argue for kicks why this churn is needed vs. just making sure the visualParent has the m prop?

> Task.qml:77
>      onItemIndexChanged: {
> +        toolTipLoader.needsReload = true
> +        taskRepeater.delayedReloadToolTip()

Plasma coding style: Terminate lines in procedural blocks with semicolae.

https://community.kde.org/Plasma/QMLStyle

Please fix throughout the patch.

> Task.qml:103
>          } else if (mouse.button == Qt.RightButton) {
> -            if (plasmoid.configuration.showToolTips) {
> -                toolTip.hideToolTip();

Why did you remove the tooltip hiding?

See also https://phabricator.kde.org/D3916 btw

> Task.qml:224
>  
> -            active: !inPopup && !groupDialog.visible && plasmoid.configuration.showToolTips
> -            interactive: true
> -            location: plasmoid.location
> -
> -            mainItem: toolTipDelegate
> -
> -            onContainsMouseChanged:  {
> -                if (containsMouse) {
> -                    toolTipDelegate.parentIndex = itemIndex;
> -
> -                    toolTipDelegate.windows = Qt.binding(function() {
> -                        return model.LegacyWinIdList;
> -                    });
> -                    toolTipDelegate.mainText = Qt.binding(function() {
> -                        return model.display;
> -                    });
> -                    toolTipDelegate.icon = Qt.binding(function() {
> -                        return model.decoration;
> -                    });
> -                    toolTipDelegate.subText = Qt.binding(function() {
> -                        return model.IsLauncher === true ? model.GenericName : toolTip.generateSubText(model);
> -                    });
> -                    toolTipDelegate.launcherUrl = Qt.binding(function() {
> -                        return model.LauncherUrlWithoutIcon;
> -                    });
> +            // workaround, see: https://bugreports.qt.io/browse/QTBUG-47523
> +            //                  https://bugreports.qt.io/browse/QTBUG-55864

I know we talked about this, but this approach strikes me as weird. A component is something to be instanciated; reloading the sourceComponent value seems wrong since that's not about a particular instance, and any problem you may have with the index would be related to a particular instance. You may want to reinstanciate the component, but why reset the component?

> Task.qml:235
>              }
> +            Component.onCompleted: taskRepeater.reloadToolTips.connect(reload)
> +            //

Bad coding style / bad coupling: Referencing a non-generic name outside the delegate ... now this delegate only works right if there's a taskRepeater outside somewhere.

> ToolTipDelegate.qml:40
> +    property int parentIndex: parentTask.itemIndex
> +    property bool isGroup: 1 < parentTask.m.ChildCount
> +    property var windows: parentTask.m.LegacyWinIdList

Sanity check: Why count children instead of the IsGroupParent role?

Also:

(1) For performance and code hygiene reasons please mark properties readonly if you won't assign them.

(2) Why copy tons of data roles into local props in the first place instead of just accessing the model directly?

> ToolTipDelegate.qml:174
> +                    Column {
> +                        spacing: 0.75 * units.smallSpacing
> +                        PlasmaComponents.Label {

Why 0.75? That's not an even number of pixels ...

> ToolTipDelegate.qml:199
> +                            width: isWin ? textWidth : undefined
> +                            height: 0.6 * theme.mSize(theme.defaultFont).height
> +                            font.pointSize: -1

Why 0.6? Is that in the HIG somewhere? Maybe instead of Label we could use a Heading of a particular level? We can't have consistent typography across applets if applets contain magic numbers for sizes.

> ToolTipDelegate.qml:446
> +
> +//                                // TODO: These buttons produce error messages on emergence,
> +//                                //       see: https://bugreports.qt.io/browse/QTBUG-55844

Is this your code? What's "emergence"?

> ToolTipDelegate.qml:489
>  
> -            PlasmaComponents.ToolButton {
> -                enabled: canGoNext
> -                iconName: LayoutMirroring.enabled ? "media-skip-backward" : "media-skip-forward"
> -                tooltip: i18nc("Go to next song", "Next")
> -                Accessible.name: tooltip
> -                onClicked: mpris2Source.goNext(mprisSourceName)
> -            }
> -        }
> -    }
> +                    var appNameRegex = new RegExp(appName + "$", "i")
> +                    text = text.replace(appNameRegex, "")

Needs a code comment explaining what it wants to achieve.

> main.qml:372
> +
> +            // tooltips submodel index workaround
> +            signal reloadToolTips()

Please use proper capitalization and punctuation in code comments throughout the patch.

> main.qml:379
> +        Timer {
> +            id: reloadTimer
> +            interval: 10

"Do things later" timers tend to be hacks that need a comment justifying their existence ...

REPOSITORY
  R119 Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, #vdg, #plasma, hein
Cc: hein, colomar, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170103/1d93d348/attachment-0001.html>