- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.5k
 
Fix/drag and drop on linux #19232
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
base: master
Are you sure you want to change the base?
Fix/drag and drop on linux #19232
Conversation
| 
           You can test this PR using the following package version.   | 
    
          
  | 
    
| 
           @cla-avalonia agree  | 
    
| 
           Thank you for your contribution! :) I haven't reviewed the code yet, but we're currently in the process of refactoring the clipboard and drag and drop architecture. Consequently, your PR will be on hold until that work is completed and will very likely need modifications to accommodate the new types. Edit: clipboard refactoring PR is #19347  | 
    
| 
           @MrJul Ok, but there a no much usage of Avalonia's inner DragNDrop system, mostly implementations of lots and lots of X11 requests  | 
    
| 
           IDataObject can be queried for data at any stage ( DragEnter, DragOver, DragDrop, DragLeave) for any of the supported types, apps are allowed to inspect dragged contents to decide if they support the dragged data. This behavior is actually described by XDND spec too: 
 The PR only loads data for only one random data type after getting the final drop event. So the implementation is incorrect.  | 
    
| 
           @kekekeks Changed it.  | 
    
| 
           You can test this PR using the following package version.   | 
    
          
 This is technically incorrect too. The dragged data could be a 8K image that is presented in multiple compressed and uncompressed formats. Transferring all of that via INCR's window of 16KB will take hours.  | 
    
| 
           e. g. dragging image from firefox produces the following formats:  | 
    
| 
           @kekekeks Changed it.  | 
    
| 
           You can test this PR using the following package version.   | 
    
| 
           I have tried with mp3/wav and gpz guitar profile its working for me dont see any issues .  | 
    
          
 Haven't read the diff, but what you described here is not a correct implementation of IDataObject contract. Have you tried the described usage scenario with inspecting the actual data of dragged image inside of DragMove event? If this is not addressed, we can probably merge the PR for the sake of the most common file list drag-n-drop scenario, but we'll have to disable support for other data formats.  | 
    
| } | ||
| } | ||
| 
               | 
          ||
| if (targetWindow != IntPtr.Zero) | 
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.
From XDND spec:
When the source and target windows are part of the same application, sending X Client Messages is a waste of time and bandwidth, especially if the program and server are on different machines. Implementations should therefore detect the special cases of "source = target" and "source and target in same application" and handle them via function calls.
We should switch to in-process implementation when DnD occurs between app's own windows.
        
          
                src/Avalonia.X11/Avalonia.X11.csproj
              
                Outdated
          
        
      | <TargetFrameworks>$(AvsCurrentTargetFramework);$(AvsLegacyTargetFrameworks);netstandard2.0</TargetFrameworks> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <EnableRuntimeMarshalling>true</EnableRuntimeMarshalling> | ||
| <EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization> | 
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.
No new code should be introduced that uses BinaryFormatter. It has been deprecated from .NET for years as a security hazard and is being migrated out of the framework altogether, hence the need for this flag. In net9.0+, it also requires a package install for it to work at all, and is heavily discouraged from use.
There are some legacy places within Avalonia where it is used; those should be removed for v12 or ASAP. No new code using this should be added. This needs to be replaced.
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.
Ok, but what should I use as alternative? Microsoft recommends JsonSerializator, but it absent in netstandart 2.0.
Or skip this functionality in netstandart 2.0?
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.
@kekekeks what do you think?
netcore includes System.Text.Json. Framework doesn't, and obviously not in netstandard, but that could be brought in as a dependency. Is there other serialization frameworks within Avalonia that could be used here?
I'm open to pulling in System.Text.Json since it's already in the netcore, it would need to be referenced for legacy frameworks.
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.
I don't think we should do any kind of automatic magic serialization.
With #19347 we're handling conversions for known formats, but everything else should be a string or byte[]/Memory<byte>. It's up to the user to properly serialize/deserialize their payload with their preferred format.
The only exception is on Windows which always had the BinaryFormatter in place (for compatibility with WPF/Winforms), but it will be removed for v12.
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.
removed BinaryFormatter
| 
           @kekekeks Sorry, I'm not sure i'm understood what is wrong with IDataObject contract. Could you describe more, which functionality should I add?  | 
    
| 
           Ok, looks like after #19347 some of my problems, such as BinaryFormatter, will be gone)  | 
    
| 
           You can test this PR using the following package version.   | 
    
| 
           @drasticactions , @kekekeks  | 
    
| 
           You can test this PR using the following package version.   | 
    
| 
           @mikekrylov Now that #19347 is merged, could you please update this PR when you have time? You should now implement   | 
    
| 
           @MrJul Thanks! Should only async one? Not need to implementing sync version?  | 
    
| 
           You can test this PR using the following package version.   | 
    
| 
           @MrJul @kekekeks @drasticactions  | 
    
| 
           You can test this PR using the following package version.   | 
    
| 
           You can test this PR using the following package version.   | 
    
What does the pull request do?
Added realisation of XDnD protocol for X11 on Linux.
What is the current behavior?
There are no Drag and Drop between Avalonia window and other apps on Linux systems with X11 and Wayland-X11 mode.
What is the updated/expected behavior with this PR?
Avalonia app can receive dropped uri and text data and initiate drag for those datas on Linux (if under X11 server).
Avalonia app can receive/transmit other X11's mime types, if app's devs support them.
How was the solution implemented (if it's not obvious)?
To X11Window added helper instance of X11DropTarget, which process XDnD messages and request data according specifications. X11DropTarget works as mid-level between X11 and InputRoot system.
To X11Platform added X11DragSource (similar as OLE implementation). On drag X11DragSource creates instance of DragSourceWindow, an invisible window, which process pointer movements and sends XDnD messages to windows under the cursor.
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Fixes #6085