Support Placeholder Text & Toggle Resize Search Window Mode #3396
Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request introduces several UI and functionality changes across multiple modules. In the core theme file, property access levels are restricted, and methods for adjusting window border thickness are added. The obsolete Changes
Sequence Diagram(s)sequenceDiagram
participant MW as MainWindow
participant QTB as QueryTextBox
participant QTSB as QueryTextSuggestionBox
participant PTB as QueryTextPlaceholderBox
participant Settings as Settings
MW->>MW: SetupPlaceholderText()
MW->>QTB: Subscribe to TextChanged event
MW->>QTSB: Subscribe to TextChanged event
QTB->>MW: TextChanged event triggered
QTSB->>MW: TextChanged event triggered
MW->>MW: SetPlaceholderText() updates PTB visibility
sequenceDiagram
participant MW as MainWindow
participant Settings as Settings
participant Theme as Theme
MW->>Settings: Retrieve ResizeWindow setting
MW->>Theme: Call SetResizeBorderThickness(Thickness?)
MW->>MW: Update window ResizeMode based on setting
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
546-570: Fixed minor typo in method name.The method name contains a typo: "Boarder" should be "Border". This appears to be consistent with the existing codebase and isn't introduced by this PR, but it might be worth correcting in a future update.
-private void SetResizeBoarderThickness(Thickness? effectMargin) +private void SetResizeBorderThickness(Thickness? effectMargin)Also applies to the method name on line 534.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Flow.Launcher.Core/Resource/Theme.cs(8 hunks)Flow.Launcher.Core/Resource/ThemeManager.cs(0 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/App.xaml.cs(0 hunks)Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/MainWindow.xaml(1 hunks)Flow.Launcher/MainWindow.xaml.cs(5 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml(5 hunks)
💤 Files with no reviewable changes (2)
- Flow.Launcher/App.xaml.cs
- Flow.Launcher.Core/Resource/ThemeManager.cs
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-413)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
🔇 Additional comments (24)
Flow.Launcher/MainWindow.xaml (1)
221-228: Well-structured implementation of placeholder text functionalityThe new TextBox implementation provides a clean way to show placeholder text when the query box is empty. The control is properly configured with:
- Appropriate binding to maintain consistent styling with the main query box
- IsEnabled="False" to ensure it's just a visual element with no input capability
- Visibility set to "Collapsed" by default, which will be toggled based on the ShowPlaceholder setting
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (2)
262-266: Clean implementation of ResizeWindow propertyThe property is correctly implemented with a simple getter and setter that interacts with the Settings object. This provides a clean way for the UI to bind to this setting.
268-272: Good implementation of ShowPlaceholder propertyThe property properly exposes the ShowPlaceholder setting from the Settings object, enabling the UI to toggle the placeholder text visibility.
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (2)
116-125: Good implementation of ResizeWindow setting with default valueThe ResizeWindow property is correctly implemented with:
- Private backing field initialized to true (sensible default to enable window resizing)
- Property change notification via OnPropertyChanged()
- Clean getter and setter implementation
127-136: Well-implemented ShowPlaceholder setting with appropriate defaultThe ShowPlaceholder property is well-structured with:
- Private backing field initialized to false (logical default - don't show placeholder until user enables it)
- Property change notification properly implemented
- Clean property implementation
Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml (4)
501-511: Good UI implementation for ResizeWindow settingThe Card element for the ResizeWindow setting is well-structured with:
- Appropriate title and icon
- Tooltip providing context via the "ResizeWindowTip" resource
- ToggleSwitch properly bound to the ResizeWindow property
- Consistent UI styling with other settings cards
544-554: Well-structured UI implementation for PlaceholderText settingThe Card element for the PlaceholderText setting is properly implemented with:
- Clear title and appropriate icon
- Tooltip providing context via the "PlaceholderTextTip" resource
- ToggleSwitch correctly bound to the ShowPlaceholder property
- Consistent UI styling with other settings cards
513-514: Minor comment improvement for consistencyUpdated the comment to use lowercase "height" for consistency in the comments formatting throughout the file.
266-267: Minor comment improvement for consistencyUpdated the comment to use lowercase "preview" for consistency in the comments formatting throughout the file.
Flow.Launcher/MainWindow.xaml.cs (6)
14-14: Added System.Windows.Shell namespace.This is a necessary addition to support the WindowChrome functionality used for the window resizing feature.
119-121: Good placement for placeholder text initialization.The placeholder text setup is correctly initialized after the control elements are created but before the window is shown to the user.
153-157: Good sequencing of resize mode initialization.Setting up the resize mode after theme frame refresh ensures that any theme-related border settings are applied correctly before configuring the resize behavior.
248-253: Proper property change notification handling.The code correctly subscribes to property changes for the new settings and calls the appropriate setup methods when those settings change, enabling dynamic UI updates.
1045-1080: Well-structured placeholder text implementation.The placeholder text implementation follows a clean pattern:
- The setup method adds/removes event handlers based on settings
- Event handlers update the placeholder visibility when text changes
- The visibility logic correctly checks both text boxes
1082-1093: Well-implemented resize mode functionality.The resize mode implementation correctly:
- Sets the window's ResizeMode property based on user settings
- Updates the window chrome's border thickness using the theme object
- Accesses the theme functionality through the appropriate method call
Flow.Launcher.Core/Resource/Theme.cs (6)
27-27: Improved encapsulation by restricting the BlurEnabled property setter.Changing the setter from public to private ensures that this property can only be modified within the Theme class, preventing unexpected changes from external code.
45-46: Added private field to store theme resize border thickness.This field appropriately stores the theme-specific border thickness that's needed for the window resizing functionality.
468-469: Generalized property references.Replacing Border-specific property references with more general UIElement and FrameworkElement references broadens their application scope and is more appropriate for the intended usage.
Also applies to: 483-484
513-514: Consistent property reference updates.These property reference updates maintain consistency with the earlier changes and follow the same pattern of using more general property accessors.
Also applies to: 518-519
534-544: New method for configuring WindowChrome resize behavior.This method cleanly implements the functionality to toggle the resize border thickness based on the user's preference. The conditional logic ensures that resize borders are only applied when the feature is enabled.
600-601: Changed dispatcher priority to improve UI responsiveness.Changing the DispatcherPriority from Normal to Render may help ensure that UI updates related to theme changes happen at the most appropriate time in the rendering pipeline.
Also applies to: 614-615
Flow.Launcher/Languages/en.xaml (3)
42-42: Added placeholder text resource.The "Type here to search" message provides clear guidance to users about the purpose of the search box.
198-202: Added backdrop type resources.These resources provide localized strings for the different backdrop type options in the UI.
205-208: Added resources for new UI features.These well-named resources provide clear and descriptive text for the placeholder and resize window features in the settings UI, including helpful tooltips that explain their functionality.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces support for displaying placeholder text when the query is empty and enables users to toggle window resize mode. The changes include adding new settings properties for placeholder text and resize behavior, updating the main window logic to initialize these features, removing an obsolete ThemeManager, and enhancing theme-related logic in the application.
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Added properties to expose ResizeWindow and ShowPlaceholder settings. |
| Flow.Launcher/MainWindow.xaml.cs | Integrated initialization for placeholder text and resize mode with new setup methods. |
| Flow.Launcher/App.xaml.cs | Removed an obsolete comment regarding ThemeManager usage. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Introduced backing fields and properties for ResizeWindow and ShowPlaceholder. |
| Flow.Launcher.Core/Resource/ThemeManager.cs | Removed obsolete ThemeManager in favor of using DI for Theme. |
| Flow.Launcher.Core/Resource/Theme.cs | Updated DropShadow effect settings and added methods for adjusting the resize border thickness. |
Files not reviewed (3)
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/MainWindow.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml: Language not supported
Comments suppressed due to low confidence (2)
Flow.Launcher.Core/Resource/Theme.cs:534
- The method name 'SetResizeBoarderThickness' contains a misspelling; consider renaming it to 'SetResizeBorderThickness' for clarity.
public void SetResizeBoarderThickness(WindowChrome windowChrome, bool resizeWindow)
Flow.Launcher.Core/Resource/Theme.cs:548
- The private method 'SetResizeBoarderThickness' has a typographical error in its name; it should be renamed to 'SetResizeBorderThickness' to match standard naming conventions.
private void SetResizeBoarderThickness(Thickness? effectMargin)
This comment has been minimized.
This comment has been minimized.
|
🥷 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:
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:
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml (2)
267-267: Improved comment format consistency.Minor improvement to comment format for better consistency.
514-514: Improved comment format consistency.Minor improvement to comment format for better consistency.
Flow.Launcher/MainWindow.xaml.cs (2)
1049-1084: Well-structured placeholder text implementation.The placeholder region provides a clean, organized implementation for the placeholder text functionality. Key observations:
- Good separation of concerns with distinct methods for setup, event handling, and visibility logic
- Proper event subscription/unsubscription based on settings
- Clear visibility logic checking both text inputs
Consider adding null checks for UI elements to prevent potential NullReferenceExceptions if called before UI initialization:
private void SetPlaceholderText() { if (QueryTextBox == null || QueryTextSuggestionBox == null || QueryTextPlaceholderBox == null) return; var queryText = QueryTextBox.Text; var suggestionText = QueryTextSuggestionBox.Text; QueryTextPlaceholderBox.Visibility = string.IsNullOrEmpty(queryText) && string.IsNullOrEmpty(suggestionText) ? Visibility.Visible : Visibility.Collapsed; }
1086-1097: Clean resize mode implementation with minor spelling inconsistency.The resize mode implementation is concise and effectively toggles the window's resize capability. However, there's a spelling inconsistency in the method name being called from Theme.cs -
SetResizeBoarderThicknessshould be "Border" not "Boarder".This is consistent with the misspelling in the Theme class itself, but it would be good to correct this spelling across the codebase in a future update.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/MainWindow.xaml(1 hunks)Flow.Launcher/MainWindow.xaml.cs(5 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml(6 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/MainWindow.xaml
- Flow.Launcher/Languages/en.xaml
🧰 Additional context used
🧬 Code Definitions (3)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-423)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
Flow.Launcher/App.xaml.cs (2)
App(28-338)App(50-114)Flow.Launcher/PublicAPIInstance.cs (1)
GetTranslation(158-158)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-423)Flow.Launcher.Core/Resource/Theme.cs (2)
SetResizeBoarderThickness(534-544)SetResizeBoarderThickness(548-570)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
🔇 Additional comments (11)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
768-784: Implementation of PlaceholderText looks good.The property correctly handles the null/empty case by providing a localized default value, and properly implements property change notification. This approach ensures the UI will always have appropriate placeholder text to display.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (1)
262-278: Well-implemented settings properties.The three new properties (ResizeWindow, ShowPlaceholder, and PlaceholderText) follow the established pattern in this class, correctly delegating to the corresponding properties in the Settings object. They are logically grouped together, making the code easy to understand and maintain.
Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml (2)
502-512: Good implementation of the window resize option UI.The card for the "Resize Window" setting follows the established pattern with appropriate icon, tooltip, and binding to the view model. The UI element is positioned logically in the settings flow.
546-566: Well-structured placeholder text settings UI.The card group for placeholder text settings provides a clean interface with both the toggle option and text input field. The implementation follows the established UI patterns with proper bindings, informative tooltips, and consistent styling.
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
116-146: Well-implemented settings properties with appropriate defaults.The three new properties (ResizeWindow, ShowPlaceholder, and PlaceholderText) follow the established pattern in the Settings class with private backing fields and proper property change notification. The default values are sensible: window resizing is enabled by default while the placeholder feature is disabled by default with an empty placeholder text.
Flow.Launcher/MainWindow.xaml.cs (6)
14-14: Appropriate addition of System.Windows.Shell namespace.The added import is necessary for accessing the WindowChrome class, which is used in the resize mode implementation. Good practice to only add the namespaces that are needed.
119-121: Good initialization sequence for placeholder text.The implementation properly initializes the placeholder text feature during window loading. The code sets up event handlers and updates the UI with the configured placeholder text from settings.
156-157: Proper sequencing of resize mode setup.Setting up the resize mode after refreshing the theme frame is the correct approach, as it ensures that the theme's properties are fully initialized before configuring the resize behavior.
249-251: Appropriate property change handler for ShowPlaceholder.The code correctly responds to changes in the ShowPlaceholder setting by calling SetupPlaceholderText(), which will add or remove the necessary event handlers.
252-254: Synchronizing PlaceholderText with ViewModel.The implementation properly updates the ViewModel's PlaceholderText property when the setting changes, maintaining the data binding consistency.
255-257: Responsive window resize mode update.The property change handler correctly calls SetupResizeMode() when the ResizeWindow setting changes, allowing for dynamic updates to the window's resize behavior.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
1093-1104: Typo in method name referenced.The implementation of the resize mode functionality is good, but there's a spelling error in the method name referenced.
In
_theme.SetResizeBoarderThickness(), "Boarder" should be "Border":- _theme.SetResizeBoarderThickness(windowChrome, _settings.KeepMaxResults); + _theme.SetResizeBorderThickness(windowChrome, _settings.KeepMaxResults);This should be fixed here and in the Theme class where the method is defined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml(2 hunks)Flow.Launcher/MainWindow.xaml.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/MainWindow.xaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (7)
Flow.Launcher/MainWindow.xaml.cs (7)
14-14: Appropriate import for window chrome functionality.The addition of the
System.Windows.Shellnamespace is necessary for accessing theWindowChromeclass used in the new resize mode functionality.
119-122: Good initialization of placeholder text functionality.The placeholder setup is appropriately placed in the window's loading sequence, ensuring it's initialized before the user interacts with the interface.
156-157: Proper sequencing of resize mode initialization.The resize mode is correctly set up after the theme frame is refreshed, ensuring all visual elements are properly initialized before applying resize behavior.
249-257: Appropriate property change handlers for new settings.The addition of these property change handlers ensures the UI correctly responds when the user changes placeholder or resize mode settings. This follows good reactive programming practices.
467-476: Good handling of shadow margins in size calculations.The code now accounts for drop shadow effects when calculating the available space, which will provide more consistent UI sizing across different theme configurations.
492-496: Proper conditional width updating.The code correctly updates the window width in the view model only when resizing is enabled, maintaining consistency with the application's settings.
1056-1091: Well-implemented placeholder text functionality.The placeholder implementation follows good practices:
- Properly manages event subscriptions based on settings
- Uses a clean separation of concerns with dedicated methods
- Correctly determines visibility based on the state of both text boxes
This will improve user experience by providing guidance when the input is empty.
This comment has been minimized.
This comment has been minimized.
Jack251970
added
enhancement
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces placeholder text functionality when the query is empty and enables toggling the fixed window size, along with updating related settings and UI behavior.
- Updates width adjustment commands in the main view model.
- Adds placeholder text properties and event handlers in both view model and view layers.
- Refactors settings management and theme resource methods while removing an obsolete ThemeManager.
Reviewed Changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Replaces direct Settings.WindowSize updates with MainWindowWidth and adds placeholder text support. |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Adds properties to control and display placeholder text options and tips. |
| Flow.Launcher/MainWindow.xaml.cs | Integrates placeholder text setup, updates theme refresh, and initializes window resize mode. |
| Flow.Launcher/App.xaml.cs | Removes an obsolete comment and retains theme change behavior. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Introduces properties for placeholder text and refactors the KeepMaxResults setting with proper change notifications. |
| Flow.Launcher.Core/Resource/ThemeManager.cs | Removes the obsolete ThemeManager. |
| Flow.Launcher.Core/Resource/Theme.cs | Refines drop shadow effect implementation and adds a new method for setting the resize border thickness. |
Files not reviewed (6)
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/MainWindow.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml: Language not supported
- Flow.Launcher/Themes/Discord Dark.xaml: Language not supported
- Flow.Launcher/Themes/League.xaml: Language not supported
- Flow.Launcher/Themes/Pink.xaml: Language not supported
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for displaying placeholder text when the query is empty and enables toggling the window resize mode based on user settings. Key changes include updating the window width logic in the main view model, adding new properties and initialization for placeholder text in multiple view models and the main window, and refactoring theme management to update resize border thickness.
Reviewed Changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Updates window width handling and adds a new PlaceholderText property. |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Introduces ShowPlaceholder and PlaceholderText properties, and provides a translated tip. |
| Flow.Launcher/MainWindow.xaml.cs | Initializes placeholder text and sets up event handlers; adds support for toggling resize mode. |
| Flow.Launcher/App.xaml.cs | Minor cleanup by removing a TODO comment. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Modifies property setters for ShowPlaceholder, PlaceholderText, and KeepMaxResults. |
| Flow.Launcher.Core/Resource/ThemeManager.cs | Removes obsolete ThemeManager class. |
| Flow.Launcher.Core/Resource/Theme.cs | Updates drop shadow effect and border margin properties, and adds logic to update resize border thickness. |
Files not reviewed (6)
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/MainWindow.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml: Language not supported
- Flow.Launcher/Themes/Discord Dark.xaml: Language not supported
- Flow.Launcher/Themes/League.xaml: Language not supported
- Flow.Launcher/Themes/Pink.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Resource/Theme.cs:548
- The method name 'SetResizeBoarderThickness' contains a typo ('Boarder' instead of 'Border'). Consider renaming it to 'SetResizeBorderThickness' for clarity and consistency.
private void SetResizeBoarderThickness(Thickness? effectMargin)
@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 ...
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Flow.Launcher/MainWindow.xaml.cs (3)
468-473: Consider documenting the shadow margin magic number.While the shadow margin calculation is correct, the value
32appears to be a magic number without explanation.// Get shadow margin var shadowMargin = 0; var (_, useDropShadowEffect) = _theme.GetActualValue(); if (useDropShadowEffect) { - shadowMargin = 32; + // Shadow margin of 32 accounts for the drop shadow effect's visual overflow + shadowMargin = 32; }
475-476: Document the formula for calculating max results.The calculation formula includes magic numbers that would benefit from clarification for future maintainers.
// Calculate max results to show -var itemCount = (Height - (_settings.WindowHeightSize + 14) - shadowMargin) / _settings.ItemHeightSize; +// Height - (window header height + padding(14px)) - shadow margin divided by individual item height +var itemCount = (Height - (_settings.WindowHeightSize + 14) - shadowMargin) / _settings.ItemHeightSize;
1056-1091: Consider using IsNullOrWhiteSpace for better placeholder behavior.The placeholder implementation is well-structured, but consider using
string.IsNullOrWhiteSpace()instead ofstring.IsNullOrEmpty()to handle cases where the text boxes contain only whitespace.private void SetPlaceholderText() { var queryText = QueryTextBox.Text; var suggestionText = QueryTextSuggestionBox.Text; - QueryTextPlaceholderBox.Visibility = string.IsNullOrEmpty(queryText) && string.IsNullOrEmpty(suggestionText) ? Visibility.Visible : Visibility.Collapsed; + QueryTextPlaceholderBox.Visibility = string.IsNullOrWhiteSpace(queryText) && string.IsNullOrWhiteSpace(suggestionText) ? Visibility.Visible : Visibility.Collapsed; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.cs(8 hunks)Flow.Launcher/MainWindow.xaml.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Core/Resource/Theme.cs
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/MainWindow.xaml.cs (4)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
ResetPreview(980-995)Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-435)Flow.Launcher.Infrastructure/Win32Helper.cs (1)
Win32Helper(19-491)Flow.Launcher.Core/Resource/Theme.cs (1)
SetResizeBorderThickness(534-544)
🔇 Additional comments (6)
Flow.Launcher/MainWindow.xaml.cs (6)
14-14: Import added for window chrome customization.The addition of the
System.Windows.Shellnamespace is necessary for utilizing theWindowChromeclass in the resize mode functionality.
119-122: Good initialization of placeholder text functionality.The placeholder functionality is correctly initialized during window loading, ensuring it's set up before the UI is shown to users. The view model is properly updated with the placeholder text from settings.
154-158: Proper sequencing of resize mode setup after theme refresh.Setting up the resize mode after refreshing the theme frame ensures that all visual elements are properly initialized before configuring resizing behavior.
249-257: Well-structured property change handlers for new settings.The added handlers react appropriately to changes in the relevant settings, ensuring UI synchronization with user preferences for placeholders and window resizing.
492-496: Width update conditional is properly implemented.The width update properly respects the
KeepMaxResultssetting, only updating the view model's width property when resizing is allowed.
1093-1104: Clean implementation of resize mode toggle.The resize mode functionality is well-implemented, appropriately setting the window's
ResizeModeproperty and border thickness based on user settings. The null check onWindowChrome.GetWindowChrome(this)is a good defensive programming practice.
| #region Placeholder | ||
|
|
||
| private void SetupPlaceholderText() | ||
| { | ||
| if (_settings.ShowPlaceholder) | ||
| { | ||
| QueryTextBox.TextChanged += QueryTextBox_TextChanged; | ||
| QueryTextSuggestionBox.TextChanged += QueryTextSuggestionBox_TextChanged; | ||
| SetPlaceholderText(); | ||
| } | ||
| else | ||
| { | ||
| QueryTextBox.TextChanged -= QueryTextBox_TextChanged; | ||
| QueryTextSuggestionBox.TextChanged -= QueryTextSuggestionBox_TextChanged; | ||
| QueryTextPlaceholderBox.Visibility = Visibility.Collapsed; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider event handler cleanup in Dispose method.
Event handlers are correctly added/removed based on settings, but consider also ensuring these handlers are detached in the Dispose method to prevent potential memory leaks.
🏁 Script executed:
#!/bin/bash
# Check if event handlers are properly cleaned up in Dispose method
echo "Checking MainWindow.xaml.cs for event handler cleanup in Dispose method..."
rg -A 5 -B 5 "Dispose\(bool disposing\)" Flow.Launcher/MainWindow.xaml.csLength of output: 708
Event Handler Cleanup in Dispose Method Needed
The setup in SetupPlaceholderText properly adds and removes the event handlers based on settings, but as verified, the Dispose(bool disposing) method does not currently include logic to detach these handlers. This omission could lead to memory leaks if the handlers remain attached even after disposal.
- Action Item: Update the
Dispose(bool disposing)method (inFlow.Launcher/MainWindow.xaml.cs) to remove the attached event handlers (e.g., detachQueryTextBox_TextChangedandQueryTextSuggestionBox_TextChanged) whendisposingis true.
| @@ -39,7 +39,7 @@ | |||
| <system:String x:Key="GameMode">Game Mode</system:String> | |||
| <system:String x:Key="GameModeToolTip">Suspend the use of Hotkeys.</system:String> | |||
| <system:String x:Key="PositionReset">Position Reset</system:String> | |||
| <system:String x:Key="PositionResetToolTip">Reset search window position</system:String> | |||
There was a problem hiding this comment.
Please see #3448
jjw24
changed the title
Placeholder Text
Add placeholder text when query is empty
Fixed Window Size
Allow dragging to change window size