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
