Feature: Enable drag and drop reordering for Pinned Sidebar items#18320
Feature: Enable drag and drop reordering for Pinned Sidebar items#18320kalmix wants to merge 14 commits into
Conversation
abd09bc to
7456ba8
Compare
|
@kalmix I haven't reviewed the code yet, but I tested the changes. It works pretty well but I noticed that I can't drag Recycle Bin to a different position. I'm not sure if this is specific to Recycle Bin or if it's because it's the last item in the list. As a side note, can you remove the old content dialog since it's no longer needed? |
|
@yair100 Sorry, I missed |
176a3c0 to
fff0cd5
Compare
|
I've also updated the "drop on center" behavior to better align with the Explorer. For pinned items, dropping on the center now performs a Create Link action instead of a move operation, while non-pinned items continue to be moved as before. In addition, I added support for drag-and-drop pinning. |
|
Seems to be no way to drag an item to the first position. I have to drag the item to the second from top and then the first one down. Also has issues keeping Gallery pined when the list is updated |
…ss sync - Fix sidebar drag-to-top missing item UI virtualization bug by replacing .Move() with a data reset Manager_DataChanged, which allows drag reordering to the very first position. - Resolve Gallery unpinning failures by explicitly normalizing Windows Virtual Folder PIDL paths (stripping \\SHELL\) and implementing a fallback remove COM verb for virtual shell folders that ignore the standard unpinfromhome verb. - Eliminate race conditions over Quick Access pinning by introducing a FileSystemWatcher in WaitUntilAsync on the OS automaticDestinations-ms registry. - Update LocationItem to correctly display friendly hover tooltips for virtual folders instead of exposing their raw path.
|
@Josh65-2201 Yes, sorry I blocked users from draging to the top because I was running into a UI virtualization bug that left a blank space and also because the file explorer doesn't allow you to do that either but now i've fixed it using Regarding the Gallery issue, thanks for pointing that out, in this case Gallery is an outlier that also causes problem with the current state of Files (try it on the prod app), I gave my best shot at fixing this by normalizing the PIDL paths and also adding a fallback remove COM verb and then to keep everything in sync I added a FileSystemWatcher that tracks the Quick Access registry. |
|
Working perfectly now. |
marcelwgn
left a comment
There was a problem hiding this comment.
As a general thought, why are we now introducing a bunch of try catches for functions that should REALLY not throw and why not leverage the SafetyExtensions for this?
marcelwgn
left a comment
There was a problem hiding this comment.
I think we should only try/catch what we expect to fail, a lot of the try catches catch a more than is necessary/expected to throw, e.g. SideBarItem.cs:440 or SideBarItem.cs:499.
Also regarding all of these try catches, what is the need for the try catches based on? I couldnt find any documentation saying that those are expected to throw and the fact that you introduced them now in a lot of places that we didnt have them before is a bit suspicious to me.
Adding to that, what do you mean by this part in the PR description:
Replaced the previous app-level COM exception workaround [...]
@marcelwgn You make a very fair point. I just pushed a new commit that completely cleans up all of those broad try-catches. To answer your question on why those exceptions were happening at all, during drag-and-drop operations (especially when reordering pinned items while the background Quick Access syncs are running) Windows occasionally invalidates the OLE drag payload mid-flight. If the UI tries to read a property like You were totally right that catching broad Exceptions or wrapping huge blocks of code was the wrong approach. I've switched everything over to use the existing Let me know what you think of the latest commit. |
marcelwgn
left a comment
There was a problem hiding this comment.
Sorry for the delay, changes look good to me. Thank you for your help with this!
|
I noticed the dragging a file to the pinned section shows the reorder visual. This should probably be hidden when dragging files. |
|
Noted. Working on it. |
b917b6f to
1a5d615
Compare
1a5d615 to
1a16f57
Compare
|
Thanks! I noticed another issue where reordering items sometimes shifts existing sidebar items to the right. |
|
Not sure if there intended but dragging a pin over another pin will make a shortcut in the hovered folder or open it |
|
@Josh65-2201 that isn't intended. |
@yair100 Could I get steps to reproduce this issue? |
@yair100 @Josh65-2201 I intentionally set it to create a shortcut because it's the safest UX. Before this PR, accidentally dropping a pin onto another pin triggered a Move operation, which can trigger a lot of accidental move operations for example of a drive to another... and it's a much less common use case. |
We can have a discussion about this behavior in general but it's out of scope for this PR. |
Noted. Will revert those changes. |
|
Thanks! |
0x5bfa
left a comment
There was a problem hiding this comment.
Drag and drop stuff should be similar implementation as the WinUI one. We dont want to handle drop validation inside a control. Have to keep in mind that these controls have to be standalone and any of functionalitis of them have to be able to be used in any scenario.
When you try to drag an item but drag back to the original position, it shows a dialog saying this. This can be a bad UX.
| /// <summary> | ||
| /// Indicates whether the item supports reorder dropping. | ||
| /// </summary> | ||
| bool IsReorderDropItem => false; |
There was a problem hiding this comment.
@yair100 This is the kind of behavior I was trying to avoid with Center Drop => Create Shortcut.
| } | ||
|
|
||
| private static bool IsValidDropPath(string? path) | ||
| => path is not null && (System.IO.Path.IsPathRooted(path) || path.StartsWith("Shell:", StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
Please do not put system specific code inside Files.App.Controls. It should be done within Files.App. Sidebar reordering should work in any other scenario.
There was a problem hiding this comment.
@0x5bfa Thanks for the review, I will work on it this week.
Resolved / Related Issues
Code Changes
ReorderSidebarItemsDialogentirely and replaced it with native inline drag-and-drop directly within the sidebar.SidebarItemUI and logic to calculate insertion positions (top, bottom, or center) and display the appropriate visual feedback when hovering during a drag operation.FileSystemWatcherinWindowsQuickAccessServiceto monitor the hiddenautomaticDestinations-msfile. This guarantees that the Windows Shell has finished asynchronously saving the new pin order before the app triggers a UI refresh, preventing snapping and stuttering.QuickAccessWidgetViewModelto useItems.Move()when diffing and updating reordered items, rather than destroying and rebuilding the UI collection. This eliminates screen flickering during reorders.SafetyExtensions.IgnoreExceptionshelper, strictly targetingCOMExceptionsto ensure the app doesn't crash during drag race conditions.Before:
before.mp4
After:
after.mp4
Steps used to test these changes
COMExceptionand no longer crashes