Make the action selector OSD independent of the other OSDs
ClosedPublic

Authored by gladhorn on Jul 15 2018, 7:12 PM.

Details

Summary

This OSD should handle keyboard input and behave different, so make it work on its own.
In follow up changes, the keyboard handling will be added.

CCBUG: 395804

Diff Detail

Repository
R104 KScreen
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gladhorn created this revision.Jul 15 2018, 7:12 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 15 2018, 7:12 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gladhorn requested review of this revision.Jul 15 2018, 7:12 PM
zzag added a subscriber: zzag.Jul 15 2018, 7:27 PM
zzag added inline comments.
kded/osd.cpp
119

Please delete reference. Compilers can eliminate unnecessary copying, so there is no need to do the lifetime expansion.

zzag added inline comments.Jul 15 2018, 7:35 PM
kded/osd.cpp
119

s/expansion/extension/

gladhorn added inline comments.Jul 16 2018, 7:46 AM
kded/osd.cpp
119

Good point. I copied this from the original instantiation, will fix both.

gladhorn updated this revision to Diff 37850.Jul 16 2018, 7:48 AM

Remove string ref in return.

gladhorn updated this revision to Diff 37923.Jul 17 2018, 5:31 AM

re-added const

gladhorn marked 3 inline comments as done.
gladhorn updated this revision to Diff 37984.Jul 17 2018, 7:27 PM

return qstring without ref

davidedmundson requested changes to this revision.Jul 17 2018, 7:43 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
kded/osd.cpp
119

This will reconnect on the second invocation

149

I don't understand how this hideOsd() will work, it only updates m_osdObject which is the other OSD.

This revision now requires changes to proceed.Jul 17 2018, 7:43 PM
gladhorn updated this revision to Diff 37994.Jul 18 2018, 8:22 AM
gladhorn marked 2 inline comments as done.

Fix hiding of osds

gladhorn added inline comments.Jul 18 2018, 8:24 AM
kded/osd.cpp
149

Ouch, there you saved me :) osdtest works since it exits, with the real osd through kded it doesn't work at all.

broulik added inline comments.
kded/osd.cpp
125

Should we use QmlObjectSharedEngine here? (could be done separately later)

139

This assert can never be hit, you *always* create the object or return early and never end up here

kded/qml/OsdSelector.qml
31

Why initially visible?

117

This looks unrelated to this particular patch ("In follow up changes, the keyboard handling will be added.")

gladhorn added inline comments.Jul 18 2018, 4:33 PM
kded/osd.cpp
125

I think the whole OSD class is constantly being deleted/re-created (after 5 seconds of not being used iirc) so for now this is moot.

139

That is correct. Do you prefer not to have the assert?

kded/qml/OsdSelector.qml
31

It doesn't matter, can happily be removed.

117

True, I can take it out, it doesn't work anyway since we don't the focus into the dialog anyway.

gladhorn updated this revision to Diff 38029.Jul 18 2018, 4:43 PM
gladhorn marked 4 inline comments as done.

Do not yet introduce any keyboard handling (escape snuck in) and remove clutter (assert, visible:true).

gladhorn updated this revision to Diff 38117.Jul 20 2018, 7:56 AM

Use QmlObjectSharedEngine

I'd like to get this in since the meaningful changes (keyboard handling) are based on this :)

This revision was not accepted when it landed; it landed in state Needs Review.Jul 24 2018, 12:04 PM
This revision was automatically updated to reflect the committed changes.