[TabBox] Fix Arrow Key / Keyboard Events in QML Alt+Tab Skins

Authored by Zren on Sep 24 2018, 4:29 AM.


Group Reviewers

Send Keyboard Events to Dialog.children instead of Dialog.contentItem.children.

The only children of contentItem are a Plasma::FrameSvgItem and a ColorScope, and not the very important mainItem which is where most Keys.onPressed is defined.

BUG: 370185

Assuming this is backportable:
Fixed In: 5.12.7 (released tomorrow so too late for that)
Fixed In: 5.12.8 (Feb 12th) <= Most likely?

Fixed In: 5.14.0 (Beta was Sep 13, Tagged Oct 4, Released Oct 9)
Fixed In: 5.14.1 (Oct 16)

Here's where FrameSvgItem is defined / set as a child on contentItem.

Here's where mainItem *should* be a child of contentItem.

I've no idea where ColorScope is defined as a child, but when I logged it, it had a null parent.

kwin_tabbox:     d->window() PlasmaQuick::Dialog(0x15bcc30 exposed, ...)
kwin_tabbox:     d->window()->contentItem() QQuickRootItem(0x15820e0, parent=0x0, geometry=0,0 932x206)
kwin_tabbox:     d->window()->sendEvent Plasma::FrameSvgItem(0x15be890, parent=0x15820e0, geometry=0,0 932x206)
kwin_tabbox:     d->window()->sendEvent ColorScope(0x16862a0, parent=0x0, geometry=0,0 0x0)

Seeing as mainItem is not a child of contentItem, this could actually be a plasma-framework bug too. That said, I don't really see a need to send keyboard events to the PlasmaCore.Dialog background svg or the ColorScope.

Here's what it sends to now:

kwin_tabbox:     d->window() PlasmaQuick::Dialog(0x1b97d00 exposed, ...)
kwin_tabbox:     d->window()->contentItem() QQuickRootItem(0x1b94be0, parent=0x0, geometry=0,0 932x602)
kwin_tabbox:     d->window()->sendEvent QQuickRootItem(0x1b94be0, parent=0x0, geometry=0,0 932x602)
kwin_tabbox:     d->window()->sendEvent QQuickItem_QML_72(0x1b99a60, parent=0x1b94be0, geometry=4,4 924x594)
Test Plan

Use arrow keys with any skin that defines the key handler at Dialog.mainItem.Keys.onPressed. Breeze does not define a key event handler at all (so a patch to plasma-workspace is next).

If you have the kwin-addons package, aka the kdeplasma-addons git repo (sudo apt search kwin-addons in Neon), you can test Large Icons. Informative does not work however.

A full table of which QML skins work can be seen at this comment in the bug report:

Diff Detail

R108 KWin
Lint Skipped
Unit Tests Skipped
Zren created this revision.Sep 24 2018, 4:29 AM
Restricted Application added a project: KWin. · View Herald TranscriptSep 24 2018, 4:29 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
Zren requested review of this revision.Sep 24 2018, 4:29 AM
zzag added a subscriber: zzag.EditedSep 24 2018, 5:19 AM

setParentItem sets visual parent. Object parent and visual parent can be different.

I have a feeling that if we change

const QList<QQuickItem*> items = d->window()->contentItem()->findChildren<QQuickItem*>(QString(), Qt::FindDirectChildrenOnly);
const QList<QQuickItem*> items = d->window()->contentItem()->childItems();

arrow keys will work.

Maybe, it should be fixed in Plasma::Dialog instead?

Zren added a comment.Sep 24 2018, 6:10 AM

I confirmed that contentItem()->childItems() did indeed send it to mainItem. It also sends it to the FrameSvgItem too. It no longer sends it to the mysterious ColorScope.

kwin_tabbox:     d->window() PlasmaQuick::Dialog(0x11927d0 exposed, ...)
kwin_tabbox:     d->window()->contentItem() QQuickRootItem(0x117b820, parent=0x0, geometry=0,0 1240x404)
kwin_tabbox:     d->window()->sendEvent Plasma::FrameSvgItem(0x11a5910, parent=0x117b820, geometry=0,0 1240x404)
kwin_tabbox:     d->window()->sendEvent QQuickItem_QML_52(0x10f2090, parent=0x117b820, geometry=4,4 1232x396)

Maybe, it should be fixed in Plasma::Dialog instead?

That's what I was afraid of.

Thanks for telling me about the differences of QQuickItem::setParentItem(). Totally missed that. So I assume that is suppose to be QObject::setParent()? Changing dialog.cpp would require a crapload of testing.

I decided to check what mainItem.parent is right now, and it looks like it's QQuickRootItem. A normal Item defined in a property has a parent of null.

PlasmaCore.Dialog {
	id: dialog
	mainItem: Item {
		id: dialogMainItem

		Component.onCompleted: {
			// Before:
			//   dialogMainItem.parent = ?
			//   dialogMainItem.parentItem = dialog.contentItem()

			// qml: dialog PlasmaQuick::Dialog(0xd8e650)
			// qml: dialogMainItem QQuickItem_QML_49(0xdf3d00)
			// qml: dialogMainItem.parent QQuickRootItem(0xd84180)

			// After:
			//   dialogMainItem.parent = dialog.contentItem()
			//   dialogMainItem.parentItem = ?

			// ?

	property Item testItem: Item {
		id: dialogTestItem
		Component.onCompleted: {
			console.log('dialogTestItem', dialogTestItem)
			console.log('dialogTestItem.parent', dialogTestItem.parent)
			// qml: dialogTestItem QQuickItem(0x101cf70)
			// qml: dialogTestItem.parent null
ngraham added a reviewer: KWin.EditedOct 8 2018, 4:52 PM
ngraham added a subscriber: ngraham.

What's the status of this patch? I'd love to get this fix, because it's a blocker for https://bugs.kde.org/show_bug.cgi?id=308331.

Zren added a subscriber: mart.Oct 8 2018, 7:07 PM

I need to try patching plasma-framework's dialog.cpp and see if that introduces issues. Right now it's mainItem->setParentItem(contentItem());, and I need to test if mainItem->setParent(contentItem()); breaks anything. I haven't patched C++ in frameworks before so I'll need to figure that out.


If I look at the git blame of that line, the last commit that touched setParentItem shows that there used to be a d->mainItem->setParent(parent());


Hmmm, it looks like setParent was removed in this commit.


@mart: Should I add main mainItem->setParent(contentItem());, or should we fix this in KWin?

Don't focus too much on Plasma::Dialog. It should also work with a plain QQuickWindow or a QuickControls2::Dialog

Both old and patched code seem weird.

I would expect to send it to window->activeFocusItem
(or just send the event to the window which will then do that)

It will then match what a normal QML user would expect.
That /should/ work for at least the main breeze theme (untested)

@mart: Should I add main mainItem->setParent(contentItem());, or should we fix this in KWin?

Not having mainItem a visual child of the content item is super weird - but changing anything in Dialog is risky.

A reason for the old code being weird is that KWin is weird. I remember that I fought quite a lot when porting this to Qt5 and there was a time when it was working.

davidedmundson requested changes to this revision.Nov 8 2018, 1:32 PM

The other fix is merged, we should be able to drop this.

This revision now requires changes to proceed.Nov 8 2018, 1:32 PM
Zren abandoned this revision.Nov 8 2018, 1:38 PM