chore: switch to use nativeEventFilter #405
Merged
+84
−73
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
此提交也是对后续 Tray SelectionOwner 支持相关提交的预先准备。 Log:
Reviewer's GuideRoute X11 tray handling through Qt’s native event filter instead of a custom XCB thread by making the tray Util a QObject that integrates the XCB connection into the Qt event loop and having XembedProtocol implement QAbstractNativeEventFilter for relevant X11 events, while cleaning up the old thread-based implementation and improving logging and atom utilities. Sequence diagram for X11 tray events via Qt native event filtersequenceDiagram
participant X11 as X11_Server
participant XCB as xcb_connection_t
participant Util as Util
participant QSN as QSocketNotifier
participant QED as QAbstractEventDispatcher
participant XEP as XembedProtocol
X11->>XCB: X11 event available
XCB-->>Util: Readable on X11 fd
Util->>QSN: QSocketNotifier activated
QSN->>Util: activated(fd)
Util->>Util: dispatchEvents(Poll)
loop dispatch all pending events
Util->>XCB: xcb_poll_for_event
XCB-->>Util: xcb_generic_event_t*
Util->>QED: filterNativeEvent("xcb_generic_event_t", event)
alt XembedProtocol handles event
QED->>XEP: nativeEventFilter(eventType, message, result)
XEP->>XEP: handle leave_notify
XEP->>Util: setX11WindowInputShape(window, QSize(0,0))
else other filters or default handling
QED->>QED: other native event processing
end
Util->>Util: free(event)
end
Util->>XCB: xcb_flush
QED->>Util: aboutToBlock()
Util->>Util: dispatchEvents(EventQueue)
QED->>Util: awake()
Util->>Util: dispatchEvents(EventQueue)
Class diagram for updated Util and XembedProtocolclassDiagram
class QObject
class QAbstractNativeEventFilter
class AbstractTrayProtocol
class Util {
+static Util* instance()
+xcb_atom_t getAtomByName(QString name)
+QString getNameByAtom(xcb_atom_t atom)
+xcb_atom_t getAtomFromDisplay(const char* name)
+void moveX11Window(xcb_window_t window, uint32_t x, uint32_t y)
+void setX11WindowSize(xcb_window_t window, QSize size)
+void setX11WindowInputShape(xcb_window_t window, QSize size)
+xcb_connection_t* getX11Connection()
+_XDisplay* getDisplay()
+xcb_ewmh_connection_t* getEwmhConnection()
+xcb_window_t getRootWindow()
+bool isTransparentImage(QImage image)
+QImage convertFromNative(xcb_image_t* image)
+xcb_image_t* convertToNative(const QImage& image)
+xcb_atom_t internAtom(const QString& name)
+void deleteWindowPixmap(xcb_pixmap_t pixmap)
+void deleteWindowCursor(xcb_cursor_t cursor)
+void destroyX11Window(xcb_window_t window)
+void destroyX11SubWindows(xcb_window_t window)
+void setX11WindowVisible(xcb_window_t window, bool visible)
+void setX11WindowProperty(xcb_window_t window, xcb_atom_t property, xcb_atom_t type, uint32_t format, QByteArray data)
+QByteArray getX11WindowProperty(xcb_window_t window, xcb_atom_t property)
+void sendX11ClientMessage(xcb_window_t window, xcb_atom_t type, QVector~uint32_t~ data)
+void sendX11TrayNotify(xcb_window_t window, xcb_atom_t message, QVector~uint32_t~ data)
+void sendX11TrayRequestDock(xcb_window_t window)
+void sendX11TrayBeginMessage(xcb_window_t window, uint32_t size)
+void sendX11TrayCancelMessage(xcb_window_t window)
+void sendX11TrayMessageData(xcb_window_t window, const QByteArray& data)
+void setX11WindowGeometry(xcb_window_t window, QRect rect)
+void setX11WindowStrutPartial(xcb_window_t window, const QVector~uint32_t~& values)
+void setX11WindowSkipTaskbar(xcb_window_t window, bool skip)
+void setX11WindowSkipPager(xcb_window_t window, bool skip)
+void setX11WindowSticky(xcb_window_t window, bool sticky)
+void setX11WindowBelow(xcb_window_t window, bool below)
+void setX11WindowAbove(xcb_window_t window, bool above)
+void setX11WindowTypeDock(xcb_window_t window)
+void setX11WindowTypeNormal(xcb_window_t window)
+void setX11WindowDesktop(xcb_window_t window)
+void setX11WindowStrut(xcb_window_t window, const QVector~uint32_t~& values)
+void setX11WindowIconPixmap(xcb_window_t window, xcb_pixmap_t pixmap)
+void setX11WindowClass(xcb_window_t window, const QByteArray& name)
+void setX11WindowNetWmName(xcb_window_t window, const QString& name)
+void setX11WindowInputShape(xcb_window_t window, int x, int y, int width, int height)
+void clearX11WindowInputShape(xcb_window_t window)
+void setX11WindowShapeRectangles(xcb_window_t window, const QVector~QRect~& rects)
+void setX11WindowOpacity(xcb_window_t window, double opacity)
+void setX11WindowXEmbedInfo(xcb_window_t window, uint32_t flags, uint32_t version)
+xcb_window_t createX11Window(int x, int y, int width, int height, uint32_t eventMask)
+xcb_window_t createTransparentX11Window(int x, int y, int width, int height, uint32_t eventMask)
+xcb_pixmap_t createX11PixmapFromImage(const QImage& image)
+xcb_cursor_t createX11CursorFromImage(const QImage& image)
+void dispatchEvents(DispatchEventsMode mode)
-Util()
-~Util()
-xcb_connection_t* m_x11connection
-xcb_ewmh_connection_t m_ewmh
-xcb_window_t m_rootWindow
-_XDisplay* m_display
-QSet~QString~ m_currentIds
<<enum>> DispatchEventsMode
DispatchEventsMode Poll
DispatchEventsMode EventQueue
}
class XembedProtocol {
+XembedProtocol(QObject* parent)
+~XembedProtocol()
+void onTrayIconsChanged()
+void onTrayIconAdded(QString id)
+void onTrayIconRemoved(QString id)
+void setManager(WindowId manager)
+void registerTrayIcon(WindowId window)
+void unregisterTrayIcon(WindowId window)
+void updateTrayIconGeometry(WindowId window, QRect geometry)
+void handleTrayMessage(WindowId window, QByteArray message)
+void handleTraySelectionCleared()
+void handleTrayManagerDestroyed()
+void activateWindow(WindowId window)
+void sendClick(WindowId window, QPoint pos)
+void sendDoubleClick(WindowId window, QPoint pos)
+void sendContextMenu(WindowId window, QPoint pos)
+void sendScroll(WindowId window, QPoint pos, int delta, int orientation)
+void sendKey(WindowId window, int key, int modifiers)
+void focusIn(WindowId window)
+void focusOut(WindowId window)
+bool nativeEventFilter(QByteArray eventType, void* message, qintptr* result)
-QSharedPointer~TrayManager~ m_trayManager
-QSet~WindowId~ m_registedItem
}
Util --|> QObject
XembedProtocol --|> AbstractTrayProtocol
XembedProtocol --|> QAbstractNativeEventFilter
class XcbThread {
+XcbThread(xcb_connection_t* connection)
+~XcbThread()
+void run()
-xcb_connection_t* m_connection
}
XcbThread ..x Util : removed_dependency
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来分析一下这段代码的改进：
建议的进一步改进：
void Util::dispatchEvents(DispatchEventsMode mode)
{
xcb_connection_t *connection = m_x11connection;
if (!connection) {
qCWarning(TRAYUTIL, "Attempting to dispatch X11 events with no connection");
return;
}
auto pollEventFunc = mode == DispatchEventsMode::Poll ? xcb_poll_for_event : xcb_poll_for_queued_event;
// 添加事件计数限制，防止事件处理阻塞
int eventCount = 0;
const int maxEvents = 100; // 设置最大处理事件数
while (xcb_generic_event_t *event = pollEventFunc(connection)) {
if (++eventCount >= maxEvents) {
qCWarning(TRAYUTIL, "Too many events pending, processing later");
break;
}
qintptr result = 0;
QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher();
if (!dispatcher) {
qCWarning(TRAYUTIL, "No event dispatcher available");
free(event);
break;
}
dispatcher->filterNativeEvent(QByteArrayLiteral("xcb_generic_event_t"), event, &result);
free(event);
}
if (eventCount > 0) {
xcb_flush(connection);
}
}
bool XembedProtocol::nativeEventFilter(const QByteArray &eventType, void *message, qintptr *result)
{
Q_UNUSED(result)
if (eventType != "xcb_generic_event_t") {
return false;
}
if (!message) {
qCWarning(TRAYUTIL, "Received null event message");
return false;
}
auto *ev = static_cast<xcb_generic_event_t *>(message);
if (!ev) {
return false;
}
const auto responseType = XCB_EVENT_RESPONSE_TYPE(ev);
if (responseType == XCB_LEAVE_NOTIFY) {
xcb_leave_notify_event_t *lE = reinterpret_cast<xcb_leave_notify_event_t*>(ev);
if (lE && lE->event) {
UTIL->setX11WindowInputShape(lE->event, QSize(0, 0));
return true;
}
}
return false;
}
Util::Util()
: QObject()
{
m_x11connection = xcb_connect(nullptr, nullptr);
if (!m_x11connection) {
qCCritical(TRAYUTIL, "Failed to connect to X server");
return;
}
m_display = XOpenDisplay("");
if (!m_display) {
qCCritical(TRAYUTIL, "Failed to open X display");
xcb_disconnect(m_x11connection);
m_x11connection = nullptr;
return;
}
// ... 其余代码
}这些改进主要增加了：
这些改进能够提高代码的健壮性和可靠性，同时保持良好的性能。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In Util::dispatchEvents, consider guarding against a null QAbstractEventDispatcher from QCoreApplication::eventDispatcher() before calling filterNativeEvent to avoid potential crashes during early startup or shutdown.
- The new Util::getAtomFromDisplay helper is slightly confusing given the existing getAtomByName(QString); it may be clearer to either delegate directly to getAtomByName with a QString name or rename/adjust the implementation to better reflect what xcb_atom_name_by_screen returns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In Util::dispatchEvents, consider guarding against a null QAbstractEventDispatcher from QCoreApplication::eventDispatcher() before calling filterNativeEvent to avoid potential crashes during early startup or shutdown.
- The new Util::getAtomFromDisplay helper is slightly confusing given the existing getAtomByName(QString); it may be clearer to either delegate directly to getAtomByName with a QString name or rename/adjust the implementation to better reflect what xcb_atom_name_by_screen returns.
## Individual Comments
### Comment 1
<location> `plugins/application-tray/util.cpp:52-57` </location>
<code_context>
+ return;
+ }
+
+ auto pollEventFunc = mode == DispatchEventsMode::Poll ? xcb_poll_for_event : xcb_poll_for_queued_event;
+
+ while (xcb_generic_event_t *event = pollEventFunc(connection)) {
+ qintptr result = 0;
+
+ QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher();
+ dispatcher->filterNativeEvent(QByteArrayLiteral("xcb_generic_event_t"), event, &result);
+ free(event);
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against a null QAbstractEventDispatcher before calling filterNativeEvent.
`QCoreApplication::eventDispatcher()` may return `nullptr` during early startup or shutdown, which would cause a crash when calling `dispatcher->filterNativeEvent(...)`. Please add a null check and skip processing if no dispatcher is available.
</issue_to_address>
### Comment 2
<location> `plugins/application-tray/xembedprotocolhandler.cpp:35` </location>
<code_context>
: AbstractTrayProtocol(parent)
, m_trayManager(new TrayManager("org.deepin.dde.TrayManager1", "/org/deepin/dde/TrayManager1", QDBusConnection::sessionBus(), this))
{
+ qApp->installNativeEventFilter(this);
+
m_trayManager->Manage();
</code_context>
<issue_to_address>
**issue (bug_risk):** Remove the native event filter in the destructor to avoid a dangling pointer in QCoreApplication.
`QCoreApplication` stores a raw pointer to the installed `QAbstractNativeEventFilter`. If `XembedProtocol` is destroyed without unregistering, the app may still invoke `nativeEventFilter(...)` on a freed object. Add `qApp->removeNativeEventFilter(this);` in the destructor to prevent a use-after-free during shutdown or protocol replacement.
</issue_to_address>
### Comment 3
<location> `plugins/application-tray/util.cpp:185-187` </location>
<code_context>
return name;
}
+xcb_atom_t Util::getAtomFromDisplay(const char * name)
+{
+ return getAtomByName(xcb_atom_name_by_screen(name, DefaultScreen(getDisplay())));
+}
+
</code_context>
<issue_to_address>
**question:** Clarify the intent and correctness of getAtomFromDisplay’s Xlib/XCB interop and helper usage.
This currently mixes Xlib (`DefaultScreen(getDisplay())`) with an XCB helper, then forwards to `getAtomByName`. Please explain why this indirection is needed instead of calling `getAtomByName(QString::fromLatin1(name))` directly, and confirm that `xcb_atom_name_by_screen(...)` behaves as expected here (no extra round-trips, correct encoding/screen handling). If it’s equivalent, simplifying to the direct call would reduce Xlib/XCB interop complexity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+52
to
+57
| auto pollEventFunc = mode == DispatchEventsMode::Poll ? xcb_poll_for_event : xcb_poll_for_queued_event; | ||
|
|
||
| while (xcb_generic_event_t *event = pollEventFunc(connection)) { | ||
| qintptr result = 0; | ||
|
|
||
| QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher(); |
There was a problem hiding this comment.
issue (bug_risk): Guard against a null QAbstractEventDispatcher before calling filterNativeEvent.
QCoreApplication::eventDispatcher() may return nullptr during early startup or shutdown, which would cause a crash when calling dispatcher->filterNativeEvent(...). Please add a null check and skip processing if no dispatcher is available.
| : AbstractTrayProtocol(parent) | ||
| , m_trayManager(new TrayManager("org.deepin.dde.TrayManager1", "/org/deepin/dde/TrayManager1", QDBusConnection::sessionBus(), this)) | ||
| { | ||
| qApp->installNativeEventFilter(this); |
There was a problem hiding this comment.
issue (bug_risk): Remove the native event filter in the destructor to avoid a dangling pointer in QCoreApplication.
QCoreApplication stores a raw pointer to the installed QAbstractNativeEventFilter. If XembedProtocol is destroyed without unregistering, the app may still invoke nativeEventFilter(...) on a freed object. Add qApp->removeNativeEventFilter(this); in the destructor to prevent a use-after-free during shutdown or protocol replacement.
Comment on lines
+185
to
+187
| xcb_atom_t Util::getAtomFromDisplay(const char * name) | ||
| { | ||
| return getAtomByName(xcb_atom_name_by_screen(name, DefaultScreen(getDisplay()))); |
There was a problem hiding this comment.
question: Clarify the intent and correctness of getAtomFromDisplay’s Xlib/XCB interop and helper usage.
This currently mixes Xlib (DefaultScreen(getDisplay())) with an XCB helper, then forwards to getAtomByName. Please explain why this indirection is needed instead of calling getAtomByName(QString::fromLatin1(name)) directly, and confirm that xcb_atom_name_by_screen(...) behaves as expected here (no extra round-trips, correct encoding/screen handling). If it’s equivalent, simplifying to the direct call would reduce Xlib/XCB interop complexity.
tsic404
approved these changes
Dec 25, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, tsic404 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
此提交也是对后续 Tray SelectionOwner 支持相关提交的预先准备。
Log:
Summary by Sourcery
Replace the custom XCB event thread with Qt’s native event filtering and integrate X11 event dispatch into the main event loop for the tray subsystem.
Enhancements: