Fix drag mechanism in Plugin Management Menu#894
Conversation
Convert `WindowUtility` to partial and add file drag-and-drop handling. - Added `internal FileDropEvent` and `SetFileDropEnabled` to toggle `DROPFILES`/`COPYDATA`/`COPYGLOBALDATA` filters and call `DragAcceptFiles`. - Implemented `TryGetExternalDragPosition` to read external drag cursor position. - `WndProc` now handles `WM_DROPFILES` and forwards to `HandleFileDrop`. - HandleFileDrop enumerates dropped files (`DragQueryFile`/`DragQueryPoint`) and invokes `FileDropEvent`, then cleans up with `DragFinish`. - Added `NativeFileDrop` partial class with `P/Invoke` bindings and a `NativePoint` struct.
- Enable WinUI and native file drag-and-drop support for the Plugin Manager - Add drag enter/over/leave/drop handlers - Add a `DispatcherTimer` to track external drag position, and a visual drop indicator. - Wire `WindowUtility.FileDropEvent` on page load/unload and gate drops to the import area. - Extract import flow into an async `ImportPlugins` method and improve error handling around imports.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| private async void OnNativeFileDrop(string[] selectedFiles, PointInt32 dropPoint) | ||
| { | ||
| if (!IsPointInDropArea(dropPoint)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await ImportPlugins(selectedFiles); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- PluginImporter: support importing `.zip` packages or `manifest.json`, validate filenames, copy assets to a staging directory and atomically move into place, protect against directory-traversal by resolving contained paths (`GetContainedPath`), and ensure cleanup on failure. Uses stream-based copy for assets. - PluginManagerPage: fixes drag indicator state, collects per-file failures instead of throwing `AggregateException`, logs errors, and shows a friendly dialog mapping common exceptions to readable messages.
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: The async void method OnNativeFileDrop lacks a try-catch block when calling await ImportPlugins, which can lead to an unhandled exception and crash the application.
Severity: HIGH
Suggested Fix
Wrap the await ImportPlugins(selectedFiles) call inside the OnNativeFileDrop method with a try-catch block, similar to the implementation in OnClickImportButton and OnDropImportBox. This will catch exceptions and prevent the application from crashing.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: CollapseLauncher/XAMLs/MainApp/Pages/PluginManagerPage.xaml.cs#L179
Potential issue: The `OnNativeFileDrop` event handler is an `async void` method that
calls `await ImportPlugins(selectedFiles)` without any exception handling. The
`ImportPlugins` method can throw an exception, for example, when its call to
`SimpleDialogs.SpawnDialog` fails. Unlike other event handlers in the same file,
`OnClickImportButton` and `OnDropImportBox`, which wrap the same call in a `try-catch`
block, `OnNativeFileDrop` lacks this protection. An unhandled exception in an `async
void` method will propagate to the `SynchronizationContext` and crash the application.
This can be triggered by a native file drop if the dialog fails to spawn due to UI state
issues or other errors.
There was a problem hiding this comment.
Need fix just in case UI thread being poo poo and the first two setter drops itself
bagusnl
left a comment
bagusnl
left a comment
There was a problem hiding this comment.
Some changes are needed especially on the async void error handling and some localizations to user facing errors
|
|
||
| NativeFileDrop.DragQueryPoint(dropHandle, out NativePoint dropPoint); | ||
| FileDropEvent?.Invoke(files, new PointInt32(dropPoint.X, dropPoint.Y)); | ||
| } |
There was a problem hiding this comment.
Might want to catch any error during the file drop just in case so that
- User knows (error message or dialog, telling them to select a file instead using the old mechanism)
- We knows (Sentry capture)
|
|
||
| private static unsafe partial class NativeFileDrop | ||
| { | ||
| [LibraryImport("shell32.dll")] |
There was a problem hiding this comment.
It will be much appreciated if all of the static imports is moved towards the submodule Hi3Helper.Win32/Native so we have less unsafe method in the main project
| [StructLayout(LayoutKind.Sequential)] | ||
| private struct NativePoint | ||
| { | ||
| internal int X; |
There was a problem hiding this comment.
By MSFT documentation, the type for tagPOINT is long, is there a specific reason why we use int instead?
https://learn.microsoft.com/en-us/windows/win32/api/windef/ns-windef-point
| } | ||
| catch (Exception ex) | ||
| { | ||
| ErrorSender.SendException(ex.WrapPluginException("No plugin has been imported due to following error:")); |
There was a problem hiding this comment.
Consider localizing error message as it's sent to the user facing dialog
| } | ||
| catch (Exception ex) | ||
| { | ||
| ErrorSender.SendException(ex.WrapPluginException("No plugin has been imported due to following error:")); |
There was a problem hiding this comment.
Same as above under ErrorSender
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Need fix just in case UI thread being poo poo and the first two setter drops itself
| { | ||
| NotSupportedException => exception.Message, | ||
| DuplicateNameException => exception.Message, | ||
| UnauthorizedAccessException => "Permission was denied while reading or installing the plugin.", |
| "RightPanelImportTitle2": "or", | ||
| "RightPanelImportTitle3": "here", | ||
| "RightPanelImportTitle4": "Anywhere in the box is fine!" | ||
| "RightPanelImportTitle1": "Browse or drop a", |
There was a problem hiding this comment.
"Click to browse or drop a" seems better message if the UI fits
2 participants
Main Goal
Add drag-and-drop plugin importing to
PluginManagerPage, including support for dragging files from unelevated Windows Explorer into Collapse’s elevated process, while retaining the existing file-picker workflow.PR Status :
Changelog
[New] Added drag-and-drop plugin importing to
PluginManagerPage.ImportBoxButtonthroughAllowDrop="True"and the following XAML handlers:OnDragEnterImportBoxOnDragOverImportBoxOnDragLeaveImportBoxOnDropImportBoxOnDropImportBoxretrievesStorageFileobjects fromDragEventArgs.DataView.GetStorageItemsAsync()and converts them into file-system paths..zippackages andmanifest.jsonfiles are passed to the sameImportPluginsmethod used by the existing file picker.ImportPluginscallsPluginImporter.AutoGetImportFromPathfor each path and adds successfully importedPluginInfoobjects toPluginManagerPage.Context.PluginCollection.AggregateExceptionandErrorSender.SendException.ImportPluginsis running, preventing overlapping imports.[New] Added elevated-process file-drop support to
WindowUtility.requireAdministrator, which prevents normal Explorer drag events from reaching the WinUI drop target because Explorer usually runs at a lower integrity level.WindowUtility.SetFileDropEnabledto register the main window with the nativeDragAcceptFilesAPI.SetFileDropEnabledusesChangeWindowMessageFilterExto permit the native messages required for cross-integrity shell drops:WM_DROPFILESWM_COPYDATAWM_COPYGLOBALDATAWindowUtility.MainWndProcto processWM_DROPFILESand forward itsHDROPhandle toHandleFileDrop.HandleFileDropuses:DragQueryFileto retrieve every dropped path.DragQueryPointto retrieve the drop coordinates.DragFinishto release the nativeHDROPhandle.WindowUtility.FileDropEvent, which passes the collected paths and drop coordinates toPluginManagerPage.OnNativeFileDrop.PluginManagerPage.OnPluginManagerPageLoadedenables native file drops and subscribes toFileDropEvent.OnPluginManagerPageUnloadeddisables native file drops and removes the event subscription.OnNativeFileDropcallsIsPointInDropAreaso native drops are accepted only when they occur insideImportBoxButton.[Imp] Refactored the existing plugin import workflow.
OnClickImportButtonremains responsible for openingFileDialogNative.GetMultiFilePicker.ImportPlugins.OnClickImportButtonandOnDropImportBoxnow callImportPlugins, keeping validation, collection updates, partial-success handling, and error reporting consistent between both input methods..zipandmanifest.json.[Imp] Added drag-hover feedback for both WinUI and elevated Explorer drops.
Added the
ImportDropIndicatoroverlay directly above the plugin import panel.The overlay contains:
RectanglewithStrokeDashArray="1,2"andStrokeDashCap="Round".AccentColortheme resource.AccentFillColorDefaultBrush.Added a
ScalarTransitionwith a duration of 180 ms to fade the indicator in and out.SetImportDropIndicatortracks the current visibility state and changes the overlay opacity only when necessary.Standard WinUI drag events update
_isWinUiFileDragActiveand callSetImportDropIndicator.Native
WM_DROPFILESdoes not provide drag-enter or drag-leave notifications, soPluginManagerPageuses_fileDragIndicatorTimerto detect elevated Explorer hover state.OnFileDragIndicatorTimerTickcallsWindowUtility.TryGetExternalDragPositionevery 50 ms.TryGetExternalDragPositionuses:GetAsyncKeyStateto detect left- or right-button dragging.GetCursorPosto obtain the global pointer position.ScreenToClientto convert it into launcher client coordinates.GetForegroundWindowto avoid treating normal clicks inside Collapse as external drags.IsPointInDropAreacompares the native pointer coordinates against the transformed bounds ofImportBoxButton, including the current monitor scale factor.[Loc] Updated the English plugin-import instructions.
Templates
Changelog Prefixes