#511: the UI update should in the main loop#724
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR ensures that UI list updates for pals are dispatched via the GLib main loop instead of running directly in the core thread, adding a small context struct and a dedicated UI update method while removing an unused header include. Sequence diagram for pal list UI update via GLib main loopsequenceDiagram
participant CoreThread
participant UiCoreThread
participant GLibMainContext
CoreThread->>UiCoreThread: UpdatePalToList(palKey)
UiCoreThread->>CoreThread: CoreThread::UpdatePalToList(palKey)
UiCoreThread->>GLibMainContext: g_main_context_invoke(NULL, iptux_uithread_update_pal_to_list, ctx)
GLibMainContext->>UiCoreThread: iptux_uithread_update_pal_to_list(ctx)
UiCoreThread->>UiCoreThread: UpdatePalToListInUI(palKey)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider avoiding the heap allocation and manual delete for UiCoreThreadCtx by either capturing
this/palKey in a lambda passed tog_main_context_invokeor using a smart pointer to manage the context lifetime more safely. - The C-style cast in
iptux_uithread_update_pal_to_list(auto* ctx = (UiCoreThreadCtx*)data;) could be replaced with astatic_castto improve type safety and readability. - It may be clearer to mark
UpdatePalToListInUIas a private helper and add a brief comment indicating it must only be called from the main/UI thread, sinceUpdatePalToListnow ensures thread-safe invocation viag_main_context_invoke.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the heap allocation and manual delete for UiCoreThreadCtx by either capturing `this`/palKey in a lambda passed to `g_main_context_invoke` or using a smart pointer to manage the context lifetime more safely.
- The C-style cast in `iptux_uithread_update_pal_to_list` (`auto* ctx = (UiCoreThreadCtx*)data;`) could be replaced with a `static_cast` to improve type safety and readability.
- It may be clearer to mark `UpdatePalToListInUI` as a private helper and add a brief comment indicating it must only be called from the main/UI thread, since `UpdatePalToList` now ensures thread-safe invocation via `g_main_context_invoke`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #724 +/- ##
==========================================
+ Coverage 51.75% 51.99% +0.23%
==========================================
Files 64 64
Lines 8704 8714 +10
==========================================
+ Hits 4505 4531 +26
+ Misses 4199 4183 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a GTK UI thread-safety issue by dispatching pal-list UI refresh work onto the GLib main context rather than performing UI updates directly from the core/network thread.
Changes:
- Route
UiCoreThread::UpdatePalToList(...)updates throughg_main_context_invoke()so UI refresh happens on the main loop. - Introduce a small invoke-context struct and UI-thread callback trampoline.
- Add a dedicated
UpdatePalToListInUI(...)helper to separate core updates from UI updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/iptux/UiCoreThread.h | Adds a UI-thread helper entrypoint for pal-list updates. |
| src/iptux/UiCoreThread.cpp | Schedules pal-list UI updates onto the GLib main loop via g_main_context_invoke(). |
| void ClearAllPalFromList() override; | ||
| void UpdatePalToList(PalKey palKey) override; | ||
| void UpdatePalToListInUI(PalKey palKey); | ||
| void UpdatePalToList(in_addr ipv4) override { | ||
| UpdatePalToList(PalKey(ipv4, port())); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
❌ The last analysis has failed. |
/close #511
Summary by Sourcery
Ensure updating pal entries in the UI is dispatched to the main thread for thread-safe UI refreshes.
Bug Fixes:
Enhancements: