Display notify icon before loading plugins #3672
Conversation
|
🥷 Code experts: onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes refactor the management of the application's system tray notify icon by moving its instantiation and configuration from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant MainWindow
User->>App: Start application
App->>App: InitializeNotifyIcon()
App->>App: Set NotifyIcon properties and show icon
App->>MainWindow: Create MainWindow
MainWindow->>App: Use App.NotifyIcon for event handling and context menu
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
664-684: 🛠️ Refactor suggestionAdd null check before attaching event handlers.
The method assumes
App.NotifyIconexists, but it could be null if initialization failed in the App class.private void InitializeNotifyIcon() { + if (App.NotifyIcon == null) + { + App.API.LogWarn(nameof(MainWindow), "App.NotifyIcon is null, cannot attach event handlers"); + return; + } + App.NotifyIcon.MouseClick += (o, e) => { // ... existing event handler code }; }
🧹 Nitpick comments (1)
Flow.Launcher/App.xaml.cs (1)
159-167: Consider adding error handling for NotifyIcon creation.The initialization logic is correct, but consider adding try-catch around NotifyIcon creation to handle potential failures gracefully.
private void InitializeNotifyIcon() { + try + { NotifyIcon = new NotifyIcon { Text = Constant.FlowLauncherFullName, Icon = Constant.Version == "1.0.0" ? Launcher.Properties.Resources.dev : Launcher.Properties.Resources.app, Visible = !_settings.HideNotifyIcon }; + } + catch (Exception ex) + { + API.LogException(ClassName, "Failed to initialize notify icon", ex); + // NotifyIcon will remain null, which should be handled elsewhere + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/App.xaml.cs(4 hunks)Flow.Launcher/MainWindow.xaml.cs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Report (PR)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (5)
Flow.Launcher/App.xaml.cs (3)
27-27: LGTM! Good namespace alias practice.Using an alias for
System.Windows.Forms.NotifyIconprevents potential namespace conflicts and makes the code more readable.
36-36: Good centralization approach.The static property properly encapsulates the NotifyIcon instance and aligns with the PR objective of centralizing notify icon management.
199-200: Perfect implementation of the PR objective!Displaying the notify icon before plugin loading directly addresses the user need to see that Flow Launcher is starting up. The timing and comment are excellent.
Flow.Launcher/MainWindow.xaml.cs (2)
1316-1316:✅ Verification successful
Verify static NotifyIcon usage across the codebase.
The shift to static NotifyIcon management needs verification to ensure no other parts of the codebase are affected.
🏁 Script executed:
#!/bin/bash # Check for any remaining _notifyIcon references that might have been missed rg "_notifyIcon" --type cs # Check for other NotifyIcon instantiations that might conflict rg "new.*NotifyIcon" --type cs # Check for other MainWindow dispose patterns rg -A 10 -B 5 "\.Dispose\(\)" Flow.Launcher/MainWindow.xaml.cs # Verify App.Dispose method handles NotifyIcon disposal rg -A 15 -B 5 "protected virtual void Dispose" Flow.Launcher/App.xaml.csLength of output: 1987
Static NotifyIcon usage verified
– Searched for any private
_notifyIconreferences: none found.
– Onlynew NotifyIconinstantiation is inFlow.Launcher/App.xaml.cs.
– Disposal ofApp.NotifyIcononly occurs inMainWindow.Dispose(); there are no conflicting disposals inApp.Dispose().No further changes required.
50-50:✅ Verification successful
Verify implications of static context menu.
Changing
_contextMenuto static means all MainWindow instances share the same context menu. Ensure this doesn't cause issues with multiple windows or state persistence.
🏁 Script executed:
#!/bin/bash # Search for other MainWindow instantiations or context menu usage patterns rg -A 5 "new MainWindow|MainWindow.*=" rg -A 3 "_contextMenu\.|contextMenu\." --type csLength of output: 13487
🏁 Script executed:
#!/bin/bash # Display the top of MainWindow.xaml.cs to inspect contextMenu declaration and constructor sed -n '1,200p' Flow.Launcher/MainWindow.xaml.csLength of output: 6853
🏁 Script executed:
#!/bin/bash # Locate and display the InitializeContextMenu method implementation grep -R "InitializeContextMenu" -n Flow.Launcher/MainWindow.xaml.cs | head -n 5Length of output: 222
🏁 Script executed:
#!/bin/bash # Extract the InitializeContextMenu method implementation sed -n '690,800p' Flow.Launcher/MainWindow.xaml.csLength of output: 4804
No issues with the static ContextMenu usage
Only one MainWindow instance is ever created (in App.xaml.cs), and the static
_contextMenuis always re-initialized via InitializeContextMenu() on load or translation updates. All access occurs on the UI thread, so there are no threading or shared-state concerns. The change is safe as is.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR surfaces the tray icon earlier in the startup sequence so users immediately see that Flow is launching plugins.
- Move
NotifyIconcreation fromMainWindowtoAppinitialization - Update
MainWindowto referenceApp.NotifyIconfor icon updates and click handling - Shift disposal of the notify icon into
App.Dispose
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/MainWindow.xaml.cs | Remove instance NotifyIcon, reference App.NotifyIcon, make context menu static |
| Flow.Launcher/App.xaml.cs | Add static NotifyIcon property, initialize early in Main, dispose in App |
Comments suppressed due to low confidence (1)
Flow.Launcher/MainWindow.xaml.cs:50
- [nitpick] Making the window context menu static could introduce shared state if multiple windows are ever created; consider reverting to an instance field or documenting why it must be static.
private static readonly ContextMenu _contextMenu = new();
| NotifyIcon = new NotifyIcon | ||
| { | ||
| Text = Constant.FlowLauncherFullName, | ||
| Icon = Constant.Version == "1.0.0" ? Launcher.Properties.Resources.dev : Launcher.Properties.Resources.app, |
There was a problem hiding this comment.
Is there a way to not have this hardcoded? I am wary when moving to v2.0.0+ we need to bump this dev version too.
There was a problem hiding this comment.
I have not changed the original codes here. And I cannot think a better to do this. Dev branch version should be 1.0.0
There was a problem hiding this comment.
Yeah ok.
I think it's more consistent if we raise this from 1.0.0 to 2.0.0. we can do this in separate PR.
|
Can you provide a screenshot please |
This comment has been minimized.
This comment has been minimized.
? Just display a tray icon. And why we need a screenshot? |
|
I mean the notification icon |
Here you go.
|
|
Hmm I am not sure if this will communicate that flow is loading. @onesounds thoughts? |
No. The click event is not registered until main window is loaded. This tray icon just displays there and left click or right click will not trigger anything |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
I've been looking at this PR for a while and honestly couldn't figure out what it's doing — but after reading your conversation, I finally get it. (Please write more detailed explanations in PR descriptions. You need to provide enough background so people can understand why the change is necessary. As it stands, I can't even tell how to properly test this.) In my opinion, this approach isn't ideal. The way I understand this PR, Flow will not respond even if the tray icon is visible — but that could actually cause more user confusion. People might think "Flow is running, but clicking doesn't do anything." In fact, this might generate even more issue reports. (For example, when I debug, I usually interpret the tray icon as a definite signal that Flow is running and responsive. According to this PR, even if the tray icon is present, Flow may or may not be functional.) I am a bit curious to see how Jack would suffer from conflicting issue reports if this PR gets merged... but I think this problem should be addressed differently, and in the following priority order:
If we absolutely need to implement this behavior, one idea would be: Since the tray icon can be updated dynamically (like Discord does), we could display a different icon during the plugin loading phase. When users click or right-click the icon, we could show a context flyout that says: If Flow is triggered via a hotkey during this time, we could show a temporary OS notification. This is just a suggestion. Honestly, if the loading time isn’t too long, this method wouldn’t be a big issue either. |
Alright, let us close it |
3 participants
Display notify icon before loading plugins to tell users that Flow is loading.
If plugin initialization causes much time, it could be helpful for users to know Flow has started already