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

Authored by trmdi on Feb 4 2020, 5:08 AM.

Details

Summary
  • When the text is truncated, we could hover over it to read the full text
Test Plan

Everything works well

Diff Detail

Repository
R119 Plasma Desktop
Branch
add-tooltip-textWrapper (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22256
Build 22274: arc lint + arc unit
trmdi created this revision.Feb 4 2020, 5:08 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 4 2020, 5:08 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
trmdi requested review of this revision.Feb 4 2020, 5:08 AM
trmdi edited the test plan for this revision. (Show Details)Feb 4 2020, 5:12 AM
ndavis accepted this revision.Feb 4 2020, 5:18 AM
ndavis added a subscriber: ndavis.

+1

This revision is now accepted and ready to land.Feb 4 2020, 5:18 AM
cblack added a subscriber: cblack.Feb 4 2020, 5:18 AM

Be sure to add a copyright/license header to the new QML file you added.

trmdi updated this revision to Diff 74981.Feb 4 2020, 8:15 AM
  • Add license
broulik added a subscriber: broulik.Feb 4 2020, 6:25 PM

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

applets/taskmanager/package/contents/ui/TextWrapper.qml
25 ↗(On Diff #74981)

Define it more specifically as property Text textItem

50 ↗(On Diff #74981)

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

58 ↗(On Diff #74981)

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

applets/taskmanager/package/contents/ui/ToolTipInstance.qml
254–256

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

259

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

TextWrapper {
    Text {
        ...
    }
}
264

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

268

Coding style: space before ?

277–290

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

trmdi updated this revision to Diff 75026.Feb 5 2020, 4:52 AM
  • Address broulik's comment
trmdi marked 5 inline comments as done.Feb 5 2020, 4:54 AM
trmdi added a comment.Feb 6 2020, 5:14 PM

Any other objection?

trmdi updated this revision to Diff 75286.Feb 9 2020, 4:14 AM
  • Rebase
ngraham accepted this revision.Feb 9 2020, 5:23 AM
ngraham added a subscriber: ngraham.

LGTM! @broulik, are you happy with this now?

trmdi updated this revision to Diff 75287.Feb 9 2020, 5:55 AM
  • Remove the redundant line
trmdi updated this revision to Diff 75321.Feb 10 2020, 5:15 AM
  • Rebase
This revision was automatically updated to reflect the committed changes.