[Differential] [Updated] D3738: [Task Manager] Tooltips redesign

Tue Jan 3 21:11:06 UTC 2017

subdiff added inline comments.

INLINE COMMENTS

> hein wrote in ContextMenu.qml:59
> 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?

The context menu can be called now also for a grouped task. The visualParent is the grouped task parent though. So we need to get the modelProps of property var modelIndex (which is in case of a grouped task different from visualParent.index, i.e. visualParent.m)

> hein wrote in Task.qml:103
> Why did you remove the tooltip hiding?
> 
> See also https://phabricator.kde.org/D3916 btw

When the context menu is opened, it requests focus and the tooltip area loses mouse over, which results anyway in hiding the tooltip. The hideToolTip() call is unnecessary therefore.

https://phabricator.kde.org/D3916 only concerns left click. Since it was created after I created this diff, it wasn't yet integrated.

> hein wrote in Task.qml:224
> 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?

How would reinstating look in contrast to undefining the sourceComponent and then setting it back to the component?

> hein wrote in Task.qml:235
> 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.

This is also part of the workaround hack and therefore hopefully only temporary until the Qt bug is fixed.

> hein wrote in ToolTipDelegate.qml:40
> 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?

(1) Done
(2) Was carried over from old tooltips. Otherwise we get lots of "can not assign undefined" warning messages on loading the component, because the model is not yet ready.

> hein wrote in ToolTipDelegate.qml:199
> 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.

I wanted to use Headings with different levels. The problem is, that they always automatically create huge margins. Since we wanted to minimize the size of the tooltips as much as possible, I used instead relative sizes, which worked way better for me, because they give a more direct control over the look in the end.

They are not really magic numbers, are they? Read the 0.75 value for example as: the window title font size is 75% as large as the font size of the app name.

Of course on the other side you raise a valid point with the consistency. Though I've seen relative font sizes also elsewhere (digital clock).

> hein wrote in ToolTipDelegate.qml:446
> Is this your code? What's "emergence"?

No, not my code. These were the buttons from the old revision. I would remove this code portion, if there is no problem with my rewrite of them as IconItems directly above, which don't produce error messages on emergence (when the tooltip of a player opens) and which imo look better here in this context. I'll just remove it in the second diff to minimize confusion.

> hein wrote in main.qml:379
> "Do things later" timers tend to be hacks that need a comment justifying their existence ...

It's the same hack / workaround from Task.qml because of the Qt bugs of the submodel index. Clarified it.

REPOSITORY
  R119 Plasma Desktop

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

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

To: subdiff, #vdg, hein, #plasma
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/3a1d5194/attachment.html>