Move window titlebar logic and ui to new CustomWindowTitleBar#4501
Move window titlebar logic and ui to new CustomWindowTitleBar#4501DavidGBrett wants to merge 29 commits into
Conversation
779ad97 to
1cceb2d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new CustomWindowTitleBar UserControl and migrates multiple windows/dialogs from inline title-bar XAML and per-window state handlers to the control, centralizing icon/title and minimize/maximize/close wiring and last-non-minimized state tracking. ChangesWindow Title Bar Component Consolidation
Sequence Diagram(s)sequenceDiagram
participant HostWindow
participant CustomWindowTitleBar
participant ConsumerWindow
HostWindow->>CustomWindowTitleBar: Loaded (attach, sync Icon/Title)
ConsumerWindow->>CustomWindowTitleBar: Configure Show* props / subscribe LastNonMinimizedWindowStateChanged
CustomWindowTitleBar->>CustomWindowTitleBar: Track LastNonMinimizedWindowState (StateChanged)
ConsumerWindow->>CustomWindowTitleBar: User clicks Close/Minimize/Maximize
CustomWindowTitleBar->>ConsumerWindow: Raise Close/Minimize/Maximize events
CustomWindowTitleBar->>HostWindow: Set WindowState or Close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml (1)
40-40: ⚡ Quick winInconsistent resource reference: use StaticResource consistently.
The
MinimizeButtonuses{DynamicResource TitleBarButtonStyle}(line 40), whileMaximizeButtonandRestoreButtonuse{StaticResource TitleBarButtonStyle}(lines 62, 84). Unless runtime style switching is needed for the minimize button specifically, useStaticResourceconsistently for better performance.♻️ Proposed fix to use StaticResource consistently
<Button x:Name="MinimizeButton" Grid.Column="2" Click="MinimizeButton_OnClick" RenderOptions.EdgeMode="Aliased" - Style="{DynamicResource TitleBarButtonStyle}"> + Style="{StaticResource TitleBarButtonStyle}">Also applies to: 62-62, 84-84, 107-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml` at line 40, The MinimizeButton currently uses a DynamicResource for TitleBarButtonStyle while MaximizeButton and RestoreButton use StaticResource; change the MinimizeButton's Style attribute to use {StaticResource TitleBarButtonStyle} so all title bar buttons (MinimizeButton, MaximizeButton, RestoreButton and any other title bar button references) consistently reference TitleBarButtonStyle with StaticResource for better performance and to avoid unnecessary runtime lookups.Flow.Launcher/ReportWindow.xaml (1)
33-33: 💤 Low valueRedundant
IconSource.
Window.Icon(Line 16) is already/Images/app_error.png, andIconSourcedefaults to the host window icon, so this assignment is redundant. Safe to drop unless you intend a different titlebar icon.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/ReportWindow.xaml` at line 33, Remove the redundant IconSource assignment in ReportWindow.xaml: the Window.Icon (set to "/Images/app_error.png") already provides the titlebar icon, and IconSource defaults to the host window icon, so delete the IconSource="/Images/app_error.png" attribute to avoid duplication (look for the IconSource attribute in the ReportWindow.xaml and remove it).Flow.Launcher/WelcomeWindow.xaml (1)
27-41: 💤 Low valueStale
#region TitleBarmarkers.The region now opens before
WindowChrome(Line 27) and closes after the titlebar control (Line 41), so it wraps unrelated layout. Consider tightening or removing these markers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/WelcomeWindow.xaml` around lines 27 - 41, The region markers currently capture unrelated layout because <!--#region TitleBar--> is placed before WindowChrome.WindowChrome while <!--#endregion--> is after controls:CustomWindowTitleBar; either move the opening marker so it starts immediately before WindowChrome.WindowChrome and place <!--#endregion--> immediately after the controls:CustomWindowTitleBar declaration, or remove both markers entirely; update the comments so the region spans only the titlebar elements (WindowChrome.WindowChrome and controls:CustomWindowTitleBar).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher/PluginSettingsWindow.xaml`:
- Around line 56-58: PluginSettingsWindow is missing wiring for
persistence-related window events: add the same root event attributes used by
SettingWindow (e.g., Loaded="OnLoaded", StateChanged="Window_StateChanged",
Activated="Window_Activated") to PluginSettingsWindow.xaml and wire the
CustomWindowTitleBar's LastNonMinimizedWindowStateChanged to
OnLastNonMinimizedWindowStateChanged (or a similarly named handler) on the
<controls:CustomWindowTitleBar> element; then implement the corresponding
handlers in PluginSettingsWindow.xaml.cs (OnLoaded, Window_StateChanged,
Window_Activated and OnLastNonMinimizedWindowStateChanged) or subscribe to
LastNonMinimizedWindowStateChanged in code-behind to persist/restore maximized
vs normal state just like SettingWindow does.
In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml`:
- Line 85: The RestoreButton XAML currently sets Visibility="Hidden" which
leaves reserved layout space; change it to Visibility="Collapsed" to match the
PR intent and the C# code that uses Visibility.Collapsed (see the code paths
that manipulate RestoreButton visibility in the window logic). Update the
RestoreButton element in CustomWindowTitleBar.xaml to use Collapsed so layout
gaps are removed and the XAML/C# behavior is consistent.
---
Nitpick comments:
In `@Flow.Launcher/ReportWindow.xaml`:
- Line 33: Remove the redundant IconSource assignment in ReportWindow.xaml: the
Window.Icon (set to "/Images/app_error.png") already provides the titlebar icon,
and IconSource defaults to the host window icon, so delete the
IconSource="/Images/app_error.png" attribute to avoid duplication (look for the
IconSource attribute in the ReportWindow.xaml and remove it).
In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml`:
- Line 40: The MinimizeButton currently uses a DynamicResource for
TitleBarButtonStyle while MaximizeButton and RestoreButton use StaticResource;
change the MinimizeButton's Style attribute to use {StaticResource
TitleBarButtonStyle} so all title bar buttons (MinimizeButton, MaximizeButton,
RestoreButton and any other title bar button references) consistently reference
TitleBarButtonStyle with StaticResource for better performance and to avoid
unnecessary runtime lookups.
In `@Flow.Launcher/WelcomeWindow.xaml`:
- Around line 27-41: The region markers currently capture unrelated layout
because <!--#region TitleBar--> is placed before WindowChrome.WindowChrome while
<!--#endregion--> is after controls:CustomWindowTitleBar; either move the
opening marker so it starts immediately before WindowChrome.WindowChrome and
place <!--#endregion--> immediately after the controls:CustomWindowTitleBar
declaration, or remove both markers entirely; update the comments so the region
spans only the titlebar elements (WindowChrome.WindowChrome and
controls:CustomWindowTitleBar).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6314bb97-0f56-403f-82b4-14bd5dcde60f
📒 Files selected for processing (25)
Flow.Launcher/ActionKeywords.xamlFlow.Launcher/ActionKeywords.xaml.csFlow.Launcher/CustomQueryHotkeySetting.xamlFlow.Launcher/CustomQueryHotkeySetting.xaml.csFlow.Launcher/CustomShortcutSetting.xamlFlow.Launcher/CustomShortcutSetting.xaml.csFlow.Launcher/MessageBoxEx.xamlFlow.Launcher/MessageBoxEx.xaml.csFlow.Launcher/PluginSettingsWindow.xamlFlow.Launcher/PluginSettingsWindow.xaml.csFlow.Launcher/PluginUpdateWindow.xamlFlow.Launcher/PluginUpdateWindow.xaml.csFlow.Launcher/ProgressBoxEx.xamlFlow.Launcher/ProgressBoxEx.xaml.csFlow.Launcher/ReleaseNotesWindow.xamlFlow.Launcher/ReleaseNotesWindow.xaml.csFlow.Launcher/ReportWindow.xamlFlow.Launcher/ReportWindow.xaml.csFlow.Launcher/Resources/Controls/CustomWindowTitleBar.xamlFlow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml.csFlow.Launcher/SelectBrowserWindow.xamlFlow.Launcher/SelectFileManagerWindow.xamlFlow.Launcher/SettingWindow.xamlFlow.Launcher/SettingWindow.xaml.csFlow.Launcher/WelcomeWindow.xaml
💤 Files with no reviewable changes (3)
- Flow.Launcher/ReportWindow.xaml.cs
- Flow.Launcher/ReleaseNotesWindow.xaml.cs
- Flow.Launcher/PluginSettingsWindow.xaml.cs
cb0add8 to
4d79031
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher/PluginUpdateWindow.xaml.cs (1)
63-75: 💤 Low valueConsider adding
e.Handled = truefor consistency.
BtnCancel_OnClicksetse.Handled = truebefore closing, butbtnUpdate_OnClickalso closes the window without marking the event handled. While this may not cause functional issues (since the Update button isn't the title bar close button), adding it would be consistent across all button handlers that close the dialog.♻️ Proposed fix for consistency
private void btnUpdate_OnClick(object sender, RoutedEventArgs e) { if (Plugins.Count == 0) { App.API.ShowMsgBox(Localize.updatePluginNoSelected()); return; } _ = PluginInstaller.UpdateAllPluginsAsync(Plugins, Restart); + e.Handled = true; DialogResult = true; Close(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/PluginUpdateWindow.xaml.cs` around lines 63 - 75, btnUpdate_OnClick currently closes the dialog without marking the click event handled; for consistency with BtnCancel_OnClick add e.Handled = true at the start (or just before closing) in the btnUpdate_OnClick handler so the RoutedEventArgs is marked handled before calling DialogResult = true and Close(); update the method referenced as btnUpdate_OnClick to mirror the existing behavior in BtnCancel_OnClick.Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml (1)
47-55: 💤 Low valueConsider extracting the inactive-window opacity style to a shared resource.
The same
DataTriggerstyle for reducing opacity whenWindow.IsActive == Falseis repeated four times for each button'sPath. This could be extracted to a sharedStylein the UserControl's resources.♻️ Proposed refactor to reduce duplication
<UserControl x:Class="Flow.Launcher.Resources.Controls.CustomWindowTitleBar" ...> + <UserControl.Resources> + <Style x:Key="TitleBarPathStyle" TargetType="Path"> + <Style.Triggers> + <DataTrigger Binding="{Binding Path=IsActive, RelativeSource={RelativeSource FindAncestor, AncestorType={x:Type Window}}}" Value="False"> + <Setter Property="Opacity" Value="0.5" /> + </DataTrigger> + </Style.Triggers> + </Style> + </UserControl.Resources> <Grid> ... <Path ... - <Path.Style> - <Style TargetType="Path"> - <Style.Triggers> - <DataTrigger Binding="{Binding Path=IsActive, ...}" Value="False"> - <Setter Property="Opacity" Value="0.5" /> - </DataTrigger> - </Style.Triggers> - </Style> - </Path.Style> + Style="{StaticResource TitleBarPathStyle}" </Path>Also applies to: 69-77, 92-100, 114-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml` around lines 47 - 55, Extract the repeated DataTrigger style into the UserControl's resources by adding a Style (TargetType="Path") in UserControl.Resources (e.g., key it "InactiveWindowPathStyle") that contains the DataTrigger checking Window.IsActive == False and setting Opacity to 0.5, then replace each Path.Style block in the control with Style="{StaticResource InactiveWindowPathStyle}" so the identical trigger used in the Path elements (the DataTrigger binding to IsActive via RelativeSource FindAncestor AncestorType=Window) is centralized and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 53: The README's separator <img> tag (src
"https://user-images.githubusercontent.com/6903107/144858082-8b654daf-60fb-4ee6-89b2-6183b73510d1.png")
is missing an alt attribute; add an appropriate alt text to the <img> element
(for decorative use, set alt="" to mark it as decorative, otherwise provide a
short descriptive string) so it satisfies accessibility lint MD045 and improves
screen-reader UX.
---
Nitpick comments:
In `@Flow.Launcher/PluginUpdateWindow.xaml.cs`:
- Around line 63-75: btnUpdate_OnClick currently closes the dialog without
marking the click event handled; for consistency with BtnCancel_OnClick add
e.Handled = true at the start (or just before closing) in the btnUpdate_OnClick
handler so the RoutedEventArgs is marked handled before calling DialogResult =
true and Close(); update the method referenced as btnUpdate_OnClick to mirror
the existing behavior in BtnCancel_OnClick.
In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml`:
- Around line 47-55: Extract the repeated DataTrigger style into the
UserControl's resources by adding a Style (TargetType="Path") in
UserControl.Resources (e.g., key it "InactiveWindowPathStyle") that contains the
DataTrigger checking Window.IsActive == False and setting Opacity to 0.5, then
replace each Path.Style block in the control with Style="{StaticResource
InactiveWindowPathStyle}" so the identical trigger used in the Path elements
(the DataTrigger binding to IsActive via RelativeSource FindAncestor
AncestorType=Window) is centralized and reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec36778b-95c6-4ad6-a669-ba88f02e0d50
📒 Files selected for processing (26)
Flow.Launcher/ActionKeywords.xamlFlow.Launcher/ActionKeywords.xaml.csFlow.Launcher/CustomQueryHotkeySetting.xamlFlow.Launcher/CustomQueryHotkeySetting.xaml.csFlow.Launcher/CustomShortcutSetting.xamlFlow.Launcher/CustomShortcutSetting.xaml.csFlow.Launcher/MessageBoxEx.xamlFlow.Launcher/MessageBoxEx.xaml.csFlow.Launcher/PluginSettingsWindow.xamlFlow.Launcher/PluginSettingsWindow.xaml.csFlow.Launcher/PluginUpdateWindow.xamlFlow.Launcher/PluginUpdateWindow.xaml.csFlow.Launcher/ProgressBoxEx.xamlFlow.Launcher/ProgressBoxEx.xaml.csFlow.Launcher/ReleaseNotesWindow.xamlFlow.Launcher/ReleaseNotesWindow.xaml.csFlow.Launcher/ReportWindow.xamlFlow.Launcher/ReportWindow.xaml.csFlow.Launcher/Resources/Controls/CustomWindowTitleBar.xamlFlow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml.csFlow.Launcher/SelectBrowserWindow.xamlFlow.Launcher/SelectFileManagerWindow.xamlFlow.Launcher/SettingWindow.xamlFlow.Launcher/SettingWindow.xaml.csFlow.Launcher/WelcomeWindow.xamlREADME.md
💤 Files with no reviewable changes (3)
- Flow.Launcher/ReportWindow.xaml.cs
- Flow.Launcher/PluginSettingsWindow.xaml.cs
- Flow.Launcher/ReleaseNotesWindow.xaml.cs
🚧 Files skipped from review as they are similar to previous changes (19)
- Flow.Launcher/ProgressBoxEx.xaml.cs
- Flow.Launcher/CustomShortcutSetting.xaml.cs
- Flow.Launcher/CustomQueryHotkeySetting.xaml.cs
- Flow.Launcher/WelcomeWindow.xaml
- Flow.Launcher/ActionKeywords.xaml.cs
- Flow.Launcher/MessageBoxEx.xaml
- Flow.Launcher/ReportWindow.xaml
- Flow.Launcher/SelectFileManagerWindow.xaml
- Flow.Launcher/PluginUpdateWindow.xaml
- Flow.Launcher/CustomShortcutSetting.xaml
- Flow.Launcher/ActionKeywords.xaml
- Flow.Launcher/SettingWindow.xaml
- Flow.Launcher/CustomQueryHotkeySetting.xaml
- Flow.Launcher/ReleaseNotesWindow.xaml
- Flow.Launcher/ProgressBoxEx.xaml
- Flow.Launcher/MessageBoxEx.xaml.cs
- Flow.Launcher/SelectBrowserWindow.xaml
- Flow.Launcher/PluginSettingsWindow.xaml
- Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml.cs
…CustomWindowTitleBar
These are internal details that are no longer needed externally
…ing its own version
…icating its own version
…ng its own version
Now that CustomWindowTitleBar is its own control we don't need the complicated grid columns
…leBar This removes the space reserved for the hidden button Otherwise if the restore button was hidden there would still be a gap between minimize and close buttons
…the Icon set on the host window if no other value set
…indow Since this now defaults to the Icon set on the window, it is better not to duplciate that
…ng its own version
…ating its own version
…licating its own version
…s style like the other buttons
4d79031 to
7aa6250
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml (1)
35-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd automation names to the caption buttons.
These buttons are Path-only, so assistive tech will surface them as unlabeled controls. Since this shared control is now used across most windows, the accessibility gap becomes global.
Proposed fix
<Button x:Name="MinimizeButton" Grid.Column="2" + AutomationProperties.Name="Minimize" Click="MinimizeButton_OnClick" RenderOptions.EdgeMode="Aliased" Style="{StaticResource TitleBarButtonStyle}"> @@ <Button x:Name="MaximizeButton" Grid.Column="3" + AutomationProperties.Name="Maximize" Click="MaximizeRestoreButton_OnClick" Style="{StaticResource TitleBarButtonStyle}"> @@ <Button x:Name="RestoreButton" Grid.Column="3" + AutomationProperties.Name="Restore" Click="MaximizeRestoreButton_OnClick" Style="{StaticResource TitleBarButtonStyle}" Visibility="Collapsed"> @@ <Button x:Name="CloseButton" Grid.Column="4" + AutomationProperties.Name="Close" Click="CloseButton_OnClick" Style="{StaticResource TitleBarCloseButtonStyle}">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml` around lines 35 - 72, The caption buttons are path-only and lack automation names; update each Button element (MinimizeButton, MaximizeButton, RestoreButton, CloseButton) to include an accessibility label by adding AutomationProperties.Name (e.g., "Minimize", "Maximize", "Restore", "Close") so assistive tech can announce them; ensure the RestoreButton's label matches its visible purpose when Visibility toggles and keep existing Style and Click handlers untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xaml`:
- Around line 35-72: The caption buttons are path-only and lack automation
names; update each Button element (MinimizeButton, MaximizeButton,
RestoreButton, CloseButton) to include an accessibility label by adding
AutomationProperties.Name (e.g., "Minimize", "Maximize", "Restore", "Close") so
assistive tech can announce them; ensure the RestoreButton's label matches its
visible purpose when Visibility toggles and keep existing Style and Click
handlers untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a0f8656-cca3-4340-b67c-04e42c6e9753
📒 Files selected for processing (2)
Flow.Launcher/Resources/Controls/CustomWindowTitleBar.xamlFlow.Launcher/Resources/CustomControlTemplate.xaml
…sibility Tells screen readers etc what these buttons are for
|
You can use the changes in PR #4504 to have clean test windows for Report, Progress & Plugin Update (the branches should merge cleanly) I just put that together quickly - so might do more with that still but should be good enough to use temporarily for testing this |
Summary
Bringing up the Windows
new ReportWindow(new Exception("test")).Show();)You can use the changes in PR #4504 to have clean test windows for Report, Progress & Plugin Update (the branches should merge cleanly)
Summary by cubic
Centralized the window title bar UI and logic into a shared
CustomWindowTitleBaracross thirteen windows to remove duplication and keep behavior consistent. Hidden buttons now collapse, titles/icons default to the host window, and buttons include accessibility names for screen readers.Summary of changes
SettingWindow,PluginSettingsWindow,ReleaseNotesWindow,SelectBrowserWindow,SelectFileManagerWindow,WelcomeWindow,ReportWindow,MessageBoxEx,ProgressBoxEx,PluginUpdateWindow,CustomShortcutSetting,CustomQueryHotkeySetting, andActionKeywordsnow useCustomWindowTitleBar.SettingWindowpersists state viaLastNonMinimizedWindowStateChanged.e.Handled) to keep dialog behavior.TitleBarButtonPathStylefor consistent visuals.CustomWindowTitleBar(XAML + .cs) with settableTitle(defaults to window),IconSourceasImageSource(defaults to window icon), and toggles forShowIcon,ShowTitle,ShowMinimizeButton,ShowMaximizeRestoreButton,ShowCloseButton.LastNonMinimizedWindowStateand a change event; hidden buttons collapse to remove gaps.AutomationProperties.Nameon minimize, maximize/restore, and close buttons for screen readers.Release Note
All app windows now share a consistent title bar with standard controls and improved accessibility.
Written for commit c890fc2. Summary will update on new commits.