Add StatusIcon for Windows - #735
Merged
Merged
Conversation
Reviewer's GuideAdds a Windows-specific system tray app indicator implementation, wires it into the build for Windows (including tests), and introduces a small configuration and test harness tweak to support the new functionality. Sequence diagram for Windows tray interactions with the new app indicatorsequenceDiagram
actor User
participant WindowsShell
participant IptuxAppIndicatorPrivate
participant IptuxAppIndicator
participant GActionGroup
User->>WindowsShell: Click tray icon
WindowsShell->>IptuxAppIndicatorPrivate: WndProc(kTrayCallbackMessage)
IptuxAppIndicatorPrivate->>IptuxAppIndicator: sigActivateMainWindow.emit()
User->>WindowsShell: Right-click tray icon
WindowsShell->>IptuxAppIndicatorPrivate: WndProc(kTrayCallbackMessage)
IptuxAppIndicatorPrivate->>IptuxAppIndicatorPrivate: ShowMenu()
IptuxAppIndicatorPrivate->>WindowsShell: TrackPopupMenu(menu)
User->>WindowsShell: Select menu item
WindowsShell->>IptuxAppIndicatorPrivate: WndProc(WM_COMMAND)
IptuxAppIndicatorPrivate->>GActionGroup: ActivateAction("open_main_window"/"preferences"/"quit")
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- The Windows-specific logic in
src/iptux/meson.buildis now split across two separateif host_machine.system() == 'windows'blocks; consider consolidating these into a single block to keep platform-specific configuration easier to follow and maintain. - In
IptuxAppIndicatorPrivate's destructor you unconditionally callUnregisterClassW(kTrayWindowClass, GetModuleHandleW(nullptr)); if multiple instances are ever created this will tear down the window class globally—consider registering/unregistering the class once (e.g., via a static guard) or only unregistering if this instance actually registered it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Windows-specific logic in `src/iptux/meson.build` is now split across two separate `if host_machine.system() == 'windows'` blocks; consider consolidating these into a single block to keep platform-specific configuration easier to follow and maintain.
- In `IptuxAppIndicatorPrivate`'s destructor you unconditionally call `UnregisterClassW(kTrayWindowClass, GetModuleHandleW(nullptr))`; if multiple instances are ever created this will tear down the window class globally—consider registering/unregistering the class once (e.g., via a static guard) or only unregistering if this instance actually registered it.
## Individual Comments
### Comment 1
<location path="src/meson.build" line_range="61-62" />
<code_context>
test_conf_data = configuration_data()
-test_conf_data.set_quoted('CURRENT_SOURCE_PATH', meson.current_source_dir())
+test_conf_data.set(
+ 'CURRENT_SOURCE_PATH',
+ 'R"(' + meson.current_source_dir() + ')"',
+)
</code_context>
<issue_to_address>
**issue:** Raw string construction for CURRENT_SOURCE_PATH can break if the path ever contains the sequence ")".
This builds `CURRENT_SOURCE_PATH` as `"R"(" + meson.current_source_dir() + ")""`, which relies on the path never containing the raw-string terminator `" )"`. If that sequence appears in the directory name, the generated C++ literal is ill-formed and compilation will fail. Use a custom raw-string delimiter (e.g. `R"iptux(... )iptux"`) or another safe encoding instead of hard-coding `R"(` and `)"` around the path.
</issue_to_address>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 #735 +/- ##
==========================================
+ Coverage 52.37% 52.53% +0.15%
==========================================
Files 70 71 +1
Lines 8994 9004 +10
==========================================
+ Hits 4711 4730 +19
+ Misses 4283 4274 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces a confirmed conversion error path in Utf8ToWide() and the implemented “centralized” status-icon logic currently only applies to Windows, diverging from the PR’s stated scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds Windows system-tray (status icon) support and introduces a small utility for consistent status-icon state resolution, along with build/config updates to enable this on Windows.
Changes:
- Add a Windows-specific
IptuxAppIndicatorimplementation usingShell_NotifyIconWand a Win32 popup menu. - Introduce
AppIndicatorStatehelpers (ShouldBlink,ResolveIconState) with a new unit test. - Update Meson/config to expose
SYSTEM_WINDOWS, enableHAVE_STATUS_ICONon Windows, linkshell32, and adjustCURRENT_SOURCE_PATHquoting for tests.
File summaries
| File | Description |
|---|---|
| src/meson.build | Adds SYSTEM_WINDOWS define and adjusts CURRENT_SOURCE_PATH configuration for tests. |
| src/iptux/meson.build | Links shell32 on Windows, compiles AppIndicatorWindows.cpp, and adds AppIndicatorStateTest.cpp. |
| src/iptux/AppIndicatorWindows.cpp | New Win32 tray icon implementation (menu, icon loading, blink timer). |
| src/iptux/AppIndicatorStateTest.cpp | Unit tests for blink/state resolution logic. |
| src/iptux/AppIndicatorState.h | New helper API for determining icon state and blink behavior. |
| src/config.h.in | Adds SYSTEM_WINDOWS and enables HAVE_STATUS_ICON when building for Windows. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+38
to
+43
| std::wstring result(static_cast<size_t>(len), L'\0'); | ||
| MultiByteToWideChar(CP_UTF8, 0, text, -1, result.data(), len); | ||
| if (!result.empty() && result.back() == L'\0') { | ||
| result.pop_back(); | ||
| } | ||
| return result; |
Comment on lines
+14
to
+20
| inline bool ShouldBlink(StatusIconMode mode, int unread_count) { | ||
| return mode == STATUS_ICON_MODE_BLINKING && unread_count > 0; | ||
| } | ||
|
|
||
| inline AppIndicatorIconState ResolveIconState(StatusIconMode mode, | ||
| int unread_count, | ||
| bool blink_state) { |
|
❌ The last analysis has failed. |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
There are confirmed build/runtime correctness issues (case-sensitive source filename mismatch in Meson and an invalid Win32 ReleaseDC call path) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/iptux/AppIndicatorWindows.cpp:81
- ReleaseDC is called unconditionally, but GetDC can return NULL; calling ReleaseDC with a NULL HDC is invalid. Guard the ReleaseDC call (or avoid acquiring an HDC at all) to prevent undefined behavior on failure paths.
ReleaseDC(nullptr, hdc);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| dependencies += [ws2_32] | ||
| shell32 = cc.find_library('shell32') | ||
| dependencies += [ws2_32, shell32] | ||
| sources += ['UiHelperWindows.cpp'] |
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
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 Sourcery
Implement Windows system tray app indicator support and centralize status icon state logic with accompanying tests and configuration updates.
New Features:
Enhancements:
Tests: