Differential D4214

Make sure we display enabled ListItem on hover
ClosedPublic
Actions

Authored by apol on Jan 19 2017, 11:59 PM.

Details

Reviewers
mart
Group Reviewers
Plasma
Commits
R242:54e87fe879ba: Make sure we display enabled ListItem on hover
Summary

I just realized there was a weird workaround in plasma-nm where it gets
checked so it's shown.
Properly use the declarative API instead of listening to a ton of signals.

Test Plan

Tested with plasma-nm and org.kde.plasma.notifications plasmoids

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol updated this revision to Diff 10372.Jan 19 2017, 11:59 PM
apol retitled this revision from to Make sure we display enabled ListItem on hover.
apol updated this object.
apol edited the test plan for this revision. (Show Details)
apol added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptJan 19 2017, 11:59 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
davidedmundson added a subscriber: davidedmundson.Jan 20 2017, 1:27 AM

Concept seems fine. New binding has some typos

src/declarativeimports/plasmacomponents/qml/ListItem.qml
97

why are you now following containsMouse. That's a behavioural change

How can background.pressed be true. I don't see where it exists?
do you mean itemMouse.pressed?

apol marked an inline comment as done.Jan 20 2017, 3:21 AM
apol added inline comments.
src/declarativeimports/plasmacomponents/qml/ListItem.qml
97

Yes, I meant to put itemMouse.pressed. I wonder why QML doesn't complain...

It's barely a behavioral change, in fact it's what it was supposed to happen in the first place: give feedback on hover when the item is clickable.
Note that at the moment we're dimming the opacity for the invisible background we have.

apol updated this revision to Diff 10379.Jan 20 2017, 3:22 AM
apol marked an inline comment as done.

Address David's comment

davidedmundson added inline comments.Jan 20 2017, 3:28 AM
src/declarativeimports/plasmacomponents/qml/ListItem.qml
97

in fact it's what it was supposed to happen in the first place

We have the highlight for the item under mouse.
Why should the delegate change too?

apol added inline comments.Jan 20 2017, 1:09 PM
src/declarativeimports/plasmacomponents/qml/ListItem.qml
97

If that's the case, then we should remove the opacity change on hover altogether then.
And make sure our plasmoids use the highlighter.

mart added a subscriber: mart.Jan 23 2017, 2:42 PM
mart added inline comments.
src/declarativeimports/plasmacomponents/qml/ListItem.qml
97

when the item contains the mouse the prefix shouldn't be "pressed"
I'm fine with using the highlight item and just dropping the opacity change

apol updated this revision to Diff 10489.Jan 24 2017, 11:31 AM

Removed hover state as requested by David and Marco

mart accepted this revision.Jan 24 2017, 12:59 PM
mart added a reviewer: mart.
This revision is now accepted and ready to land.Jan 24 2017, 12:59 PM
Closed by commit R242:54e87fe879ba: Make sure we display enabled ListItem on hover (authored by apol). · Explain WhyJan 24 2017, 2:47 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 10498

View Options

src/declarativeimports/plasmacomponents/qml/ListItem.qml

Loading...