Skip to content

fix: create XWayland wrappers before initial xprop reads caused by #948 #1031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LFRon wants to merge 1 commit into
base: master
Choose a base branch
from
+130 −92
Open

fix: create XWayland wrappers before initial xprop reads caused by #948 #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 27 additions & 20 deletions src/core/shellhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,8 +603,10 @@ void ShellHandler::onXWaylandSurfaceAdded(WXWaylandSurface *surface)
self->m_pendingAppIdResolveToplevels.indexOf(raw);
if (idx < 0)
return; // removed before callback
self->fetchInitialProperties(raw, appId);
self->m_pendingAppIdResolveToplevels.removeAt(idx);
self->ensureXwaylandWrapper(raw, appId);
if (auto current = surface.data())
self->fetchInitialProperties(current);
});
if (started) {
qCDebug(lcTlShell)
Expand All @@ -619,8 +621,10 @@ void ShellHandler::onXWaylandSurfaceAdded(WXWaylandSurface *surface)
}
}
}
// Async path not taken: directly fetch properties then match/create
fetchInitialProperties(raw, QString());
// Async path not taken: create/match first so the wrapper observes
// the initial map lifecycle, then fetch optional xprops.
ensureXwaylandWrapper(raw, QString());
fetchInitialProperties(raw);
});
surface->safeConnect(&WXWaylandSurface::aboutToDissociate, this, [this, surface] {
auto wrapper = m_rootSurfaceContainer->getSurface(surface);
Expand Down Expand Up @@ -657,50 +661,53 @@ void ShellHandler::onXWaylandSurfaceAdded(WXWaylandSurface *surface)
});
}

void ShellHandler::fetchInitialProperties(WXWaylandSurface *surface, const QString &appId)
void ShellHandler::fetchInitialProperties(WXWaylandSurface *surface)
{
if (!m_imCandidatePanelManager)
return;

auto *wrapper = m_rootSurfaceContainer->getSurface(surface);
if (!wrapper || wrapper->isIMCandidatePanel())
return;

auto *xwayland = surface->xwayland();
if (!xwayland) {
ensureXwaylandWrapper(surface, appId);
if (!xwayland)
return;
}

auto windowId = surface->handle()->handle()->window_id;
QVector<WXWayland::AsyncPropRequest> requests;
if (m_imCandidatePanelManager) {
requests.append({ m_imCandidatePanelManager->imCandidatePanelAtom(), XCB_ATOM_CARDINAL });
}

if (requests.isEmpty()) {
ensureXwaylandWrapper(surface, appId);
auto atom = m_imCandidatePanelManager->imCandidatePanelAtom();
if (atom == XCB_ATOM_NONE)
return;
}

QVector<WXWayland::AsyncPropRequest> requests;
requests.append({ atom, XCB_ATOM_CARDINAL });

xwayland->readAsyncProperties(
windowId,
requests,
50,
[self = QPointer<ShellHandler>(this),
surface = QPointer<WXWaylandSurface>(surface),
appId](xcb_window_t, const QMap<xcb_atom_t, QByteArray> &result) {
surface = QPointer<WXWaylandSurface>(surface)](xcb_window_t,
const QMap<xcb_atom_t, QByteArray> &result) {
auto *raw = surface.data();
if (!raw || !self)
return;
self->onInitialPropertiesReady(raw, appId, result);
self->onInitialPropertiesReady(raw, result);
});
}

void ShellHandler::onInitialPropertiesReady(WXWaylandSurface *surface,
const QString &appId,
const QMap<xcb_atom_t, QByteArray> &result)
{
if (m_imCandidatePanelManager) {
bool value = IMCandidatePanelManager::parseIMCandidatePanelProperty(
result,
m_imCandidatePanelManager->imCandidatePanelAtom());
surface->setProperty("imCandidatePanel", value);
auto *wrapper = m_rootSurfaceContainer->getSurface(surface);
if (wrapper)
m_imCandidatePanelManager->checkAndApplyIMCandidatePanel(wrapper, surface);
}
ensureXwaylandWrapper(surface, appId);
}

void ShellHandler::ensureXwaylandWrapper(WXWaylandSurface *surface, const QString &targetAppId)
Expand Down
6 changes: 2 additions & 4 deletions src/core/shellhandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,9 @@ private Q_SLOTS:
// Unified parent/container update for Xdg & XWayland toplevel wrappers.
void updateWrapperContainer(SurfaceWrapper *wrapper,
WAYLIB_SERVER_NAMESPACE::WSurface *parentSurface);
// Async X11 property fetch for XWayland surfaces
void fetchInitialProperties(WAYLIB_SERVER_NAMESPACE::WXWaylandSurface *surface,
const QString &appId);
// Async X11 property fetch for XWayland surfaces after wrapper creation.
void fetchInitialProperties(WAYLIB_SERVER_NAMESPACE::WXWaylandSurface *surface);
void onInitialPropertiesReady(WAYLIB_SERVER_NAMESPACE::WXWaylandSurface *surface,
const QString &appId,
const QMap<xcb_atom_t, QByteArray> &result);

WAYLIB_SERVER_NAMESPACE::WXdgShell *m_xdgShell = nullptr;
Expand Down
5 changes: 5 additions & 0 deletions src/surface/surfacewrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2124,6 +2124,11 @@ void SurfaceWrapper::setHasInitializeContainer(bool value)
// m_prelaunchSplash can't get mapped signal
createNewOrClose(OPEN_ANIMATION);
}

if (!m_prelaunchSplash && value && surface() && surface()->mapped()
&& !m_hasActiveCapability.testFlag(ActiveControlState::MappedOrSplash)) {
onMappedChanged();
}
}

void SurfaceWrapper::disableWindowAnimation(bool disable)
Expand Down
164 changes: 96 additions & 68 deletions waylib/src/server/protocols/wxwayland.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,24 @@
// Async property reading
struct PerWindowProps
{
xcb_window_t windowId = XCB_WINDOW_NONE;
QVector<WXWayland::AsyncPropRequest> requests;
QMap<xcb_atom_t, QByteArray> results;
QVector<xcb_get_property_cookie_t> cookies;
QVector<bool> done;
std::function<void(xcb_window_t, const QMap<xcb_atom_t, QByteArray> &)> callback;
QTimer *timer = nullptr;
bool propNotifySeen = false;
QTimer *timeoutTimer = nullptr;
QTimer *pollTimer = nullptr;
};

QMap<xcb_window_t, PerWindowProps> asyncProps;

@sourcery-ai sourcery-ai Bot Jun 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider simplifying the async property infrastructure by reusing window-based keys, centralizing polling and timeouts, and encoding completion in cookies instead of extra state and helper functions.

You can keep the new robustness while simplifying a few key areas. The main opportunities are:

  1. Use xcb_window_t as the primary key, and track multiple requests per window inside.
  2. Use a single shared poll timer instead of per-request pollTimers.
  3. Drop the parallel done vector by encoding completion in cookies.
  4. Simplify cleanup by removing cleanupAsyncRequest indirection.

Below are concrete, incremental changes that keep your current behavior (timeouts, discard of pending replies, etc.) but reduce moving parts.

1. Go back to QMap<xcb_window_t, PerWindowProps>, allow multiple requests per window internally

Instead of QMap<quint64, PerWindowProps> and a synthetic requestId, you can keep the map keyed by window and add an inner per-request structure if you really need multiple concurrent sessions per window:

struct AsyncPropSession
{
    QVector<WXWayland::AsyncPropRequest> requests;
    QMap<xcb_atom_t, QByteArray> results;
    QVector<xcb_get_property_cookie_t> cookies;
    std::function<void(xcb_window_t, const QMap<xcb_atom_t, QByteArray> &)> callback;
    QElapsedTimer deadline; // or qint64 deadlineMs
};

struct PerWindowProps
{
    QVector<AsyncPropSession> sessions;
};

QMap<xcb_window_t, PerWindowProps> asyncProps;

Then cancelAsyncProperties(windowId) stays a simple find(windowId):

void WXWayland::cancelAsyncProperties(xcb_window_t windowId)
{
    W_D(WXWayland);
    auto it = d->asyncProps.find(windowId);
    if (it == d->asyncProps.end())
        return;

    auto *conn = xcbConnection();
    if (conn) {
        for (auto &session : it->sessions) {
            for (const auto &cookie : session.cookies) {
                if (cookie.sequence)
                    xcb_discard_reply(conn, cookie.sequence);
            }
        }
    }
    d->asyncProps.erase(it);
}

This keeps multi-session support without global requestId bookkeeping or iterator invalidation issues across a QMap<quint64, …>.

2. Replace per-request pollTimer with a single shared poll timer

Instead of creating one pollTimer per request, keep a single timer in WXWaylandPrivate that drives xcbPollReplies() as long as there are pending sessions:

// In WXWaylandPrivate
QTimer *pollTimer = nullptr;

// init once
WXWaylandPrivate::WXWaylandPrivate()
{
    pollTimer = new QTimer(q);
    connect(pollTimer, &QTimer::timeout, q, [this]() { xcbPollReplies(); });
    pollTimer->setInterval(1); // or a slightly larger value if acceptable
    pollTimer->setSingleShot(false);
}

Start/stop it based on asyncProps emptiness:

void WXWayland::readWindowPropertiesAsync(...)
{
    W_D(WXWayland);
    // ... set up session ...
    d->asyncProps[windowId].sessions.append(std::move(session));

    if (!d->pollTimer->isActive())
        d->pollTimer->start();
}

And in xcbPollReplies():

void WXWaylandPrivate::xcbPollReplies()
{
    if (asyncProps.isEmpty()) {
        pollTimer->stop();
        return;
    }

    // ... iterate windows/sessions, read replies ...

    if (asyncProps.isEmpty())
        pollTimer->stop();
}

This removes all per-request pollTimer allocation and the need to manage them in cleanupAsyncRequest.

3. Replace QVector<bool> done with “cookie cleared” as completion flag

You can drop the done vector by treating a cleared cookie (sequence == 0) as “done”. This keeps the same semantics but avoids parallel arrays.

When you successfully poll or decide a cookie is finished (including timeout/discard), clear its sequence:

// helper
inline bool isCookiePending(const xcb_get_property_cookie_t &cookie)
{
    return cookie.sequence != 0;
}

inline void markCookieDone(xcb_get_property_cookie_t &cookie)
{
    cookie.sequence = 0;
}

Use in xcbPollReplies():

for (int i = 0; i < session.cookies.size(); ++i) {
    auto &cookie = session.cookies[i];
    if (!isCookiePending(cookie))
        continue;

    void *replyPtr = nullptr;
    xcb_generic_error_t *err = nullptr;
    int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);

    if (ret == 0) {
        windowDone = false;
        continue;
    }

    markCookieDone(cookie);

    if (ret == 1 && replyPtr) {
        auto *reply = static_cast<xcb_get_property_reply_t *>(replyPtr);
        if (reply->type != 0 && reply->value_len > 0) {
            QByteArray data(static_cast<const char *>(xcb_get_property_value(reply)),
                            xcb_get_property_value_length(reply));
            session.results.insert(session.requests[i].atom, data);
        }
        free(reply);
    } else if (err) {
        free(err);
    }
}

Completion check becomes:

bool windowDone = std::all_of(session.cookies.begin(), session.cookies.end(),
                              [](const auto &c) { return !isCookiePending(c); });

Timeout handling uses the same mechanism and calls xcb_discard_reply before clearing:

for (int i = 0; i < session.cookies.size(); ++i) {
    auto &cookie = session.cookies[i];
    if (!isCookiePending(cookie))
        continue;

    void *replyPtr = nullptr;
    xcb_generic_error_t *err = nullptr;
    int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);
    if (ret == 0 && cookie.sequence)
        xcb_discard_reply(conn, cookie.sequence);

    markCookieDone(cookie);
    // ... same result insertion / free logic ...
}

This preserves all your new behavior (including discarding unconsumed replies) without the additional done vector and its synchronization cost.

4. Simplify cleanup, avoid cleanupAsyncRequest as a separate concept

If you:

  • key by xcb_window_t,
  • use a single shared pollTimer,
  • encode completion in cookies,

then cleanupAsyncRequest can be inlined into the point where you decide a session is done:

void WXWaylandPrivate::xcbPollReplies()
{
    W_Q(WXWayland);
    xcb_connection_t *conn = q->xcbConnection();
    if (!conn)
        return;

    for (auto winIt = asyncProps.begin(); winIt != asyncProps.end();) {
        auto &perWindow = winIt.value();

        for (int s = 0; s < perWindow.sessions.size();) {
            auto &session = perWindow.sessions[s];
            // ... poll replies, update cookies/results ...

            bool sessionDone = std::all_of(session.cookies.begin(), session.cookies.end(),
                                           [](const auto &c) { return !isCookiePending(c); });

            if (sessionDone) {
                auto callback = std::move(session.callback);
                auto windowId = winIt.key();
                auto results = session.results;
                perWindow.sessions.remove(s);
                callback(windowId, results);
            } else {
                ++s;
            }
        }

        if (perWindow.sessions.isEmpty())
            winIt = asyncProps.erase(winIt);
        else
            ++winIt;
    }
}

Timeouts can be handled similarly via a single timer that checks deadlines per session instead of maintaining a timeoutTimer per request. If you want to keep a per-request timeout, you can store the expiry time in AsyncPropSession instead of spawning another QTimer.

These changes preserve the functional improvements you added (timeouts, discard of pending replies, more robust async handling) while:

  • removing artificial requestId indirection,
  • centralizing polling into one timer,
  • eliminating QVector<bool> done,
  • and simplifying lifecycle / cleanup logic.

QMap<quint64, PerWindowProps> asyncProps;
quint64 nextAsyncPropRequestId = 1;

void xcbPollReplies();
void xcbAsyncTimeoutForWindow(xcb_window_t windowId);
void xcbAsyncTimeoutForRequest(quint64 requestId);
void cleanupAsyncRequest(QMap<quint64, PerWindowProps>::iterator it, bool discardPending);

W_DECLARE_PUBLIC(WXWayland)

Check warning on line 75 in waylib/src/server/protocols/wxwayland.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

There is an unknown macro here somewhere. Configuration is required. If W_DECLARE_PUBLIC is a macro then please configure it.

xcb_screen_t *screen = nullptr;

Expand Down Expand Up @@ -101,28 +105,9 @@

auto *d = self->d_func();

// Trigger async property reading infrastructure if this window is being tracked.
// Progress any pending async property reads while X11 events are flowing.
if (!d->asyncProps.isEmpty()) {
d->xcbPollReplies();
auto it = d->asyncProps.find(pe->window);
if (it != d->asyncProps.end()) {
auto &props = it.value();
props.propNotifySeen = true;
// Resend requests on PROPNOTIFY to get the latest value after the change.
xcb_connection_t *conn = self->xcbConnection();
props.cookies.clear();
for (const auto &req : std::as_const(props.requests)) {
auto cookie = xcb_get_property_unchecked(conn,
false,
pe->window,
req.atom,
req.type,
0,
1024);
props.cookies.append(cookie);
}
xcb_flush(conn);
}
}

const auto &list = self->surfaceList();
Expand Down Expand Up @@ -477,8 +462,10 @@
}

WXWaylandPrivate::PerWindowProps props;
props.windowId = windowId;
props.requests = requests;
props.callback = std::move(callback);
props.done.resize(requests.size());

props.cookies.reserve(requests.size());
for (const auto &req : std::as_const(requests)) {
Expand All @@ -488,15 +475,24 @@
}
xcb_flush(conn);

// Create per-window timer.
props.timer = new QTimer(this);
props.timer->setSingleShot(true);
connect(props.timer, &QTimer::timeout, this, [d, windowId]() {
d->xcbAsyncTimeoutForWindow(windowId);
const quint64 requestId = d->nextAsyncPropRequestId++;
if (d->nextAsyncPropRequestId == 0)
d->nextAsyncPropRequestId = 1;

props.timeoutTimer = new QTimer(this);
props.timeoutTimer->setSingleShot(true);
connect(props.timeoutTimer, &QTimer::timeout, this, [d, requestId]() {
d->xcbAsyncTimeoutForRequest(requestId);
});
props.timeoutTimer->start(timeoutMs);

props.pollTimer = new QTimer(this);
connect(props.pollTimer, &QTimer::timeout, this, [d]() {
d->xcbPollReplies();
});
props.timer->start(timeoutMs);
props.pollTimer->start(1);

d->asyncProps.insert(windowId, std::move(props));
d->asyncProps.insert(requestId, std::move(props));
}

void WXWaylandPrivate::xcbPollReplies()
Expand All @@ -508,16 +504,21 @@

xcb_connection_t *conn = q->xcbConnection();

QList<xcb_window_t> toRemove;
struct CompletedRequest
{
xcb_window_t windowId = XCB_WINDOW_NONE;
QMap<xcb_atom_t, QByteArray> results;
std::function<void(xcb_window_t, const QMap<xcb_atom_t, QByteArray> &)> callback;
};
QVector<CompletedRequest> completed;

for (auto it = asyncProps.begin(); it != asyncProps.end(); ++it) {
xcb_window_t windowId = it.key();
for (auto it = asyncProps.begin(); it != asyncProps.end();) {
auto &props = it.value();

bool windowDone = true;
for (int i = 0; i < props.cookies.size(); ++i) {
if (props.results.contains(props.requests[i].atom))
continue; // already got this one
if (props.done[i])
continue;

void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
Comment on lines 518 to 524

@sourcery-ai sourcery-ai Bot Jun 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): windowDone never becomes false when replies are not ready, causing premature completion.

In the new xcbPollReplies loop, windowDone stays true even when xcb_poll_for_reply64 returns 0 or an error, and props.done[i] is set unconditionally. This means a failed or pending poll can still mark the whole window as complete. Only set props.done[i] and keep windowDone as true when ret == 1, and set windowDone = false when ret == 0 or on error so incomplete properties don’t prematurely complete the request.

Expand All @@ -528,6 +529,8 @@
continue;
}

props.done[i] = true;

if (ret == 1 && replyPtr) {
auto *reply = static_cast<xcb_get_property_reply_t *>(replyPtr);
if (reply->type != 0 && reply->value_len > 0) {
Expand All @@ -541,33 +544,28 @@
}
}

// Fire callback when all replies received AND propNotifySeen.
if (windowDone && props.propNotifySeen) {
auto resultCopy = props.results;
props.callback(windowId, resultCopy);

toRemove.append(windowId);
if (windowDone) {
CompletedRequest request;
request.windowId = props.windowId;
request.results = props.results;
request.callback = std::move(props.callback);
completed.append(std::move(request));
cleanupAsyncRequest(it, false);
it = asyncProps.erase(it);
} else {
++it;
}
}

// Cleanup completed windows.
for (auto windowId : std::as_const(toRemove)) {
auto it = asyncProps.find(windowId);
if (it != asyncProps.end()) {
if (it->timer) {
it->timer->stop();
delete it->timer;
}
asyncProps.erase(it);
}
}
for (auto &request : completed)
request.callback(request.windowId, request.results);
}

void WXWaylandPrivate::xcbAsyncTimeoutForWindow(xcb_window_t windowId)
void WXWaylandPrivate::xcbAsyncTimeoutForRequest(quint64 requestId)
{
W_Q(WXWayland);

auto it = asyncProps.find(windowId);
auto it = asyncProps.find(requestId);
if (it == asyncProps.end())
return;

Expand All @@ -577,11 +575,12 @@
if (conn) {
// Drain remaining replies.
for (int i = 0; i < props.cookies.size(); ++i) {
if (props.results.contains(props.requests[i].atom))
if (props.done[i])
continue;
void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
int ret = xcb_poll_for_reply64(conn, props.cookies[i].sequence, &replyPtr, &err);
props.done[i] = true;
if (ret == 1 && replyPtr) {
auto *reply = static_cast<xcb_get_property_reply_t *>(replyPtr);
if (reply->type != 0 && reply->value_len > 0) {
Expand All @@ -595,31 +594,60 @@
free(replyPtr);
if (err)
free(err);
if (ret == 0 && props.cookies[i].sequence)
xcb_discard_reply(conn, props.cookies[i].sequence);
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
}
}
}

auto windowId = props.windowId;
auto resultCopy = props.results;
props.callback(windowId, resultCopy);
auto callback = std::move(props.callback);
cleanupAsyncRequest(it, false);
asyncProps.erase(it);
callback(windowId, resultCopy);
}

void WXWaylandPrivate::cleanupAsyncRequest(QMap<quint64, PerWindowProps>::iterator it,
bool discardPending)
{
W_Q(WXWayland);
auto &props = it.value();

if (discardPending) {
auto *conn = q->xcbConnection();
if (conn) {
for (int i = 0; i < props.cookies.size(); ++i) {
if (!props.done[i] && props.cookies[i].sequence)
xcb_discard_reply(conn, props.cookies[i].sequence);
}
}
}

// Cleanup.
if (props.timer) {
delete props.timer;
if (props.timeoutTimer) {
props.timeoutTimer->stop();
props.timeoutTimer->deleteLater();
props.timeoutTimer = nullptr;
}

if (props.pollTimer) {
props.pollTimer->stop();
props.pollTimer->deleteLater();
props.pollTimer = nullptr;
}
asyncProps.erase(it);
}

void WXWayland::cancelAsyncProperties(xcb_window_t windowId)
{
W_D(WXWayland);
auto it = d->asyncProps.find(windowId);
if (it == d->asyncProps.end())
return;

if (it->timer) {
delete it->timer;
for (auto it = d->asyncProps.begin(); it != d->asyncProps.end();) {
if (it.value().windowId == windowId) {
d->cleanupAsyncRequest(it, true);
it = d->asyncProps.erase(it);
} else {
++it;
}
}
d->asyncProps.erase(it);
}

bool WXWayland::event(QEvent *ev)
Expand Down
Loading