Fix Clock/Search Icon show logic #3388
Conversation
onesounds
added
kind/ui
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe changes remove the Changes
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 (3)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
1476-1477: Remove debug statementThe
Debug.WriteLine("Call Animation")statement should be removed as it's not needed in production code.- Debug.WriteLine("Call Animation");
1544-1546: Remove empty dispatcher invokeThere's an empty dispatcher invoke block that doesn't perform any operations. It should be removed to avoid confusion and unnecessary code.
- Application.Current.Dispatcher.Invoke(() => - { - - }, DispatcherPriority.Render);
1558-1559: Remove commented-out codeThe commented-out
mainWindow.SearchIcon.Visibility = Visibility.Hidden;line should be removed for cleaner code.- //mainWindow.SearchIcon.Visibility = Visibility.Hidden;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(5 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (6)
Flow.Launcher/MainWindow.xaml.cs (3)
770-770: Method accessibility changed from private to publicThe
WindowAnimationmethod has been made public to allow it to be called fromMainViewModel. This is necessary for the new animation approach, but changing method visibility is a significant API change.
774-783: Simplified icon visibility logic improves animation behaviorThe conditional logic now directly controls the opacity of icons based on animation settings instead of using the removed
_animatingflag. This is a cleaner approach that addresses the issue where animation ceases to function when a query is being typed.
852-857: Animation sequencing simplifiedThe animation starting logic has been streamlined by removing the
_animatingflag checks and assignments. This removes the complexity that was causing issues with animation when in query-remaining mode.Flow.Launcher/ViewModel/MainViewModel.cs (3)
1442-1469: Enhanced icon visibility logic in Show methodThe updated logic correctly:
- Sets clock panel and search icon opacity based on animation settings
- Prevents the clock from appearing when a query exists
- Hides the search icon when a plugin-specific icon is shown
This addresses the overlapping issues with both the clock and search icons.
1510-1522: Explicit LastQueryMode handling improves code clarityThe commented-out block has been replaced with a cleaner case statement for handling empty query mode. This improves code readability and maintainability.
1547-1564: Improved Hide method icon handlingThe enhanced logic in the Hide method properly:
- Controls icon opacity based on animation settings
- Sets appropriate visibility states
- Forces UI updates with UpdateLayout calls
This addresses issues where the UI state wasn't properly updated during hide operations.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 onesounds, Jack251970 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: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
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 ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
1480-1480: Consider removing commented-out codeThis commented-out line appears to be old code that's no longer needed since the visibility logic has been refactored.
- //mainWindow.SearchIcon.Visibility = Visibility.Visible;
1560-1560: Consider removing commented-out codeSimilar to the previous comment, this appears to be old code that's no longer needed with the new approach.
- //MainWindowOpacity = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(11 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/ViewModel/MainViewModel.cs (5)
Flow.Launcher/MainWindow.xaml.cs (2)
MainWindow(30-1034)MainWindow(70-82)Flow.Launcher/PublicAPIInstance.cs (1)
ChangeQuery(55-58)Flow.Launcher/App.xaml.cs (2)
App(28-339)App(50-114)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
ChangeQuery(22-25)Flow.Launcher.Infrastructure/Win32Helper.cs (2)
Win32Helper(19-491)DWMSetCloakForWindow(30-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (10)
Flow.Launcher/MainWindow.xaml.cs (5)
768-773: Animation state simplification implemented correctlyThe code now directly sets the opacity of UI elements based on the animation settings rather than using the old animation state tracking mechanism. This simplifies the animation flow and addresses the issue with animation ceasing to function when typing a query.
282-285: Proper initialization of icon statesSetting both ClockPanel and SearchIcon opacity to 0 here ensures that they start in a consistent hidden state before the animation or visibility logic takes effect, which helps prevent the icon overlapping issues mentioned in the PR objectives.
804-804: Good utility function for opacity retrievalThis helper function cleanly extracts opacity values from styles, supporting the more dynamic approach to UI element visibility management implemented in this PR.
815-815: Good utility function for margin retrievalSimilar to the opacity helper, this function cleanly extracts thickness values from styles, making the animation code more maintainable.
286-288: Clear comment explaining animation behaviorThis comment provides important context about preventing extra hide calls when the animator is active, which would otherwise cause toggling issues.
Flow.Launcher/ViewModel/MainViewModel.cs (5)
633-660: Properly implemented asynchronous query handlingConverting the query handling to an asynchronous pattern is a good improvement that will make the UI more responsive. Using
Dispatcher.InvokeAsyncensures that UI updates happen on the UI thread without blocking.The implementation correctly maintains backward compatibility by having a synchronous method that calls the async version.
1028-1032: Async pattern consistently applied to Query methodThe Query method now properly delegates to QueryAsync, following the same pattern as ChangeQueryText. This ensures consistency in how asynchronous operations are handled throughout the codebase.
1457-1478: Clock and search icon visibility logic properly implementedThis section addresses the core issues mentioned in the PR objectives:
- The clock panel visibility is now correctly tied to whether query text is present
- The search icon visibility is properly controlled based on whether a plugin icon is present
These changes effectively prevent the overlapping issues described in the PR objectives.
1512-1514: Async handling properly implemented for Hide() methodThe Hide method now correctly awaits the ChangeQueryTextAsync method when in Empty mode, ensuring that UI updates are handled asynchronously and preventing potential race conditions.
1532-1549: Proper window hiding with DWM cloakThe use of DWM cloak to hide the window is a good approach as it provides a clean way to completely hide the window without affecting its state.
The order of operations is correct:
- First reset the UI elements
- Then apply the DWM cloak to hide the window
What's the PR
Problem
Currently, there are the following issues in the dev branch:
Fixes
Test Cases
ETC
With this change, modes where the query remains will no longer show the clock by default. I considered various approaches—like showing the clock while adjusting the length of QuerySuggestionText to fit the clock area—but ultimately, I think this is the right solution.