Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retry when shellnotifyicon fails #3710

Merged
merged 8 commits into from
Sep 2, 2024
Merged

retry when shellnotifyicon fails #3710

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
51fd0b2
add retry mechanism for create shell notification icon
DeltaLaboratory Aug 20, 2024
6cdfe2a
add retry mechanism for create shell notification icon
DeltaLaboratory Aug 20, 2024
50dd38c
Merge remote-tracking branch 'origin/fix/tray-icon-retry' into fix/tr…
DeltaLaboratory Aug 28, 2024
00a835e
adjust delay and attempts
DeltaLaboratory Aug 28, 2024
8108b05
added changelog
DeltaLaboratory Aug 28, 2024
c5a9e41
Merge branch 'v3-alpha' into fix/tray-icon-retry
leaanthony Sep 1, 2024
6ac9018
Merge branch 'v3-alpha' into fix/tray-icon-retry
DeltaLaboratory Sep 1, 2024
197f0e3
Use application fatal handler instead of panic
leaanthony Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions v3/pkg/application/systemtray_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ package application
import (
"fmt"
"syscall"
"time"
"unsafe"

"github.com/wailsapp/wails/v3/pkg/icons"

"github.com/samber/lo"

"github.com/wailsapp/wails/v3/pkg/events"
"github.com/wailsapp/wails/v3/pkg/w32"
)
Expand Down Expand Up @@ -174,8 +176,16 @@ func (s *windowsSystemTray) run() {
}
nid.CbSize = uint32(unsafe.Sizeof(nid))

if !w32.ShellNotifyIcon(w32.NIM_ADD, &nid) {
panic(syscall.GetLastError())
for retries := range 4 {
if !w32.ShellNotifyIcon(w32.NIM_ADD, &nid) {
if retries == 3 {
panic(syscall.GetLastError())
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeltaLaboratory - I think this is a valid comment from the rabbit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

@DeltaLaboratory DeltaLaboratory Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed an update.

}

time.Sleep(2 * time.Second)
continue
}
break
leaanthony marked this conversation as resolved.
Show resolved Hide resolved
}

nid.UVersion = w32.NOTIFYICON_VERSION
Expand Down
Loading