D27149: Scroll the truncated song/artist text when long hovering over it

[Kai Uwe Broulik]

broulik added a comment.


  Overall well thought out with that `State` of yours :) Just some minor nitpicks:

INLINE COMMENTS

> TextWrapper.qml:25
> +
> +    property var textItem
> +    readonly property alias containsMouse: mouseItem.containsMouse

Define it more specifically as `property Text textItem`

> TextWrapper.qml:50
> +
> +    MouseArea {
> +        id: mouseItem

You can just make the `textWrapper` be a `MouseArea`, saves you having an extra area inside

> TextWrapper.qml:58
> +        id: timer 
> +        running: false; interval: 100; repeat: true;
> +        onTriggered: {

You can simplify this long duration handling a bit:

  Timer {
      interval: 500
      running: area.containsMouse
      onTriggered: {
          area.state = "longHovering"
      }
  }

and then all you need is

  onContainsMouseChanged: {
      if (!containsMouse) {
          area.state = "";
      }
  }

I *think* you could even do it more declaratively by having the `State` do

  when: area.containsMouse && !timer.running

but since property evaluation order is non-deterministic, it might briefly enter the state on hover

> ToolTipInstance.qml:324
>  
> -                        PlasmaComponents.Label {
> +                        TextWrapper {
> +                            id: songTextWrapper

I think this item could use a better name, maybe `TextHoverScroller` or something like that?

> ToolTipInstance.qml:330
> +
> +                            textItem: songText
> +

QML trick: Define the property as `default property` and then you can just write

  TextWrapper {
      Text {
          ...
      }
  }

> ToolTipInstance.qml:335
> +                                width: parent.width
> +                                height: contentHeight
> +                                lineHeight: 1

I think you can assign `undefined` to reset it to its default value. Plasma `Label` annoyingly overwrites its `height`

> ToolTipInstance.qml:339
> +                                wrapMode: Text.NoWrap
> +                                elide: songTextWrapper.longHovering? Text.ElideNone : Text.ElideRight
> +                                text: track || ""

Coding style: space before `?`

> ToolTipInstance.qml:348
>                              Layout.fillWidth: true
> -                            wrapMode: Text.NoWrap
> -                            lineHeight: 1
> -                            elide: Text.ElideRight
> -                            text: artist || ""
> -                            visible: text != ""
> -                            font.pointSize: theme.smallestFont.pointSize
> +                            Layout.preferredHeight: artistText.visible? artistText.contentHeight : 0
> +

Generally try to avoid binding to `visible` as that updates recursively. Since this contains the label below, you can probably just make this `visible: artistText.text !== ""` and then remove the `visible` statement below

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-tooltip-textWrapper (branched from master)

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

To: trmdi, #plasma, #vdg, ndavis
Cc: broulik, cblack, ndavis, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200204/6d44c3eb/attachment-0001.html>