D10197: Fix krunner's alt+f2 on wayland
meven added inline comments. INLINE COMMENTS > view.cpp:247 > -m_plasmaShellSurface->setPanelTakesFocus(true); > -m_plasmaShellSurface->setRole(PlasmaShellSurface::Role::Panel); > -//this should be on showEvent, but it was too soon so none of > those had any effect And this too, it causes https://bugs.kde.org/show_bug.cgi?id=389964 KRunner appears behind keep above windows. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: meven, graesslin, broulik, ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D10197: Fix krunner's alt+f2 on wayland
apol closed this revision. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin added a comment. To ensure we don't forget about it I created: https://bugs.kde.org/show_bug.cgi?id=389964 REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin added a comment. As the commit went into the 5.12 branch we cannot build up on any functionality not yet available in Dialog. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
davidedmundson added a comment.
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.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D10197
To: apol, #plasma, davidedmundson
Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot,
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
apol added a comment. Feel free to revert and reopen the bug: https://bugs.kde.org/show_bug.cgi?id=385693 REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin added a comment. In https://phabricator.kde.org/D10197#201733, @apol wrote: > In https://phabricator.kde.org/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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
apol added a comment. In https://phabricator.kde.org/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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin reopened this revision. graesslin added a comment. This revision is now accepted and ready to land. I just verified with KWin's debug console: KRunner window is now marked as NET::Normal and not as NET::Dock. This is a problem, KRunner needs to be a dock. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
graesslin added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: graesslin, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
broulik added inline comments. INLINE COMMENTS > view.cpp:246 > - > m_plasmaShellSurface->setPanelBehavior(PlasmaShellSurface::PanelBehavior::WindowsGoBelow); > -m_plasmaShellSurface->setPanelTakesFocus(true); > -m_plasmaShellSurface->setRole(PlasmaShellSurface::Role::Panel); Don't we still need this somewhere? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
This revision was automatically updated to reflect the committed changes. Closed by commit R120:fc5a45403025: Fix krunners alt+f2 on wayland (authored by apol). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10197?vs=26233=26247 REVISION DETAIL https://phabricator.kde.org/D10197 AFFECTED FILES krunner/view.cpp krunner/view.h To: apol, #plasma, davidedmundson Cc: ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
davidedmundson accepted this revision. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D10197 To: apol, #plasma, davidedmundson Cc: ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
apol updated this revision to Diff 26233. apol added a comment. Readd the comment If somebody can test it on X11, I'd merge to 5.12. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10197?vs=26232=26233 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10197 AFFECTED FILES krunner/view.cpp krunner/view.h To: apol, #plasma, davidedmundson Cc: ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
apol updated this revision to Diff 26232. apol added a comment. Remove code REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10197?vs=26230=26232 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10197 AFFECTED FILES krunner/view.cpp krunner/view.h To: apol, #plasma Cc: ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
davidedmundson added inline comments.
INLINE COMMENTS
> view.cpp:236
> if (m_plasmaShell && event->type() == QEvent::Expose) {
> using namespace KWayland::Client;
> auto ee = static_cast(event);
This code here is definitely not designed for X11
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D10197
To: apol, #plasma
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D10197: Fix krunner's alt+f2 on wayland
apol created this revision. apol added a reviewer: Plasma. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. apol requested review of this revision. REVISION SUMMARY Don't go through the workaround introduced for X11 that makes it go mental. BUG: 385693 TEST PLAN Have been a happy krunner user since REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D10197 AFFECTED FILES krunner/view.cpp To: apol, #plasma Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
