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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
258–260

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

263

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

TextWrapper {
    Text {
        ...
    }
}
268

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

272

Coding style: space before ?

281–294

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.