Added proper serialization of modal popups#13
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces multiple boolean-driven modals with a priority-aware Changes
Sequence DiagramsequenceDiagram
participant UI as Main Window (UI thread)
participant Session as StartupProbeSession
participant Hid as HidHide Worker
participant Lib as Libwdi Worker
participant Queue as SystemDialog Queue
participant ImGui as ImGui Renderer
UI->>Session: StartupProbeSession(hwnd)
activate Session
Session->>Hid: start thread
Session->>Lib: start thread
deactivate Session
par HidHide probe
Hid->>Hid: GetHidHideStatus()
Hid->>UI: PostMessage(WM_HIDHIDE_PROBE_READY)
and libwdi probe
Lib->>Lib: ProbeLibwdiUsbDevices()
Lib->>UI: PostMessage(WM_LIBWDI_PROBE_READY)
end
UI->>UI: WndProc receives WM_*_PROBE_READY
UI->>Queue: Enqueue corresponding SystemDialog
UI->>ImGui: OpenPopup(front) if queue not empty
ImGui->>ImGui: BeginCenteredModal(SystemDialogPopupId(front))
ImGui->>ImGui: Render dialog content (switch on front)
ImGui->>Queue: On dismiss -> Pop front
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.cpp`:
- Around line 430-445: The current handling of WM_SYSCOMMAND sets separate flags
(g_pendingAbout, g_showAbout, g_pendingPreferences, g_showPreferences) which
allows out-of-order/reentrant modal presentation; instead consolidate modal
requests into a single serialized queue/state machine (e.g., push a ModalRequest
{type: ABOUT|PREFERENCES} into g_systemDialogQueue) and replace the separate
pending/show booleans with a single “current modal active” check before
dequeuing or setting any show flag; update the replay/renderer paths to always
pop from g_systemDialogQueue only when no modal is active (or consult the single
current-modal state) so About and Preferences respect request order and cannot
be displayed on consecutive frames while one is open (also apply same change
where similar modal handling exists around lines 743-910).
- Around line 265-276: EnqueueSystemDialog currently runs std::lower_bound
across the entire g_systemDialogQueue which allows a later high-priority dialog
to be inserted before the active front; change the insertion range so the
existing front remains fixed: if g_systemDialogQueue is non-empty, run the
lower_bound and insert only into the tail range (begin()+1 to end()) so you
preserve the current front, otherwise insert into the empty queue as before;
update EnqueueSystemDialog (referencing SystemDialogQueueContains,
SystemDialogPriority and g_systemDialogQueue) to search/insert starting from the
second element (or alternatively track the active dialog separately and only
priority-sort pending entries).
In `@src/startup_probe.cpp`:
- Around line 28-94: The shutdown hang is caused by std::jthread joins running
on the UI thread when g_startupProbeSession.reset() is called; instead, avoid
joining on the UI thread by performing the jthread teardown off-thread:
implement a ShutdownAsync helper that captures/moves g_startupProbeSession into
a background std::thread (or threadpool task) and calls reset() there, then make
the WM_DESTROY path call ShutdownAsync rather than resetting directly; keep
RunHidHideWorker and RunLibwdiWorker unchanged except ensure they respect
stop_token (they already do) and reference g_startupProbeSession, std::jthread,
RunHidHideWorker, RunLibwdiWorker, and the WM_DESTROY shutdown call when
locating where to change the reset to an asynchronous teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cbe859c-7910-423a-876c-b62ffa6e4e70
📒 Files selected for processing (5)
src/main.cppsrc/modal_helpers.cppsrc/modal_helpers.hsrc/startup_probe.cppsrc/startup_probe.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/startup_probe.cpp`:
- Around line 102-115: The catch path can block trying to join partially-started
std::jthread workers; to fix, publish hwnd_.store(nullptr,
std::memory_order_release) before touching threads so
RunHidHideWorker/RunLibwdiWorker see the invalidated handle and exit quickly,
then reset hidThread_ and libwdiThread_; ensure the code sets hwnd_ to nullptr
as the very first action in the catch block (reference symbols: hidThread_,
libwdiThread_, hwnd_, RunHidHideWorker, RunLibwdiWorker).
- Around line 147-156: In StartupProbeSession_ShutdownAsync, invalidate the
notify handle and request stop synchronously before handing the unique_ptr off
to a detached thread: call the session's stop/request method and set its hwnd_
(or notify handle) to nullptr immediately, then attempt to construct the
std::thread that takes the moved session only for the join/cleanup work; wrap
thread construction in a try/catch (catch std::system_error) and if thread
construction fails, synchronously reset the moved session to ensure teardown
happens immediately. Ensure all references to hwnd_ and the stop request happen
on the session object before moving it into the thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e3ff9dc-3b52-4e5f-9cff-cd4bde6cd8af
📒 Files selected for processing (3)
src/main.cppsrc/startup_probe.cppsrc/startup_probe.h
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Refactor
New Features