Skip to content

Comments

Add a public EventLoop class#85

Merged
MiKom merged 13 commits intomainfrom
wip/eventLoop
Sep 5, 2025
Merged

Add a public EventLoop class#85
MiKom merged 13 commits intomainfrom
wip/eventLoop

Conversation

@akreuzkamp
Copy link
Contributor

Refactors the eventloop functionality of CoreApplication into its own EventLoop class. This EventLoop class can be used to execute event loops on worker threads.

Renames CoreApplication::eventLoop() to
CoreApplication::platformEventLoop() and make
CoreApplication::eventLoop() return a pointer to the new EventLoop type.

@akreuzkamp akreuzkamp requested review from MiKom, Copilot and lemirep July 30, 2025 11:41
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 refactors the event loop functionality from CoreApplication into a new standalone EventLoop class. This allows event loops to be created and executed on worker threads, enabling better thread-based event processing.

  • Extracts event loop functionality from CoreApplication into a new public EventLoop class
  • Renames CoreApplication::eventLoop() to platformEventLoop() and makes eventLoop() return the new EventLoop type
  • Updates all references to use the appropriate event loop interface

Reviewed Changes

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

Show a summary per file
File Description
tests/auto/gui/gui_application/tst_gui_application.cpp New test file for GuiApplication that validates EventLoop usage on worker threads
tests/auto/gui/gui_application/CMakeLists.txt CMake configuration for the new GuiApplication tests
tests/auto/gui/CMakeLists.txt Adds gui_application subdirectory to test build
tests/auto/foundation/object/tst_object.cpp Updated tests to include worker thread EventLoop scenarios
tests/auto/foundation/core_application/tst_core_application.cpp Refactored tests to use new EventLoop API and added worker thread test cases
tests/auto/foundation/common/event_mockups.h Extracted shared test event classes into common header
src/KDGui/platform/linux/wayland/linux_wayland_platform_integration.cpp Moved Wayland initialization logic to constructor and removed event loop initialization
src/KDGui/platform/linux/wayland/linux_wayland_platform_event_loop.cpp Moved file descriptor registration to constructor
src/KDFoundation/timer.cpp Updated to use EventLoop::instance() instead of CoreApplication event loop
src/KDFoundation/platform/win32/win32_platform_timer.cpp Updated platform timer to use EventLoop::instance()
src/KDFoundation/platform/macos/macos_platform_timer.mm Updated platform timer to use EventLoop::instance()
src/KDFoundation/platform/abstract_platform_integration.h Added documentation warnings about initialization timing
src/KDFoundation/object.cpp Updated to use EventLoop for deferred deletion
src/KDFoundation/file_descriptor_notifier.cpp Updated to use current thread's EventLoop instead of CoreApplication
src/KDFoundation/event_receiver.cpp Enhanced to clear events from both thread-local and main event loops
src/KDFoundation/event_loop.h New EventLoop class header
src/KDFoundation/event_loop.cpp New EventLoop class implementation
src/KDFoundation/core_application.h Refactored to use EventLoop member instead of direct platform event loop
src/KDFoundation/core_application.cpp Simplified implementation by delegating event loop functionality
src/KDFoundation/CMakeLists.txt Added event_loop.cpp and event_loop.h to build

m_postman = std::make_unique<Postman>();
m_platformEventLoop->setPostman(m_postman.get());

assert(s_eventLoopInstance == nullptr && "Cannot have more than one event loop per thread. Nested event loops are not supported.");
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The assertion message mentions that nested event loops are not supported, but the actual check only prevents multiple event loops per thread. Consider clarifying the message to match the actual restriction being enforced.

Suggested change
assert(s_eventLoopInstance == nullptr && "Cannot have more than one event loop per thread. Nested event loops are not supported.");
assert(s_eventLoopInstance == nullptr && "Cannot have more than one event loop per thread.");

Copilot uses AI. Check for mistakes.
Copy link
Member

@MiKom MiKom left a comment

Choose a reason for hiding this comment

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

Looks good but we need to fix some things before it can land.

I also advise doing interactive rebase and squashing the pre-commit commits.

@MiKom MiKom force-pushed the wip/eventLoop branch 2 times, most recently from ebda6be to 22368e4 Compare September 4, 2025 12:47
akreuzkamp and others added 13 commits September 5, 2025 17:48
Refactores the eventloop functionality of CoreApplication into its own
EventLoop class. This EventLoop class can be used to execute event
loops on worker threads.

Renames CoreApplication::eventLoop() to
CoreApplication::platformEventLoop() and make
CoreApplication::eventLoop() return a pointer to the new EventLoop type.
* Create a GUI-specific platform event loop only when GuiApplication
  asks the platform integration to do so
* CoreApplication::quit sends a QuitEvent again, instead of calling
  EventLoop::quit directly
* Remove some stale comments
* Correct copyright headers
instead of requiring them to fail, since shouldFail doesn't work if the
unit test aborts.
Also moves the tests/auto/foundation/common dir to tests/auto/common,
since it's common to all unit tests, not only foundation unit tests.
Apparently, the ctuOneDefinitionRuleViolation check isn't triggered
anymore so we got a warning about unnecessary suppression.
Copy link
Member

@MiKom MiKom left a comment

Choose a reason for hiding this comment

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

Approved. I added some cppcheck fixes, removed one stale commented-out line in tests and changed one word in test description. I also squashed the pre-commit commits.

@MiKom MiKom merged commit 8b18f2b into main Sep 5, 2025
34 checks passed
@MiKom MiKom deleted the wip/eventLoop branch September 5, 2025 16:14
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