-
-
Notifications
You must be signed in to change notification settings - Fork 448
Enable reordering of websearches with drap and drop #4038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds drag-and-drop reordering to the WebSearch settings ListView. XAML enables 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 movement threshold & identify source item
CodeBehind->>ListView: DoDragDrop(sourceItem, Move)
User->>ListView: Drop on target position
ListView->>CodeBehind: Drop
CodeBehind->>Collection: Remove source item
CodeBehind->>Collection: Insert source 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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月13日T06: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月13日T06: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Please resolve reviews from coderabbitai[bot] and Copilot
not entirely resolved as both tackled the casting issue in a different way, I did the one I'd prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs
[warning] 220-220:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 215-215:
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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (1)
176-195: LGTM!The drag initiation logic correctly:
- Checks the minimum drag distance threshold
- Locates the source ListViewItem and SearchSource
- Includes proper null checks before initiating the drag operation
Optionally, you could modernize lines 185-186 with pattern matching for consistency with newer C# idioms:
- var listView = (ListView)sender; - ListViewItem listViewItem = FindAncestor<ListViewItem>((DependencyObject)e.OriginalSource); + if (sender is not ListView listView) return; + if (FindAncestor<ListViewItem>((DependencyObject)e.OriginalSource) is not ListViewItem listViewItem) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs
[warning] 220-220:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 217-217:
Idx is not a recognized word. (unrecognized-spelling)
[warning] 215-215:
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). (4)
- GitHub Check: build
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (4)
1-7: LGTM!The added using directives are appropriate for the drag-and-drop implementation (Math, WPF types, input events, and VisualTreeHelper).
18-18: LGTM!The private field correctly tracks the initial mouse position for the drag threshold calculation.
171-174: LGTM!Correctly captures the drag start point when the left mouse button is pressed.
224-242: LGTM!Both helper methods correctly implement visual tree traversal to locate the appropriate containers and ancestors for the drag-and-drop operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work for me. Could you please give me a demo video?
dragdropsample.mp4
Just hold and drag with left mouse, works on both win 10 and 11 for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checked and dragdrop can work on my device. But two features seems to be missing:
- When dragging items, there is no visual effect between dropped place.
- When dragging items out of the window, the scroll viewer will not automatically scroll. (It can bring convenience for user with small window size)
Would you mind if you can rewrite the current feature with this package?
https://github.com/punker76/gong-wpf-dragdrop
From my previous development experience with this package, it can bring these two features.
If you want to leave that improvement for the future that is fine, we can just merge this PR - it works great overall.
Yep I thought doing this just for eye candy is a bit overkill and didn't want to add any external dependencies for just that, eye candy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!👍
Closes #3548
Might be worth to consider to also add a feature to reorder with select and arrow up/down buttons