Repository navigation
fix(v3): Dock ops sync and add GetBadge method - #4838
Conversation
WalkthroughAdds a public accessor Changes
Sequence Diagram(s)(omitted — change introduces a getter and platform-local state tracking; not a multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v3/pkg/services/dock/badge_ios.go (1)
70-74: Consider returning d.Badge for consistency.Once SetBadge/RemoveBadge maintain the Badge field, GetBadge should return
d.Badgeinstead ofnilto reflect the stored state, matching the pattern used in macOS and Windows implementations.🔎 Suggested change
func (d *iosDock) GetBadge() *string { // iOS badge retrieval would go here via native bridge - return nil + return d.Badge }v3/pkg/services/dock/dock_darwin.go (1)
86-95: Note: GetBadge returns "●" for empty labels.SetBadge now converts empty labels to "●" (line 88) before storing in
d.Badge. This means GetBadge() will return a pointer to "●" rather than "" when an empty label was set. While this matches the documented behavior, callers expecting to retrieve the original empty string will see "●" instead.Consider whether storing the original label separately from the display label would better serve GetBadge consumers who may want to distinguish between an explicit "●" and an empty string.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/src/content/docs/features/platform/dock.mdxv3/UNRELEASED_CHANGELOG.mdv3/pkg/services/dock/badge_ios.gov3/pkg/services/dock/dock.gov3/pkg/services/dock/dock_darwin.gov3/pkg/services/dock/dock_linux.gov3/pkg/services/dock/dock_windows.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/pkg/services/dock/dock_darwin.gov3/pkg/services/dock/badge_ios.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/pkg/services/dock/dock_darwin.gov3/pkg/services/dock/badge_ios.go
🧬 Code graph analysis (3)
v3/pkg/services/dock/dock.go (3)
v3/examples/badge-custom/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/dock/index.ts (1)
DockService(6-6)v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/dock/index.ts (1)
DockService(6-6)v3/examples/dock/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/dock/index.ts (1)
DockService(6-6)
v3/pkg/services/dock/dock_darwin.go (4)
v3/pkg/services/dock/badge_ios.go (1)
New(18-24)v3/pkg/services/dock/dock_linux.go (1)
New(16-20)v3/pkg/services/dock/dock_windows.go (1)
New(39-45)v3/pkg/services/dock/dock.go (1)
DockService(27-29)
v3/pkg/services/dock/badge_ios.go (5)
v3/pkg/services/dock/dock_darwin.go (1)
New(52-58)v3/pkg/services/dock/dock_linux.go (1)
New(16-20)v3/pkg/services/dock/dock_windows.go (1)
New(39-45)v3/pkg/services/notifications/notifications_ios.go (1)
New(16-26)v3/pkg/services/dock/dock.go (1)
DockService(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
🔇 Additional comments (5)
v3/pkg/services/dock/dock_linux.go (1)
72-75: LGTM!The no-op stub implementation is consistent with other Linux dock methods and appropriately documented.
v3/pkg/services/dock/dock.go (2)
23-23: LGTM!The GetBadge method addition to the platformDock interface is clean and consistent with the existing interface design.
80-83: LGTM!The GetBadge implementation follows the established delegation pattern used by other DockService methods.
v3/pkg/services/dock/dock_windows.go (1)
27-27: LGTM!The badge state management is correctly implemented:
- Field properly declared and initialized
- State maintained in SetBadge, SetCustomBadge, and RemoveBadge
- GetBadge provides clean accessor
- All operations synchronized via
application.InvokeSyncWithErrorAlso applies to: 52-52, 126-126, 188-188, 216-216, 403-405
v3/pkg/services/dock/dock_darwin.go (1)
22-28: Verify activation policy guard behavior with Badge field updates.The C code now guards badge setting by checking the activation policy (line 24-28), returning early if not Regular. However, the Go code still updates
d.Badgeat line 94 regardless of whether the C code actually sets the badge.This means GetBadge() might return a badge value that's not currently displayed on the dock. This could be intentional (to remember the badge for restoration when the policy changes back to Regular), but please verify this is the desired behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/services/dock/dock_darwin.go (1)
22-37: State inconsistency: policy guard silently fails but Go state is still updated.The C
setBadgefunction now includes a policy guard (lines 24-28) that silently returns when the dock icon is hidden. However, the GoSetBadgemethod (lines 95-98) unconditionally updatesd.Badgeregardless of whether the C function succeeded or returned early. This meansGetBadge()can return a badge value that is not actually displayed on the dock.Consider one of these approaches:
- Return an error from C and check it in Go before updating
d.Badge- Move the policy check to the Go layer before calling C
- Document that
GetBadge()returns the "intended" badge, not necessarily the displayed one🔎 Proposed fix: Move policy check to Go layer
This approach keeps state consistent by preventing the C call entirely when the policy check would fail:
// SetBadge sets the badge label on the application icon. func (d *darwinDock) SetBadge(label string) error { // Always pick a label (use "●" if empty), then allocate + free exactly once. if label == "" { label = "●" // Default badge character } + + // Note: We cannot check activation policy from Go without CGO overhead, + // so we rely on the C function's policy guard. If the dock is hidden, + // we still update d.Badge to represent the "intended" badge state. + // Users must call SetBadge again after ShowAppIcon() to apply the badge. + cLabel := C.CString(label) defer C.free(unsafe.Pointer(cLabel)) C.setBadge(cLabel) d.mu.Lock() d.Badge = &label d.mu.Unlock() return nil }Alternatively, document the behavior in the method comment to clarify that
d.Badgestores the intended badge state.
🧹 Nitpick comments (1)
v3/pkg/services/dock/dock_darwin.go (1)
76-84: Consider documenting badge reapplication workflow.The PR description mentions that workflows need to restore a badge after the dock tile is hidden and shown (e.g., when switching activation policies). Since
ShowAppIcon()doesn't automatically reapply any stored badge, consider adding a comment to guide users:// ShowAppIcon shows the app icon in the macOS Dock. +// Note: After showing the dock icon, you may need to call SetBadge again +// to reapply any previously set badge, as changing activation policies clears the badge. func (d *darwinDock) ShowAppIcon() { C.showDockIcon() }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/services/dock/dock_darwin.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
🧬 Code graph analysis (1)
v3/pkg/services/dock/dock_darwin.go (4)
v3/pkg/services/dock/badge_ios.go (1)
New(18-24)v3/pkg/services/dock/dock_linux.go (1)
New(16-20)v3/pkg/services/dock/dock_windows.go (1)
New(39-45)v3/pkg/services/dock/dock.go (1)
DockService(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (5)
v3/pkg/services/dock/dock_darwin.go (5)
48-51: LGTM! Thread safety properly implemented.The addition of
sync.RWMutexand theBadgefield addresses the data race issue identified in the previous review. Using a pointer to string (*string) correctly represents the optional badge state.
54-60: LGTM! Initialization is correct.The initialization properly sets
Badge: nilto represent no badge initially, consistent with the other platform implementations.
86-100: API behavior change: empty string now shows default badge.The refactored logic now treats an empty string as a request to show the default badge "●" (lines 89-91), whereas previously an empty string might have cleared the badge. This is a subtle change in API behavior.
Please confirm this is the intended behavior. If so, consider documenting this in the method comment:
-// SetBadge sets the badge label on the application icon. +// SetBadge sets the badge label on the application icon. +// If label is empty, displays a default badge ("●"). Use RemoveBadge() to clear the badge. func (d *darwinDock) SetBadge(label string) error {
108-114: LGTM! RemoveBadge correctly clears both C and Go state.The implementation properly clears the badge at both the C level and updates the Go state with appropriate mutex protection.
116-121: LGTM! GetBadge implementation is thread-safe.The read lock usage is correct, allowing concurrent reads while preventing race conditions with SetBadge/RemoveBadge writes.
Note: Due to the policy guard behavior discussed earlier, this method may return a badge value that is not currently displayed if the dock icon was hidden when
SetBadgewas last called.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/pkg/services/dock/badge_ios.gov3/pkg/services/dock/dock_darwin.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
🧬 Code graph analysis (1)
v3/pkg/services/dock/dock_darwin.go (5)
v3/pkg/services/dock/badge_ios.go (1)
New(17-21)v3/pkg/services/dock/dock_linux.go (1)
New(16-20)v3/pkg/services/dock/dock_windows.go (1)
New(39-45)v3/pkg/services/dock/dock.go (1)
DockService(27-29)v3/examples/dock/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/dock/dockservice.ts (2)
ShowAppIcon(51-53)SetBadge(36-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
🔇 Additional comments (9)
v3/pkg/services/dock/badge_ios.go (1)
67-71: LGTM! GetBadge stub implementation is consistent with iOS platform pattern.The stub returning
nilappropriately indicates that badge state is not tracked on the Go side for iOS, pending native bridge implementation.v3/pkg/services/dock/dock_darwin.go (8)
48-51: LGTM! Thread-safe state management properly implemented.The
sync.RWMutexandBadge *stringfield correctly address the data race concern from previous reviews. Using a pointer allowsnilto represent the absence of a badge, andRWMutexis appropriate for potentially read-heavy access patterns.
54-66: LGTM! Constructor initialization is clear and consistent.Explicit
Badge: nilinitialization makes the initial state obvious.NewWithOptionsappropriately documents that badge options are not supported on macOS, consistent with the platform limitations noted inSetCustomBadge.
68-74: Lifecycle methods appropriately implement the platform interface.No initialization or cleanup is required for macOS dock operations, so returning
nilis correct.
82-86: Helpful documentation about badge persistence behavior.The comment correctly explains that changing activation policies clears the badge, documenting the manual restore workflow that motivated adding
GetBadge()(save badge → hide → show → restore badge).
88-101: LGTM! Helper method properly addresses maintainer feedback.This private
setBadgemethod successfully extracts the common pattern for badge updates (C string conversion, locking, state update) as requested in previous review comments. Memory management withdefer C.freeis correct.
113-116: LGTM! Appropriate fallback for platform limitations.Delegating to
SetBadgeand ignoring theoptionsparameter is the correct approach since macOS doesn't support custom badge styling. The comment clearly documents this limitation.
118-122: LGTM! Clean implementation leveraging the helper method.Passing
niltosetBadgeappropriately represents badge removal, and the delegation simplifies the code.
124-129: LGTM! Thread-safe read access properly implemented.Using
RLock/RUnlockallows concurrent reads while maintaining safety, and returning the pointer directly enables the save/restore workflow described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/services/dock/dock_darwin.go (1)
117-123: LGTM! SetBadge correctly implements documented default behavior.The empty string defaulting to "●" is clearly documented in the godoc comment above. The implementation correctly normalizes the input and delegates to the
setBadgehelper.Optional: The inline comment on line 118 mentions "allocate + free exactly once," but that detail is handled inside
setBadge. Consider simplifying to just// Use default badge "●" if emptyfor clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/services/dock/dock_darwin.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-24T06:08:55.645Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/pkg/services/dock/dock_darwin.go
🧬 Code graph analysis (1)
v3/pkg/services/dock/dock_darwin.go (4)
v3/pkg/services/dock/badge_ios.go (1)
New(17-21)v3/pkg/services/dock/dock_linux.go (1)
New(16-20)v3/pkg/services/dock/dock_windows.go (1)
New(39-45)v3/pkg/services/dock/dock.go (1)
DockService(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (8)
v3/pkg/services/dock/dock_darwin.go (8)
11-11: LGTM! Synchronous dispatch correctly addresses consistency issues.The change from
dispatch_asynctodispatch_syncensures operations complete before returning to the Go caller, which aligns with the PR's goal of addressing intermittent consistency issues. The activation policy check insetBadgecorrectly prevents badge operations when the dock icon is hidden, and the__block boolpattern properly returns success/failure status.Also applies to: 17-17, 22-41
46-47: LGTM! Imports are necessary for synchronization and error handling.
53-56: LGTM! Mutex correctly protects badge state from concurrent access.The
sync.RWMutexandBadgefield provide thread-safe state management, which is essential given that EventProcessor can dispatch event handlers concurrently.
59-65: LGTM! Constructor properly initializes the dock service.Note:
Badge: nilis technically redundant sincenilis the zero value for*string, but the explicit initialization improves clarity.
87-88: LGTM! Helpful documentation about badge reapplication.This comment clearly explains the need to reapply badges after activation policy changes, which directly supports the workflow described in the PR objectives.
93-111: LGTM! Well-designed helper that centralizes badge logic.The
setBadgehelper correctly:
- Manages C string memory with
defer- Only updates internal
Badgestate when the C call succeeds- Protects state mutation with the mutex
- Provides a single point of control for badge operations
This addresses the maintainer's suggestion to extract a private method for the badge-setting pattern.
131-133: LGTM! RemoveBadge cleanly delegates to the shared helper.Reusing
setBadge(nil)eliminates code duplication and ensures consistent error handling and locking across all badge operations.
135-140: LGTM! GetBadge provides thread-safe access to badge state.The read lock correctly allows concurrent reads while preventing race conditions with writes. Returning
*stringproperly represents the optional nature of the badge value (nil when no badge is set).This addition supports the badge restoration workflow described in the PR objectives.
|
|
Amazing stuff! 🙏 |
* dock fixes and get method * update changelog * async -> sync * cleanup iOS and darwin set call * handle potential errors
2 participants
Description
I found that I was seeing consistency issues dispatching dock tile calls async. Moved to sync calls and the intermittent issues were resolved. In
SetBadgeI included a check to ensure the dock tile is visible.While working on this I found it is helpful to get back the currently set label. For example after restoring the dock tile you would want to re-add the previously set label. When you set the activation policy to hide the dock tile when it gets restored the dock tile is missing. You need to call
RemoveBadgebefore/after switching policies to then set the badge again.Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.