Skip to content

Conversation

@wineee
Copy link
Member

@wineee wineee commented Dec 12, 2024

No description provided.


QAbstractEventDispatcher *dispatcher = QThread::currentThread()->eventDispatcher();
QObject::connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, q, processWaylandEvents);
QAbstractEventDispatcher *dispatcher = QAbstractEventDispatcher::instance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The implementation is actually the same as QThread::currentThread()->eventDispatcher();

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a crash in processWaylandEvents by refactoring the event processing code and adding better error handling. The main changes include splitting the inline lambda into separate methods, adding file descriptor validation, and improving logging.

Key changes:

  • Split the inline processWaylandEvents lambda into two dedicated methods: dispatchEvents() for event processing and flush() for client flushing
  • Added file descriptor validation with fatal error logging when wl_event_loop_get_fd() returns -1
  • Migrated from QDebug to QLoggingCategory for better structured logging with the category "waylib.server.core"

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/server/kernel/wsocket.cpp Removed unused forward declaration and simplified variable initialization in addClient()
src/server/kernel/wserver.cpp Refactored event processing into separate methods, added FD validation, migrated to QLoggingCategory, and changed event dispatcher retrieval to use QCoreApplication
src/server/kernel/private/wserver_p.h Added override to destructor and declared new dispatchEvents() and flush() methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
int ret = wl_event_loop_dispatch(loop, 0);
if (ret)
qCCritical(qLcWlrServer, "wl_event_loop_dispatch error: %d\n", ret);
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The error message includes a trailing newline character which is unnecessary. Qt's logging macros automatically add newlines. This is inconsistent with other qCCritical and qCFatal usage in the codebase (see line 109 and other files like wforeigntoplevelv1.cpp line 72, wcursorimage.cpp line 666) which don't include the trailing newline.

Suggested change
qCCritical(qLcWlrServer, "wl_event_loop_dispatch error: %d\n", ret);
qCCritical(qLcWlrServer, "wl_event_loop_dispatch error: %d", ret);

Copilot uses AI. Check for mistakes.
}

sockNot.reset();
QThread::currentThread()->eventDispatcher()->disconnect(q);
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

Inconsistent event dispatcher retrieval in the stop() method. The init() method was changed to use QCoreApplication::eventDispatcher() (line 117), but stop() still uses QThread::currentThread()->eventDispatcher(). These should be consistent to avoid potential issues if the event dispatcher changes or if the server is accessed from different threads.

Suggested change
QThread::currentThread()->eventDispatcher()->disconnect(q);
QCoreApplication::eventDispatcher()->disconnect(q);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants