Support Preview Panel for History Results #3370
Conversation
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.
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.
This comment has been minimized.
This comment has been minimized.
Jack251970
added
enhancement
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds 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:
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. |
📝 WalkthroughWalkthroughThe pull request introduces multiple enhancements and refactorings. A new constant ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MainWindow (UI)
participant VM as MainViewModel
participant DataBinding as DataContext
UI->>VM: OnKeyDown(event)
VM->>VM: QueryResultsSelected()
alt Query results are selected
VM->>VM: UpdatePreviewAsync()
VM->>VM: ShowPreviewAsync()
VM->>DataBinding: Update PreviewSelectedItem
else Alternate key action
VM->>VM: Execute Escape/Backspace command
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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 (9)
Flow.Launcher/ViewModel/MainViewModel.cs (9)
52-53: Consider usingArray.Empty<Result>()for_emptyResult.Storing an empty
List<Result>is fine, but usingArray.Empty<Result>()avoids extra allocations and may simplify code.-private readonly IReadOnlyList<Result> _emptyResult = new List<Result>(); +private readonly IReadOnlyList<Result> _emptyResult = Array.Empty<Result>();
167-173: Async call in property change without error handling.When the
Results.SelectedItemchanges,_ = UpdatePreviewAsync()is invoked. Consider capturing the returned task, adding logging, or using a safe fire-and-forget pattern to handle possible exceptions instead of discarding them.- _ = UpdatePreviewAsync(); + var previewTask = UpdatePreviewAsync(); // e.g. add logging or .ContinueWith(...) to handle errors
174-181: Mirroring logic forHistory.PropertyChanged.Here as well,
_ = UpdatePreviewAsync()is used. Apply similar exception handling measures as recommended forResultschanges to maintain consistent error handling across the codebase.
874-902:ShowPreviewAsync()method introduced for asynchronous handling.Transitioning from sync to async improves responsiveness. However, consider capturing the task returned by
OpenExternalPreviewAsyncor using a safe pattern to handle failures (instead of_ =).
979-979: Switching external preview asynchronously.Again, consider storing the task or handling errors from
_ = SwitchExternalPreviewAsync(...).
984-984: Open external preview fromHideInternalPreview.Double-check that external preview’s success/failure is logged or handled to prevent silent failures.
997-997: Load the preview image if internal preview is visible.Straightforward approach. Confirm performance if preview images can be large or frequently changed.
1468-1468: AwaitingCloseExternalPreviewAsync()inHide().Prevents leaving an external preview open. This is good. Consider a short comment or log if the method fails.
1481-1481: Force UI update after resetting query text.Explicitly calling
UpdateLayout()is occasionally brittle, but if needed for immediate refresh, this is acceptable. Review possible performance impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Infrastructure/Constant.cs(1 hunks)Flow.Launcher/MainWindow.xaml(5 hunks)Flow.Launcher/MainWindow.xaml.cs(3 hunks)Flow.Launcher/Themes/Base.xaml(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(25 hunks)Flow.Launcher/ViewModel/ResultViewModel.cs(9 hunks)Flow.Launcher/ViewModel/ResultsViewModel.cs(7 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
Flow.Launcher/MainWindow.xaml.cs (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
QueryResultsSelected(1386-1390)
Flow.Launcher/ViewModel/ResultViewModel.cs (1)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
Result(1318-1356)Result(1358-1384)
Flow.Launcher/ViewModel/MainViewModel.cs (5)
Flow.Launcher/PublicAPIInstance.cs (6)
List(160-160)Task(94-94)Task(165-165)Task(167-168)Task(170-171)Task(346-346)Flow.Launcher/ViewModel/ResultsViewModel.cs (2)
List(218-229)List(231-240)Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs (1)
List(146-149)Flow.Launcher/ViewModel/ResultViewModel.cs (2)
ResultViewModel(15-274)ResultViewModel(20-62)Flow.Launcher/MainWindow.xaml.cs (3)
Task(580-587)MainWindow(30-998)MainWindow(65-77)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (33)
Flow.Launcher/Themes/Base.xaml (1)
453-454: Good fix for the preview panel visibility when viewing history items.This change properly activates the condition that checks the History element's visibility, moving it outside the comment block. Now when history items are selected, the preview panel height won't be incorrectly set to 0, which directly addresses the first goal mentioned in the PR objectives.
Flow.Launcher/ViewModel/ResultsViewModel.cs (3)
11-12: Improved code organization.The reorganization of using directives enhances code organization by grouping related imports together.
31-31: Improved code readability.The addition of blank lines between logical sections improves code readability by visually separating related code blocks.
Also applies to: 241-241, 272-272, 300-300, 312-312
263-263: Modern C# pattern matching usage.Using pattern matching with
is notoperator is more concise and less error-prone than a separate cast and null check.Flow.Launcher/ViewModel/ResultViewModel.cs (6)
17-18: Improved naming conventions and immutability.Good improvement using PascalCase for static readonly fields and adding the
readonlymodifier to ensure collection immutability.
143-147: Consistent private field naming convention.The renaming of private fields to use the underscore prefix (
_imageLoaded,_previewImageLoaded, etc.) improves code consistency and follows standard C# naming conventions.
24-25: Simplified control flow.Removing unnecessary braces and whitespace makes the code more concise while maintaining readability.
Also applies to: 98-99, 108-109
40-45: More efficient dictionary lookup.Using
TryGetValueinstead of checking for key existence followed by accessing the value is more efficient as it performs a single dictionary lookup instead of two.
164-177: Robust preview image handling.The expanded
PreviewImageproperty now properly checks if the preview image is loaded before triggering an asynchronous load, which addresses the issue with preview images for history items not loading correctly.
237-241: Improved condition check for preview image loading.The combined condition now properly checks if the default preview is visible, if the preview image hasn't been loaded yet, and if the preview image should be shown, which helps fix the issue with preview images for history items.
Flow.Launcher.Infrastructure/Constant.cs (1)
35-35: Added resource path for history icon.Adding the
HistoryIconconstant provides a centralized reference to the history icon image, supporting the display of history items in the preview panel.Flow.Launcher/MainWindow.xaml.cs (1)
304-312: Improved method naming for clarity.Renaming
SelectedIsFromQueryResults()toQueryResultsSelected()makes the code more readable and better describes the method's purpose: checking if the currently selected items are from query results rather than history results.This change, combined with the other modifications, helps address the issue where the preview panel height was set to 0 when selected items were from history by making the distinction between query results and history items clearer.
Also applies to: 314-320, 324-336
Flow.Launcher/MainWindow.xaml (5)
218-218: Confirm consistent naming conventions for new Grid.Good practice to name this grid for reference, but please ensure that it aligns with existing naming patterns and style guidelines in the project (e.g., camelCase vs. PascalCase, suffix “Area” for Grids, etc.).
341-341: Grid naming forMiddleSeparatorArealooks consistent.The new x:Name property improves clarity and likely supports referencing in code-behind or triggers. No concerns with this approach.
381-381:ResultPreviewAreanaming is clear and descriptive.Naming the border makes referencing the preview area straightforward. Looks good.
444-444: DataContext bound toPreviewSelectedItemin internal preview area.Switching from
SelectedItemtoPreviewSelectedItem(with one-way binding) makes sense for a read-only preview context. Ensure any necessary updates toPreviewSelectedItemare triggered in your ViewModel so the UI refreshes properly.
521-521: Second DataContext change toPreviewSelectedItem.Same rationale as above. One-way binding is appropriate if the preview is read-only. No issues here.
Flow.Launcher/ViewModel/MainViewModel.cs (16)
284-284: LoadHistory usesif (QueryResultsSelected()).This condition likely toggles to history view only from query results. The logic looks correct, but verify if the intent is to allow flipping from other states (like context menu) as well.
298-298: Only re-query when in query results.Ensures that pressing ReQuery does not interfere when in history or context menu states. This is consistent, but confirm no user scenarios are missed if called from other states.
369-369: Autocomplete query restricted to query results.
if (result != null && QueryResultsSelected())is a fair approach to avoid autocompletion in other views. No issues here.
421-421: Storing user selection only when query results are selected.Updates
_userSelectedRecordand_historyonly if in query results. This maintains logical consistency, preventing polluting the record with context menu selections. LGTM.
500-500:SelectPrevItemalso checksQueryResultsSelected().Ensures that pressing a navigation hotkey does not incorrectly manipulate the history state. Looks consistent with your new logic.
520-520: ESC action: fallback to query results if not currently there.This is consistent with typical WPF flow: returning to main results or hiding the window. No concerns.
532-532:BackToQueryResultsmethod checksQueryResultsSelected().Ensures we only toggle back if we're not already in query results. Straightforward logic.
834-836: Introduce constants for preview area columns.
ResultAreaColumnPreviewShown= 1,ResultAreaColumnPreviewHidden= 3. This clarifies the layout logic. Good practice to use named constants.
837-849: New_selectedItemFromQueryResultstracking &PreviewSelectedItem.Storing a nullable bool to track whether the selection originates from query results is helpful for UI logic.
PreviewSelectedItemproperty looks properly implemented withOnPropertyChanged().
893-894: Close external preview then show internal preview.Sequential calls are correct, but verify that calling
CloseExternalPreviewAsyncback-to-back withShowInternalPreviewdoes not cause flicker or concurrency issues.
991-991: Close external preview if it’s visible.Ensure cancellation tokens or re-entrant calls won’t cause race conditions here.
1004-1004: UseQueryResultsPreviewed()to select the correct file path.This logic is simple and direct. Verify that other states (e.g. from history or context menu) don’t incorrectly remain empty.
1008-1012:QueryResultsPreviewed()method clarifies the preview origin condition.This is a clear helper to ensure the currently previewed item is from
Results.SelectedItem.
1019-1019: Include re-query only if we’re in query results.Consistent with your updated architecture. No blockers.
1420-1426:ShouldIgnoreHotkeysmethod introduced.Checks full-screen or game mode. This is a neat way to unify condition checks. Ensure logic is tested on multi-monitor setups where foreground app detection might vary.
1471-1471: Fallback to query results before hiding.Ensures consistency in final UI state.
|
Looks cool, but I am wondering why is this needed? |
Since context menu will not hide preview panel, we should also provide preview panel for history records |
|
Or you will get this if you enable preview panel: |
|
I see, better than empty |
It's a bit different. ContextMenu's results are provided by plugins, which may include a special preview (I don't know whether anyone do that though). |
@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 ...
|
jjw24
added
bug
Support Preview Panel for History Results
TODO