Enable reordering of websearches with drap and drop #4038
Conversation
|
🥷 Code experts: Jack251970, onesounds Jack251970 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds drag-and-drop reordering to the WebSearch settings ListView. XAML enables drag/drop and wires PreviewMouseLeftButtonDown, PreviewMouseMove, and Drop events. Code-behind implements drag initiation, movement threshold detection, drop handling to reorder items, and helpers to resolve item containers and traverse the visual tree. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ListView as Settings ListView
participant CodeBehind as SettingsControl.xaml.cs
participant Collection as ItemsSource
User->>ListView: MouseLeftButtonDown (on item)
ListView->>CodeBehind: PreviewMouseLeftButtonDown
CodeBehind->>CodeBehind: Record drag start point
User->>ListView: Mouse move with button pressed
ListView->>CodeBehind: PreviewMouseMove
CodeBehind->>CodeBehind: Check threshold and source item
CodeBehind->>ListView: DoDragDrop(start item, Move)
User->>ListView: Drop on target position
ListView->>CodeBehind: Drop
CodeBehind->>Collection: Remove source item
CodeBehind->>Collection: Insert at target index
CodeBehind->>ListView: Update selection/visuals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
![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
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (2)
176-195: Consider removing the unnecessary cast.The drag initiation logic is correct. However, line 185 unnecessarily casts
sendertoListViewwhen the parameter type already guarantees it.Apply this diff to remove the cast:
- ListView listView = sender as ListView; + var listView = (ListView)sender;Or simply use
senderdirectly if type safety is clear from context.
211-217: Consider using full variable names for improved readability.The abbreviations
removedIdxandtargetIdxare functional but could be more readable asremovedIndexandtargetIndex. This also addresses the spelling warnings from static analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T06:04:26.977Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 214-214:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 214-214:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 212-212:
Idx is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (4)
18-18: LGTM!The
_dragStartPointfield correctly tracks the initial mouse position for drag threshold detection.
171-174: LGTM!The handler correctly captures the mouse position when the left button is pressed, which is then used to calculate the drag threshold in
ListView_PreviewMouseMove.
222-229: LGTM!The visual tree traversal logic correctly locates the nearest
ListViewItemcontainer.
231-240: LGTM!The generic ancestor finder is a standard WPF pattern and correctly traverses the visual tree.
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml (1)
55-58: LGTM!The XAML changes correctly wire up the drag-and-drop functionality by adding the necessary event handlers and enabling drop support on the ListView.
| private void ListView_Drop(object sender, DragEventArgs e) | ||
| { | ||
| if (e.Data.GetDataPresent(typeof(SearchSource))) | ||
| { | ||
| SearchSource droppedData = e.Data.GetData(typeof(SearchSource)) as SearchSource; | ||
| ListView listView = sender as ListView; | ||
| var target = GetNearestContainer(e.OriginalSource); | ||
|
|
||
| if (target == null) | ||
| return; | ||
|
|
||
| SearchSource targetData = (SearchSource)listView.ItemContainerGenerator.ItemFromContainer(target); | ||
|
|
||
| var items = _settings.SearchSources; | ||
| int removedIdx = items.IndexOf(droppedData); | ||
| int targetIdx = items.IndexOf(targetData); | ||
|
|
||
| if (removedIdx == targetIdx) | ||
| return; | ||
|
|
||
| items.RemoveAt(removedIdx); | ||
| items.Insert(targetIdx, droppedData); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check for targetData to prevent NullReferenceException.
Line 208 retrieves targetData from the item container, but there's no validation before using it on line 212. If ItemContainerGenerator.ItemFromContainer returns null, the subsequent IndexOf call will throw a NullReferenceException.
Apply this diff to add the null check:
SearchSource targetData = (SearchSource)listView.ItemContainerGenerator.ItemFromContainer(target);
+
+ if (targetData == null)
+ return;
var items = _settings.SearchSources;Additionally, line 202 has an unnecessary cast (similar to line 185):
- ListView listView = sender as ListView;
+ var listView = (ListView)sender;🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 214-214:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 214-214:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 212-212:
Idx is not a recognized word. (unrecognized-spelling)
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs around lines
197 to 220, the code obtains targetData via
ItemContainerGenerator.ItemFromContainer but does not check for null before
using it, which can cause a NullReferenceException; add a null check after
retrieving targetData and return early if it's null (so subsequent IndexOf calls
are safe). Also remove the unnecessary cast at line 202 (the redundant "as
ListView" or similar) to simplify the code.
There was a problem hiding this comment.
Pull Request Overview
This PR implements drag and drop functionality to enable reordering of web search entries in the Flow Launcher Plugin WebSearch settings interface.
- Added drag and drop event handlers to the ListView control
- Implemented mouse tracking and visual tree helper methods to support drag operations
- Added necessary using statements for drag and drop functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| SettingsControl.xaml.cs | Added drag and drop implementation with mouse event handlers and helper methods |
| SettingsControl.xaml | Added event handler bindings and AllowDrop property to ListView |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return; | ||
|
|
||
| items.RemoveAt(removedIdx); | ||
| items.Insert(targetIdx, droppedData); |
There was a problem hiding this comment.
The insertion logic is incorrect when dragging items downward. When an item is removed from an earlier position and inserted at a later position, the target index should be adjusted by -1 to account for the removal. Consider using items.Insert(removedIdx < targetIdx ? targetIdx - 1 : targetIdx, droppedData);
| items.Insert(targetIdx, droppedData); | |
| items.Insert(removedIdx < targetIdx ? targetIdx - 1 : targetIdx, droppedData); |
Copilot uses AI. Check for mistakes.
| ListView listView = sender as ListView; | ||
| ListViewItem listViewItem = FindAncestor<ListViewItem>((DependencyObject)e.OriginalSource); |
There was a problem hiding this comment.
[nitpick] Use pattern matching instead of 'as' operator for better null safety and readability: if (sender is not ListView listView) return; and similar for listViewItem.
Copilot uses AI. Check for mistakes.
| SearchSource droppedData = e.Data.GetData(typeof(SearchSource)) as SearchSource; | ||
| ListView listView = sender as ListView; |
There was a problem hiding this comment.
[nitpick] Use pattern matching instead of 'as' operator for better null safety: if (e.Data.GetData(typeof(SearchSource)) is not SearchSource droppedData) return; and if (sender is not ListView listView) return;
Copilot uses AI. Check for mistakes.
|
Please resolve reviews from coderabbitai[bot] and Copilot |
Closes #3548
Might be worth to consider to also add a feature to reorder with select and arrow up/down buttons