From b1b40ed0585a7345807e0135e5e14b576715025d Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 11 Apr 2020 01:41:24 +0100 Subject: [PATCH 1/5] tests: add a test case for #299 Signed-off-by: Yuxuan Shui --- tests/run_one_test.sh | 2 +- tests/run_tests.sh | 3 + tests/testcases/issue299.py | 112 ++++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) create mode 100755 tests/testcases/issue299.py diff --git a/tests/run_one_test.sh b/tests/run_one_test.sh index 13da3df..4619359 100755 --- a/tests/run_one_test.sh +++ b/tests/run_one_test.sh @@ -7,7 +7,7 @@ fi echo "Running test $2" # TODO keep the log file, and parse it to see if test is successful -($1 --experimental-backends --backend dummy --log-level=debug --log-file=$PWD/log --config=$2) & +($1 --dbus --experimental-backends --backend dummy --log-level=debug --log-file=$PWD/log --config=$2) & main_pid=$! $3 diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 9f3c9e4..08203e0 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -3,6 +3,8 @@ set -e exe=$(realpath $1) cd $(dirname $0) +eval `dbus-launch --sh-syntax` + ./run_one_test.sh $exe configs/empty.conf testcases/basic.py ./run_one_test.sh $exe configs/issue357.conf testcases/issue357.py ./run_one_test.sh $exe configs/issue239.conf testcases/issue239.py @@ -12,3 +14,4 @@ cd $(dirname $0) ./run_one_test.sh $exe configs/issue314.conf testcases/issue314.py ./run_one_test.sh $exe configs/issue314.conf testcases/issue314_2.py ./run_one_test.sh $exe configs/issue314.conf testcases/issue314_3.py +./run_one_test.sh $exe /dev/null testcases/issue299.py diff --git a/tests/testcases/issue299.py b/tests/testcases/issue299.py new file mode 100755 index 0000000..d8be70c --- /dev/null +++ b/tests/testcases/issue299.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 + +import xcffib.xproto as xproto +import xcffib +import time +import os +import subprocess +import asyncio +from dbus_next.aio import MessageBus +from dbus_next.message import Message, MessageType +from common import * + +display = os.environ["DISPLAY"].replace(":", "_") +conn = xcffib.connect() +setup = conn.get_setup() +root = setup.roots[0].root +visual = setup.roots[0].root_visual +depth = setup.roots[0].root_depth +x = xproto.xprotoExtension(conn) +visual32 = find_32bit_visual(conn) + +async def get_client_win_async(wid): + message = await bus.call(Message(destination='com.github.chjj.compton.'+display, + path='/', + interface='com.github.chjj.compton', + member='win_get', + signature='us', + body=[wid, 'client_win'])) + return message.body[0] + +def get_client_win(wid): + return loop.run_until_complete(get_client_win_async(wid)) + +def wait(): + time.sleep(0.5) + +def create_client_window(name): + client_win = conn.generate_id() + print("Window : ", hex(client_win)) + conn.core.CreateWindowChecked(depth, client_win, root, 0, 0, 100, 100, 0, + xproto.WindowClass.InputOutput, visual, 0, []).check() + set_window_name(conn, client_win, "Test window "+name) + set_window_class(conn, client_win, "Test windows") + set_window_state(conn, client_win, 1) + conn.core.MapWindowChecked(client_win).check() + return client_win + +loop = asyncio.get_event_loop() +bus = loop.run_until_complete(MessageBus().connect()) + +cmid = conn.generate_id() +colormap = conn.core.CreateColormapChecked(xproto.ColormapAlloc._None, cmid, root, visual32).check() + +# Create window +client_wins = [] +for i in range(0,2): + client_wins.append(create_client_window(str(i))) + +# Create frame window +frame_win = conn.generate_id() +print("Window : ", hex(frame_win)) +conn.core.CreateWindowChecked(depth, frame_win, root, 0, 0, 200, 200, 0, + xproto.WindowClass.InputOutput, visual, 0, []).check() +set_window_name(conn, frame_win, "Frame") +conn.core.MapWindowChecked(frame_win).check() + +# Scenario 1.1 +# 1. reparent placeholder to frame +conn.core.ReparentWindowChecked(client_wins[0], frame_win, 0, 0).check() +wait() +# 2. reparent real client to frame +conn.core.ReparentWindowChecked(client_wins[1], frame_win, 0, 0).check() +wait() +# 3. detach the placeholder +conn.core.ReparentWindowChecked(client_wins[0], root, 0, 0).check() +wait() +assert get_client_win(frame_win) == client_wins[1] + +# Scenario 1.2 +# 1. reparent placeholder to frame +conn.core.ReparentWindowChecked(client_wins[0], frame_win, 0, 0).check() +wait() +# 2. reparent real client to frame +conn.core.ReparentWindowChecked(client_wins[1], frame_win, 0, 0).check() +wait() +# 3. destroy the placeholder +conn.core.DestroyWindowChecked(client_wins[0]).check() +wait() +assert get_client_win(frame_win) == client_wins[1] + +client_wins[0] = create_client_window("0") + +# Scenario 2 +# 1. frame is unmapped +conn.core.UnmapWindowChecked(frame_win).check() +wait() +# 2. reparent placeholder to frame +conn.core.ReparentWindowChecked(client_wins[0], frame_win, 0, 0).check() +wait() +# 3. destroy placeholder, map frame and reparent real client to frame +conn.core.DestroyWindowChecked(client_wins[0]).check() +conn.core.MapWindowChecked(frame_win).check() +conn.core.ReparentWindowChecked(client_wins[1], frame_win, 0, 0).check() +wait() +assert get_client_win(frame_win) == client_wins[1] + +client_wins[0] = create_client_window("0") + +# Destroy the windows +for wid in client_wins: + conn.core.DestroyWindowChecked(wid).check() +conn.core.DestroyWindowChecked(frame_win).check() From 440157f20b15ab01dd25cb7fe8f74d85bf4800a6 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 10 Apr 2020 01:44:33 +0100 Subject: [PATCH 2/5] win, event: delayed handling of reparent notify Instead of handling reparent notify on the spot by updating the client windows, setup a flag on the window and call win_recheck_client later. This makes handling of complex scenarios easier. As example, see the case in issue #299. Note this is not a complete fix for #299 --- src/event.c | 59 +++++++++++++++++++++++++++----------------------- src/win.c | 15 +++++++++++-- src/win_defs.h | 2 ++ 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/event.c b/src/event.c index c6e1179..f21e346 100644 --- a/src/event.c +++ b/src/event.c @@ -336,28 +336,35 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t ps->c, ev->window, XCB_CW_EVENT_MASK, (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN)}); - // Check if the window is an undetected client window - // Firstly, check if it's a known client window - if (!w_top) { - // If not, look for its frame window + if (!wid_has_prop(ps, ev->window, ps->atoms->aWM_STATE)) { + log_debug("Window %#010x doesn't have WM_STATE property, it is " + "probably not a client window. But we will listen for " + "property change in case it gains one.", + ev->window); + xcb_change_window_attributes( + ps->c, ev->window, XCB_CW_EVENT_MASK, + (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN) | + XCB_EVENT_MASK_PROPERTY_CHANGE}); + } else { auto w_real_top = find_managed_window_or_parent(ps, ev->parent); - // If found, and the client window has not been determined, or its - // frame may not have a correct client, continue - if (w_real_top && (!w_real_top->client_win || - w_real_top->client_win == w_real_top->base.id)) { - // If it has WM_STATE, mark it the client window - if (wid_has_prop(ps, ev->window, ps->atoms->aWM_STATE)) { - w_real_top->wmwin = false; - win_unmark_client(ps, w_real_top); - win_mark_client(ps, w_real_top, ev->window); - } - // Otherwise, watch for WM_STATE on it + if (w_real_top && w_real_top->state != WSTATE_UNMAPPED && + w_real_top->state != WSTATE_UNMAPPING) { + log_debug("Mark window %#010x (%s) as having a stale " + "client", + w_real_top->base.id, w_real_top->name); + win_set_flags(w_real_top, WIN_FLAGS_CLIENT_STALE); + ps->pending_updates = true; + } else { + if (!w_real_top) + log_debug("parent %#010x not found", ev->parent); else { - xcb_change_window_attributes( - ps->c, ev->window, XCB_CW_EVENT_MASK, - (const uint32_t[]){ - determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN) | - XCB_EVENT_MASK_PROPERTY_CHANGE}); + // Window is not currently mapped, unmark its + // client to trigger a client recheck when it is + // mapped later. + win_unmark_client(ps, w_real_top); + log_debug("parent %#010x (%s) is in state %d", + w_real_top->base.id, w_real_top->name, + w_real_top->state); } } } @@ -451,13 +458,11 @@ static inline void ev_property_notify(session_t *ps, xcb_property_notify_event_t ps, ev->window, WIN_EVMODE_UNKNOWN)}); auto w_top = find_managed_window_or_parent(ps, ev->window); - // Initialize client_win as early as possible - if (w_top && - (!w_top->client_win || w_top->client_win == w_top->base.id) && - wid_has_prop(ps, ev->window, ps->atoms->aWM_STATE)) { - w_top->wmwin = false; - win_unmark_client(ps, w_top); - win_mark_client(ps, w_top, ev->window); + // ev->window might have not been managed yet, in that case w_top + // would be NULL. + if (w_top) { + win_set_flags(w_top, WIN_FLAGS_CLIENT_STALE); + ps->pending_updates = true; } } } diff --git a/src/win.c b/src/win.c index 28d84ed..10f4424 100644 --- a/src/win.c +++ b/src/win.c @@ -53,6 +53,8 @@ static const int WIN_GET_LEADER_MAX_RECURSION = 20; static const int ROUNDED_PIXELS = 1; static const double ROUNDED_PERCENT = 0.05; +static void win_recheck_client(session_t *ps, struct managed_win *w); + /// Generate a "return by value" function, from a function that returns the /// region via a region_t pointer argument. /// Function signature has to be (win *, region_t *) @@ -323,8 +325,8 @@ void win_release_images(struct backend_base *backend, struct managed_win *w) { void win_process_flags(session_t *ps, struct managed_win *w) { if (win_check_flags_all(w, WIN_FLAGS_MAPPED)) { map_win_start(ps, w); + win_clear_flags(w, WIN_FLAGS_MAPPED); } - win_clear_flags(w, WIN_FLAGS_MAPPED); // Not a loop while (win_check_flags_any(w, WIN_FLAGS_IMAGES_STALE) && @@ -367,7 +369,14 @@ void win_process_flags(session_t *ps, struct managed_win *w) { } // Clear stale image flags - win_clear_flags(w, WIN_FLAGS_IMAGES_STALE); + if (win_check_flags_any(w, WIN_FLAGS_IMAGES_STALE)) { + win_clear_flags(w, WIN_FLAGS_IMAGES_STALE); + } + + if (win_check_flags_all(w, WIN_FLAGS_CLIENT_STALE)) { + win_recheck_client(ps, w); + win_clear_flags(w, WIN_FLAGS_CLIENT_STALE); + } } /** @@ -2326,6 +2335,7 @@ win_is_fullscreen_xcb(xcb_connection_t *c, const struct atom *a, const xcb_windo /// Set flags on a window. Some sanity checks are performed void win_set_flags(struct managed_win *w, uint64_t flags) { + log_debug("Set flags %lu to window %#010x (%s)", flags, w->base.id, w->name); if (unlikely(w->state == WSTATE_DESTROYING)) { log_error("Flags set on a destroyed window %#010x (%s)", w->base.id, w->name); return; @@ -2336,6 +2346,7 @@ void win_set_flags(struct managed_win *w, uint64_t flags) { /// Clear flags on a window. Some sanity checks are performed void win_clear_flags(struct managed_win *w, uint64_t flags) { + log_debug("Clear flags %lu from window %#010x (%s)", flags, w->base.id, w->name); if (unlikely(w->state == WSTATE_DESTROYING)) { log_error("Flags cleared on a destroyed window %#010x (%s)", w->base.id, w->name); diff --git a/src/win_defs.h b/src/win_defs.h index f8494bb..6099ac2 100644 --- a/src/win_defs.h +++ b/src/win_defs.h @@ -81,6 +81,8 @@ enum win_flags { WIN_FLAGS_SHADOW_STALE = 8, /// shadow has not been generated WIN_FLAGS_SHADOW_NONE = 16, + /// the client window needs to be updated + WIN_FLAGS_CLIENT_STALE = 32, /// the window is mapped by X, we need to call map_win_start for it WIN_FLAGS_MAPPED = 64, }; From 176718ad3b187f121054261532623234b4c6b387 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 10 Apr 2020 03:26:13 +0100 Subject: [PATCH 3/5] Revert "Revert previous fix for #299" This reverts commit 04fe4a76b23aa1196ffcdb822510e0a3c84ab3b3. This brings back the previous incomplete fix attempt for #299. See the commit message of the revert for why it's incomplete. A different fix is then attempted, see commit xxxxxxx for how that fix works. However, the second fix is incomplete by itself as well. The reason is that i3 reparent the real window to the frame first, before destroying the placeholder client of that frame. So briefly, that frame would have 2 client windows. And the frame is mapped before the placeholder is destroyed. So even though we only call win_recheck_client when/if the frame window is mapped, it can still be called when there are 2 client windows, it would pick up the wrong client window in that case. So what we need is to combine both fixes. The second fix makes sure we are up to date on the client window information when we starts to listen for events on the frame window; while the first fix would keep us up to date afterwards. This revert also includes a fix for assertion failure raised in #371 See #299 for root cause of the bug. Signed-off-by: Yuxuan Shui --- src/event.c | 20 +++++++++++++++++--- src/picom.c | 4 ++-- src/win.c | 10 +++++----- src/win.h | 1 + 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/event.c b/src/event.c index f21e346..69b326b 100644 --- a/src/event.c +++ b/src/event.c @@ -180,8 +180,9 @@ static inline void ev_focus_out(session_t *ps, xcb_focus_out_event_t *ev) { } static inline void ev_create_notify(session_t *ps, xcb_create_notify_event_t *ev) { - assert(ev->parent == ps->root); - add_win_top(ps, ev->window); + if (ev->parent == ps->root) { + add_win_top(ps, ev->window); + } } /// Handle configure event of a regular window @@ -263,8 +264,21 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *ev) { auto w = find_win(ps, ev->window); - if (w) { + auto mw = find_toplevel(ps, ev->window); + if (mw && mw->client_win == mw->base.id) { + // We only want _real_ frame window + assert(&mw->base == w); + mw = NULL; + } + assert(w == NULL || mw == NULL); + + if (w != NULL) { auto _ attr_unused = destroy_win_start(ps, w); + } else if (mw != NULL) { + win_recheck_client(ps, mw); + } else { + log_debug("Received a destroy notify from an unknown window, %#010x", + ev->window); } } diff --git a/src/picom.c b/src/picom.c index bbade20..fcc2a41 100644 --- a/src/picom.c +++ b/src/picom.c @@ -303,9 +303,9 @@ uint32_t determine_evmask(session_t *ps, xcb_window_t wid, win_evmode_t mode) { struct managed_win *w = NULL; // Check if it's a mapped frame window - if (WIN_EVMODE_FRAME == mode || + if (mode == WIN_EVMODE_FRAME || ((w = find_managed_win(ps, wid)) && w->a.map_state == XCB_MAP_STATE_VIEWABLE)) { - evmask |= XCB_EVENT_MASK_PROPERTY_CHANGE; + evmask |= XCB_EVENT_MASK_PROPERTY_CHANGE | XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY; if (!ps->o.use_ewmh_active_win) { evmask |= XCB_EVENT_MASK_FOCUS_CHANGE; } diff --git a/src/win.c b/src/win.c index 10f4424..1a10e06 100644 --- a/src/win.c +++ b/src/win.c @@ -53,8 +53,6 @@ static const int WIN_GET_LEADER_MAX_RECURSION = 20; static const int ROUNDED_PIXELS = 1; static const double ROUNDED_PERCENT = 0.05; -static void win_recheck_client(session_t *ps, struct managed_win *w); - /// Generate a "return by value" function, from a function that returns the /// region via a region_t pointer argument. /// Function signature has to be (win *, region_t *) @@ -1031,6 +1029,8 @@ void win_mark_client(session_t *ps, struct managed_win *w, xcb_window_t client) */ void win_unmark_client(session_t *ps, struct managed_win *w) { xcb_window_t client = w->client_win; + log_debug("Detaching client window %#010x from frame %#010x (%s)", client, + w->base.id, w->name); w->client_win = XCB_NONE; @@ -1074,7 +1074,7 @@ static xcb_window_t find_client_win(session_t *ps, xcb_window_t w) { * @param ps current session * @param w struct _win of the parent window */ -static void win_recheck_client(session_t *ps, struct managed_win *w) { +void win_recheck_client(session_t *ps, struct managed_win *w) { // Initialize wmwin to false w->wmwin = false; @@ -1084,14 +1084,14 @@ static void win_recheck_client(session_t *ps, struct managed_win *w) { // sets override-redirect flags on all frame windows. xcb_window_t cw = find_client_win(ps, w->base.id); if (cw) { - log_trace("(%#010x): client %#010x", w->base.id, cw); + log_debug("(%#010x): client %#010x", w->base.id, cw); } // Set a window's client window to itself if we couldn't find a // client window if (!cw) { cw = w->base.id; w->wmwin = !w->a.override_redirect; - log_trace("(%#010x): client self (%s)", w->base.id, + log_debug("(%#010x): client self (%s)", w->base.id, (w->wmwin ? "wmwin" : "override-redirected")); } diff --git a/src/win.h b/src/win.h index d6778b6..4f1ee41 100644 --- a/src/win.h +++ b/src/win.h @@ -298,6 +298,7 @@ void win_on_win_size_change(session_t *ps, struct managed_win *w); void win_update_wintype(session_t *ps, struct managed_win *w); void win_mark_client(session_t *ps, struct managed_win *w, xcb_window_t client); void win_unmark_client(session_t *ps, struct managed_win *w); +void win_recheck_client(session_t *ps, struct managed_win *w); bool win_get_class(session_t *ps, struct managed_win *w); /** From 660d16f5559d8cecc344373f9b4ae7f51c5149c9 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 10 Apr 2020 04:19:53 +0100 Subject: [PATCH 4/5] win: update mode in win_on_factor_change win_on_factor_change is called when client window changed for a frame, in that case, the mode of the window could change. Related: #299 Signed-off-by: Yuxuan Shui --- src/win.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/win.c b/src/win.c index 1a10e06..c8c284b 100644 --- a/src/win.c +++ b/src/win.c @@ -904,6 +904,7 @@ void win_update_opacity_rule(session_t *ps, struct managed_win *w) { * TODO need better name */ void win_on_factor_change(session_t *ps, struct managed_win *w) { + log_debug("Window %#010x (%s) factor change", w->base.id, w->name); // Focus needs to be updated first, as other rules might depend on the focused // state of the window win_update_focused(ps, w); @@ -911,6 +912,8 @@ void win_on_factor_change(session_t *ps, struct managed_win *w) { win_determine_shadow(ps, w); win_determine_invert_color(ps, w); win_determine_blur_background(ps, w); + w->mode = win_calc_mode(w); + log_debug("Window mode changed to %d", w->mode); win_update_opacity_rule(ps, w); if (w->a.map_state == XCB_MAP_STATE_VIEWABLE) w->paint_excluded = c2_match(ps, w, ps->o.paint_blacklist, NULL); From 3765c13baa26bb44940a9b9c916000822580a3fe Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 11 Apr 2020 01:05:52 +0100 Subject: [PATCH 5/5] event: reparent: mark frame CLIENT_STALE when old client detached Related: #299 Signed-off-by: Yuxuan Shui --- src/event.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/event.c b/src/event.c index 69b326b..c763552 100644 --- a/src/event.c +++ b/src/event.c @@ -320,7 +320,8 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t ev->window, ev->parent, ev->override_redirect); auto w_top = find_toplevel(ps, ev->window); if (w_top) { - win_unmark_client(ps, w_top); + win_set_flags(w_top, WIN_FLAGS_CLIENT_STALE); + ps->pending_updates = true; } if (ev->parent == ps->root) {