Zren / material-decoration

Material-ish window decoration theme for KWin, with LIM, based on zzag's original design.
GNU General Public License v2.0
197 stars 17 forks source link

Window does not get focus back when clicking on app menu buttons #3

Closed ivnvitx closed 3 years ago

ivnvitx commented 4 years ago

If there is another app on focus, clicking on other app's menu buttons will open the menus but not give focus to the window.

Zren commented 4 years ago

This bug also affect the default AppIcon (and I assume the default AppMenu) dropdown buttons.

Zren commented 4 years ago

The issue with Issue #3 @trmdi (raise window on appmenu click) is that there isn't a good example. KDecoration doesn't support it, as it assumes KWin will handle it.

Open two windows 50% wide. Focus on one window, then click the maximize of the inactive window. It will maximize the window, but not focus it. It's a KDecoration bug. Well kinda. You don't want it to focus the window when minimize or close is pressed.

I could try finding a X11 only solution similar to other x11 events, or use KWindowSystem.

trmdi commented 4 years ago

I've just found that if I add event->setAccepted(false) here [1], it will work.

[1] https://github.com/Zren/material-decoration/blob/0de8dfd4a09507b48163585dd38ebfabc9ba4531/src/Decoration.cc#L178

We also no longer need Decoration::sendMoveEvent(const QPoint pos).

Zren commented 3 years ago

We can send setAccepted(false) on LeftButton. Unfortunately I don't think there's a way to detect when the titlebar starts the drag, so we can't run unPressAllButtons() to fix the sticky button underneath the drag. A sticky button causes the (possibly) wrong menu to open on next click, as the KDecoration::Decoration::mouseReleaseEvent looks for "pressed" buttons instead of doing another cursor is inside the button hitbox check...

So we're stuck with manually sending a move event to X11 till I figure out a way around that.

Thanks for the the tip though, as this was useful for not accepting the wheel, middle and right click events in: https://github.com/Zren/material-decoration/commit/b6c5cdd7b02f3f44af2b7452ccb90b17c3ed35a6

Zren commented 3 years ago

Note to self, the default behavior starts the drag after holding for a short timer. It does not trigger after dragging a few pixels.

https://github.com/KDE/kwin/blob/master/abstract_client.cpp#L1033

trmdi commented 3 years ago

Unfortunately I don't think there's a way to detect when the titlebar starts the drag, so we can't run unPressAllButtons() to fix the sticky button underneath the drag. A sticky button causes the (possibly) wrong menu to open on next click

I don't understand this, what sticky button? And I don't see any wrong menu is opened on the next click. Could you post a video about this?

Ah, I got it. It happens when unPressAllButtons() is not called. But can we just add setAccepted(false), while still keeping that one? Could you test this patch?

diff --git a/src/Decoration.cc b/src/Decoration.cc
index d845ece..5e39547 100644
--- a/src/Decoration.cc
+++ b/src/Decoration.cc
@@ -176,6 +176,7 @@ void Decoration::mousePressEvent(QMouseEvent *event)

     if (m_menuButtons->geometry().contains(event->pos())) {
         initDragMove(event->pos());
+        event->setAccepted(false);
     }
 }
Zren commented 3 years ago

Ah, we have to keep initDragMove(event->pos()). If you remove that function it looks like this:

https://www.youtube.com/watch?v=7kD3A5p__p8

However if you keep our initDragMove function, it will call unPressAllButtons() and trigger upon dragging ~4 pixels. Since we setAccepted(false) on press however, it starts the KWin drag timer, so if you click + hold without moving the cursor, it'll initiate a drag by KWin itself. There's also a bug where the drag position starts at the window's (0,0) coordinate. Luckily though, the menu buttons are not stuck in a "sticky" state, probably due to the mouse being forcefully moved?

https://www.youtube.com/watch?v=5p55k_Y5hPs

trmdi commented 3 years ago

Can we just remove sendMoveEvent() ?

Zren commented 3 years ago

Huh, we could... We still need initDragMove() and dragMoveTick() to track the distance dragged to call unPressAllButtons() after a move has started.

On a click and hold, the force move cursor to (0,0) bug doesn't appear. For some reason QHoverEvent::pos() is (0,0) which causes the delta position to be negative m_pressedPoint that we stored during initDragMove().

kdecoration.material:     diff QPoint(0,0) mL 0 sDD 10
kdecoration.material:     diff QPoint(-256,-11) mL 267 sDD 10
kdecoration.material: AppMenuButtonGroup::unPressAllButtons

It... works, but for an unknown reason.

Edit: I bet the force move to (0,0) bug is because first the drag timer starts a move, then dragMoveTick detects a move and calls sendMoveEvent with it thinking the start of the drag is at (0,0). Still no idea why QHoverEvent::pos() is (0,0).

trmdi commented 3 years ago

I meant just removing sendMoveEvent, still keep initDragMove and dragMoveTick. I guess there is something wrong in sendMoveEvent, but we don't need it (didn't look into it carefully, just a guess).

Zren commented 3 years ago

Read my edit:

Edit: I bet the force move to (0,0) bug is because first the drag timer starts a move, then dragMoveTick detects a move and calls sendMoveEvent with it thinking the start of the drag is at (0,0). Still no idea why QHoverEvent::pos() is (0,0).

If we remove sendMoveEvent, in theory, there will be an edge case when:

The code would not detect a drag, as it's within the drag threshold.

When I first tested, I forgot that there's a smallSpacing between m_leftButtons and m_menuButtons even if m_leftButtons is empty. Upon fixing that bug so that menuButtons follow fitt's law, I was able to test click (0,0) and holding, and the button did stick. This affects a tiny number of users though, as they'd need to remove all leftButtons. It would also only stick on the first button.