I fear this broke some functionality of KRunner. It is important to be a dock window! Otherwise KRunner might not be able to go above fullscreen windows and won't be on all desktops. Also without being a PlasmaSurface the manual positioning cannot work. If it works for you, I fear it is pure chance.

In D10197#200479, @graesslin wrote:

I fear this broke some functionality of KRunner. It is important to be a dock window! Otherwise KRunner might not be able to go above fullscreen windows and won't be on all desktops. Also without being a PlasmaSurface the manual positioning cannot work. If it works for you, I fear it is pure chance.

Please read before freaking out. As David pointed out the surface code is already part of its parent class.

In D10197#201733, @apol wrote:

In D10197#200479, @graesslin wrote:

I fear this broke some functionality of KRunner. It is important to be a dock window! Otherwise KRunner might not be able to go above fullscreen windows and won't be on all desktops. Also without being a PlasmaSurface the manual positioning cannot work. If it works for you, I fear it is pure chance.

Please read before freaking out. As David pointed out the surface code is already part of its parent class.

No it is not. The parent class doesn't support docks - I checked.

I really think this needs to be reverted. Krunner must be a dock, it must be set to windows go below and it must take focus. This is all functionality not provided by Plasma::Dialog. Just removing the code doesn't work unfortunately.

I have a hard time understanding what the actual problem was. For me KRunner was always working. The change says it's about workarounds for X11, but that doesn't match the change as everything what's red is Wayland only. Can someone elaborate on what the actual problem was which this change was trying to address.

Feel free to revert and reopen the bug:
https://bugs.kde.org/show_bug.cgi?id=385693

The old code is clearly wrong.

It calls Dialog::event(event);

that will set up a shellsurface in the expose event
Then we process further and set up a shell surface in the expose event

PlasmaShellSurfaces are shared, but you still have two objects handling the move event simultaneously, and worst still

} else if (event->type() == QEvent::Hide) {

delete m_plasmaShellSurface;

is just screaming out for a crash as dialog is sharing that same object. We're just being very lucky at the moment.

I wouldn't want to see this reverted as-is.

But clearly we need the setRole and the setPanelTakesFocus. I missed that in my review.
That could mean just adding an if(Dock) in Dialog.cpp , only setting those parts in ExposeEvent here, or adding an extra virtual to configure dialog somehow.

As the commit went into the 5.12 branch we cannot build up on any functionality not yet available in Dialog.

To ensure we don't forget about it I created: https://bugs.kde.org/show_bug.cgi?id=389964