Fix possible content frame navigation issue in welcome window & setting window #3088
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes primarily involve enhancements to the user interface and navigation logic of the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
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: 1
🧹 Outside diff range and nitpick comments (5)
Flow.Launcher/WelcomeWindow.xaml.cs (1)
Line range hint
1-115: Consider implementing a navigation service patternThe current navigation implementation is tightly coupled and could benefit from:
- Extracting navigation logic into a dedicated service
- Using dependency injection for Settings
- Following MVVM pattern for better separation of concerns
This would improve testability, maintainability, and make the navigation logic more reusable across windows.
Example navigation service interface:
public interface INavigationService { void NavigateToPage(int pageNumber, Settings settings); bool CanNavigateBack { get; } bool CanNavigateForward { get; } void NavigateBack(); void NavigateForward(); }Flow.Launcher/SettingWindow.xaml.cs (2)
192-202: Consider cleaning up the Loaded event handlerWhile the implementation correctly handles both immediate and delayed loading scenarios, consider unsubscribing from the
Loadedevent after it's handled to prevent potential memory leaks.Here's a suggested implementation:
private void NavView_Loaded(object sender, RoutedEventArgs e) { if (ContentFrame.IsLoaded) { ContentFrame_Loaded(sender, e); } else { - ContentFrame.Loaded += ContentFrame_Loaded; + void Handler(object s, RoutedEventArgs args) + { + ContentFrame_Loaded(s, args); + ContentFrame.Loaded -= Handler; + } + ContentFrame.Loaded += Handler; } }
204-207: Add bounds check for MenuItems collectionWhile the null-coalescing assignment is elegant, consider adding a bounds check to ensure
MenuItems[0]exists before accessing it.Here's a suggested implementation:
private void ContentFrame_Loaded(object sender, RoutedEventArgs e) { - NavView.SelectedItem ??= NavView.MenuItems[0]; /* Set First Page */ + NavView.SelectedItem ??= NavView.MenuItems.Count > 0 + ? NavView.MenuItems[0] + : null; /* Set First Page */ }Flow.Launcher/WelcomeWindow.xaml (1)
Line range hint
1-164: Good architectural approach to fixing the navigation issue.The solution properly addresses the race condition between UI initialization and navigation by:
- Using the
Loadedevent to ensure proper timing- Maintaining a clean separation between UI layout and navigation logic
- Keeping the window's fixed dimensions which helps prevent layout issues
This is a robust fix that should prevent similar timing issues in the future.
Flow.Launcher/SettingWindow.xaml (1)
Line range hint
65-97: Manage the visibility ofMaximizeButtonandRestoreButtonBoth
MaximizeButtonandRestoreButtonare placed in the same grid cell (Grid.Row="0",Grid.Column="3"). Without controlling theirVisibility, these buttons may overlap, causing UI issues. Implement logic to toggle theVisibilityof these buttons based on the window'sWindowState.Apply this diff to manage the visibility:
<!-- Add style triggers to toggle visibility based on window state --> <Button Name="MaximizeButton" Grid.Row="0" Grid.Column="3" Click="OnMaximizeRestoreButtonClick" Style="{StaticResource TitleBarButtonStyle}"> <!-- Existing Path content --> + <Button.Style> + <Style TargetType="Button"> + <Setter Property="Visibility" Value="Visible" /> + <Style.Triggers> + <DataTrigger Binding="{Binding WindowState, RelativeSource={RelativeSource AncestorType=Window}}" Value="Maximized"> + <Setter Property="Visibility" Value="Collapsed" /> + </DataTrigger> + </Style.Triggers> + </Style> + </Button.Style> </Button> <Button Name="RestoreButton" Grid.Row="0" Grid.Column="3" Click="OnMaximizeRestoreButtonClick" Style="{StaticResource TitleBarButtonStyle}"> <!-- Existing Path content --> + <Button.Style> + <Style TargetType="Button"> + <Setter Property="Visibility" Value="Collapsed" /> + <Style.Triggers> + <DataTrigger Binding="{Binding WindowState, RelativeSource={RelativeSource AncestorType=Window}}" Value="Maximized"> + <Setter Property="Visibility" Value="Visible" /> + </DataTrigger> + </Style.Triggers> + </Style> + </Button.Style> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Flow.Launcher/SettingWindow.xaml(1 hunks)Flow.Launcher/SettingWindow.xaml.cs(2 hunks)Flow.Launcher/WelcomeWindow.xaml(5 hunks)Flow.Launcher/WelcomeWindow.xaml.cs(1 hunks)
🔇 Additional comments (6)
Flow.Launcher/WelcomeWindow.xaml.cs (1)
104-109: Verify OnActivated event subscription
The method implementation looks correct, but ensure it's properly subscribed to the window's Activated event, typically in XAML or constructor.
✅ Verification successful
OnActivated event is properly wired up
The OnActivated method is correctly subscribed to the window's Activated event in the XAML file through the attribute Activated="OnActivated" on the Window element.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Activated event subscription in XAML
rg -l "Activated=\"OnActivated\"" --type xaml
# Check for programmatic subscription
ast-grep --pattern 'this.Activated += $handler;'
Length of output: 125
Script:
#!/bin/bash
# Check for Activated event subscription in XAML with different file extensions
rg -l "Activated=\"OnActivated\"" -g "*.xaml"
# Check for programmatic subscription with different patterns
ast-grep --pattern 'Activated += OnActivated'
# Find the WelcomeWindow.xaml file to inspect its content
fd "WelcomeWindow.xaml$"
# If found, let's see its content
fd "WelcomeWindow.xaml$" -x cat {}
Length of output: 7149
Flow.Launcher/SettingWindow.xaml.cs (1)
171-175: LGTM: Good defensive programming practice
The null check for selectedItem with fallback to the first page provides a robust solution to handle navigation edge cases and prevents potential crashes.
Flow.Launcher/WelcomeWindow.xaml (3)
46-46: LGTM! Margin format standardization.
The changes standardize margin format to space-separated values, which is the preferred XAML syntax.
Also applies to: 51-51
101-101: LGTM! Layout improvements for navigation controls.
The margin adjustments improve the spacing and alignment of navigation controls.
Since these are visual changes, please verify:
- The navigation controls are properly aligned
- The spacing between elements looks consistent
- The layout works well at different window sizes (the window has fixed dimensions: 550x650)
Also applies to: 115-115, 128-128, 135-135, 141-141
Line range hint 85-89: Good fix for the navigation timing issue.
Adding the ContentFrame_Loaded event handler ensures proper navigation timing by waiting for the frame to be fully loaded before navigation occurs. This directly addresses the reported issue.
Let's verify the implementation of the event handler:
✅ Verification successful
Verified: ContentFrame_Loaded implementation properly handles navigation timing
The implementation in WelcomeWindow.xaml.cs shows that ContentFrame_Loaded correctly navigates to the first page only after the frame is fully loaded:
ContentFrame.Navigate(PageTypeSelector(1), settings); /* Set First Page */This matches the intended behavior mentioned in the review comment and ensures proper navigation timing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the ContentFrame_Loaded implementation
ast-grep --pattern 'private void ContentFrame_Loaded(object sender, RoutedEventArgs e) {
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's try a broader search for ContentFrame_Loaded using ripgrep
rg -l "ContentFrame_Loaded" --type-add 'csharp:*.{cs,xaml}' -tcsharp
# Then get the content of files containing this handler
rg "ContentFrame_Loaded" --type-add 'csharp:*.{cs,xaml}' -tcsharp -C 5
Length of output: 3047
Flow.Launcher/SettingWindow.xaml (1)
174-174: Verify that the NavView_Loaded event handler is implemented correctly
The Loaded="NavView_Loaded" attribute has been added to the NavigationView. Ensure that the NavView_Loaded method is properly implemented in SettingWindow.xaml.cs to initialize the navigation view upon loading.
Run the following script to confirm the implementation of NavView_Loaded:
✅ Verification successful
Let me verify the implementation of ContentFrame_Loaded to ensure the complete navigation flow is properly handled.
Let me verify the complete navigation setup by checking the NavView definition and its menu items.
Let me check the menu items definition for the NavView to ensure the navigation setup is complete.
NavView_Loaded implementation is correct and properly integrated
The verification shows that:
NavView_Loadedevent handler is properly implemented, callingContentFrame_Loadedwhen the frame is loadedContentFrame_Loadedcorrectly initializes the navigation by selecting the first menu item- The navigation structure is complete with:
- A properly defined
ContentFramefor page content NavigationViewwith menu items (General, Plugins, etc.)- Default selection behavior implemented
- A properly defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the implementation of NavView_Loaded in SettingWindow.xaml.cs
# Test: Expect to find the method NavView_Loaded in SettingWindow.xaml.cs
rg -A 5 'void\s+NavView_Loaded' Flow.Launcher/SettingWindow.xaml.cs
Length of output: 240
Script:
#!/bin/bash
# Search for ContentFrame_Loaded implementation
rg -A 10 'void\s+ContentFrame_Loaded' Flow.Launcher/SettingWindow.xaml.cs
# Also check if ContentFrame is properly defined in XAML
rg 'ContentFrame' Flow.Launcher/SettingWindow.xaml
Length of output: 442
Script:
#!/bin/bash
# Search for NavView definition and menu items in XAML
rg -A 10 'x:Name="NavView"' Flow.Launcher/SettingWindow.xaml
Length of output: 542
Script:
#!/bin/bash
# Search for MenuItems within NavigationView
rg -B 5 -A 15 '<ui:NavigationView.MenuItems>' Flow.Launcher/SettingWindow.xaml
Length of output: 1241
|
On the other hand would you mind reverting the style change on xaml? |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
@Jack251970 I suppose this happens very randomly? Have you found a reliable way to reproduce the issue? |
If I catch this issue again, I will record all things. Currently no idea about how to reproduce. |
|
🥷 Code experts: onesounds onesounds has 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:
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 |
Long before, when I have upgraded Flow.Launcher to v1.19.x version, I encountered the setting window navigation issue like this:
(Note: This is just a demo screenshot. I just cannot find the original screenshot. :( I forget to create a issue for this.)
Now, I fix this possible issue to make sure the navigation event will only happen after all ui components are loaded. (I guess this is the problem)