retry when shellnotifyicon fails #3710
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
Early access features: disabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| for retries := range 4 { | ||
| if !w32.ShellNotifyIcon(w32.NIM_ADD, &nid) { | ||
| if retries == 3 { | ||
| panic(syscall.GetLastError()) |
There was a problem hiding this comment.
Consider alternative error handling instead of panic.
Using panic for error handling may not be ideal in production code. Consider returning an error or using a more graceful error handling mechanism.
Suggested changes:
Modify the function signature to return an error and handle the error appropriately:
-func (s *windowsSystemTray) run() {
+func (s *windowsSystemTray) run() error {
// existing code...
for retries := range 4 {
if !w32.ShellNotifyIcon(w32.NIM_ADD, &nid) {
if retries == 3 {
- panic(syscall.GetLastError())
+ return fmt.Errorf("ShellNotifyIcon failed after %d retries: %w", retries+1, syscall.GetLastError())
}
time.Sleep(2 * time.Second)
continue
}
break
}
// existing code...
}
- s.run()
+ if err := s.run(); err != nil {
+ log.Fatalf("Failed to run system tray: %v", err)
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@DeltaLaboratory - I think this is a valid comment from the rabbit.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@leaanthony I think this is a good idea, but in the current codebase how can we modify this to allow user to handle error?
maybe like:
func (s *SystemTray) Run() error {
s.impl = newSystemTrayImpl(s)
if s.attachedWindow.Window != nil {
// Setup listener
s.attachedWindow.Window.On(events.Common.WindowLostFocus, func(event *WindowEvent) {
s.attachedWindow.Window.Hide()
// Special handler for Windows
if runtime.GOOS == "windows" {
// We don't do this unless the window has already been shown
if s.attachedWindow.hasBeenShown == false {
return
}
s.attachedWindow.justClosed = true
go func() {
time.Sleep(s.attachedWindow.Debounce)
s.attachedWindow.justClosed = false
}()
}
})
}
return InvokeSyncWithError(s.impl.run)
}
|
Thanks for taking the time to open this. It would be better to use smaller sleeps and a higher number of attempts. I'm concerned this is a blocking operation so it would be better to minimise the delay here. |
DeltaLaboratory
force-pushed
the
fix/tray-icon-retry
branch
from
51fd0b2 to
6cdfe2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/systemtray_windows.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/systemtray_windows.go
|
I checked out some Windows startup times and noticed there's usually about a 3-second gap between when programs start loading and when the desktop is ready to go. So, I adjusted the settings to wait 500 milliseconds and try 6 times. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/systemtray_windows.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/systemtray_windows.go
|
Thanks 🙏 Please could you add an entry to the changelog located at |
|
added changelog |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- mkdocs-website/docs/en/changelog.md (1 hunks)
Additional comments not posted (1)
mkdocs-website/docs/en/changelog.md (1)
26-26: Changelog Entry ReviewThe new changelog entry for the fix related to the system tray startup issue on Windows is correctly formatted and placed under the
Fixedsection for the current unreleased version. The entry clearly links to the issue and credits the contributor, which is in line with the project's documentation standards.The changelog entry is approved as it meets the project standards and provides the necessary information.
|
hello, is there anything blocking this merge? |
|
Just time :-) I think it would be better to use |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- mkdocs-website/docs/en/changelog.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- mkdocs-website/docs/en/changelog.md
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/systemtray_windows.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/systemtray_windows.go
* add retry mechanism for create shell notification icon * add retry mechanism for create shell notification icon * adjust delay and attempts * added changelog * Use application fatal handler instead of panic --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
* add retry mechanism for create shell notification icon * add retry mechanism for create shell notification icon * adjust delay and attempts * added changelog * Use application fatal handler instead of panic --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Description
Fixes #3693
I choose 4 for retry and 2 second for delay, no real reason
Type of change
How Has This Been Tested?
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes