Skip to content
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

Drag drop #1417

Merged
merged 33 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@boombuler
Copy link
Contributor

boombuler commented Mar 3, 2018

Hi,

I've started implementing system wide drag+drop for osx and windows. Please leave some comments!

@boombuler boombuler changed the title [WiP] Drag drop WIP: Drag drop Mar 3, 2018

@danwalmsley danwalmsley requested review from kekekeks and grokys Mar 3, 2018


namespace Avalonia.Controls.DragDrop
{
public sealed class DragDrop : AvaloniaObject

This comment has been minimized.

@kekekeks

kekekeks Mar 3, 2018

Member

It could be a static class. It also doesn't need to inherit from AvaloniaObject.

This comment has been minimized.

@boombuler

boombuler Mar 3, 2018

Author Contributor

Done.

namespace Avalonia.Controls.DragDrop
{
[Flags]
public enum DragOperation

This comment has been minimized.

@kekekeks

kekekeks Mar 3, 2018

Member

Might be worth to rename to DragDropEffects

This comment has been minimized.

@boombuler

boombuler Mar 3, 2018

Author Contributor

Done.


namespace Avalonia.Controls.DragDrop
{
public interface IDragData

This comment has been minimized.

@kekekeks

kekekeks Mar 3, 2018

Member

Might be worth to rename to IDataObject. We should probably also use it from our clipboard later.

This comment has been minimized.

@boombuler

boombuler Mar 3, 2018

Author Contributor

Done.

private Interactive GetTarget(IInputElement root, Point local)
{
var target = root.InputHitTest(local)?.GetSelfAndVisualAncestors()?.OfType<Interactive>()?.FirstOrDefault();
if (target != null && DragDrop.GetAcceptDrag(target))

This comment has been minimized.

@kekekeks

kekekeks Mar 3, 2018

Member

It should traverse the visual tree to check if there is an ancestor element, that accepts drag.

This comment has been minimized.

@boombuler

boombuler Mar 3, 2018

Author Contributor

This is implemented like the behavior of WPF. Since the AcceptDrag property is inherited, all child elements should accept drags if not explicitly disabled, in that case it should not give the drag data to the parent elements. (IMHO)

@@ -144,6 +151,75 @@ public void SetCursor(NSCursor cursor)
UpdateCursor();
}

public override NSDragOperation DraggingEntered(NSDraggingInfo sender)

This comment has been minimized.

@kekekeks

kekekeks Mar 3, 2018

Member

Interacction with the drag dispatcher shouldn't be in a platform-specific implementation. It should raise some events that are then handled by TopLevel.cs. Something like public Action<RawDragEventArgs> OnDrag {get;set;}

boombuler added some commits Mar 3, 2018

Improved DragSource
the drag source now gets the preferred drag effect and handles keyboard
inputs too
@@ -10,6 +10,7 @@ namespace ControlCatalog
{
internal class Program
{
[STAThread]

This comment has been minimized.

@grokys

grokys Mar 17, 2018

Member

Does this not work on .NET core? At the moment our templates assume one can use the same template across .NET Core and .NET Framework.

This comment has been minimized.

@grokys

grokys Mar 17, 2018

Member

This PR makes me think it should work on .NET core too: dotnet/coreclr#15652

This comment has been minimized.

@boombuler

boombuler Mar 17, 2018

Author Contributor

It does not work in the current version but as you mentioned the next release will take the attribute into account. For now only Thread.CurrentThread.TrySetApartmentState(ApartmentState.STA); will help on .net core

This comment has been minimized.

@danwalmsley

danwalmsley Mar 19, 2018

Member

Couldn't this be set inside the appbuilder, so that doesn't have to be marked in user code?

This comment has been minimized.

@boombuler

boombuler Mar 21, 2018

Author Contributor

MSDN says it needs to be done before the thread is started. might be possible but I guess Its to late.

What might be a possibility: Only register the Windows D+D implementation if the current thread is STA. That way "In-Process" D+D would always work and "Cross-Process" D+D would work on OSX even if the user did not set the ApartmentState.

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Mar 19, 2018

I am terribly sorry, hopefully will find time for a proper review this week.

@grokys
Copy link
Member

grokys left a comment

Hi @boombuler - first of all really sorry about the delay in reviewing this. Both @kekekeks and myself have found ourselves with little time recently.

I've taken a look at this and while I'm not the best person to comment on the platform-specific parts, I have a few comments:

  • Drag/drop really needs to be implemented in Avalonia.Input rather than Avalonia.Controls. The only thing that seems to be preventing this is a dependency on TopLevel. To avoid this I'd suggest creating an IInputRoot or IDragDropRoot (not sure which would be best, @kekekeks @jkoritzinsky any feelings?) interface in Avalonia.Input and implementing it on TopLevel
  • I tried getting it to work in ControlCatalog using the following code:
        public BorderPage()
        {
            this.InitializeComponent();
            DragDrop.SetAcceptDrag(this, true);
            AddHandler(DragDrop.DropEvent, Drop);
        }

However I couldn't get it to work. In WPF all I need to do is set AllowDrop = true and add a Drop handler. I assume the equivalent code here would be the above? What did I miss?

  • Related to the above is AcceptDrag the same as WPF's AllowDrop? Why the different name?

Thanks for the work on this, it's really needed, and sorry again about the delay :(


public RawDragEvent(IDragDropDevice inputDevice, RawDragEventType type,
IInputElement inputRoot, Point location, IDataObject data, DragDropEffects effects)
:base(inputDevice, GetTimeStamp())

This comment has been minimized.

@grokys

grokys Apr 10, 2018

Member

This could simply be : base(inputDevice, 0) right?


namespace Avalonia.Controls.DragDrop
{
public class DataObject : IDataObject

This comment has been minimized.

@grokys

grokys Apr 10, 2018

Member

This class doesn't seem to be used anywhere. Am I missing something?

This comment has been minimized.

@boombuler

boombuler Apr 11, 2018

Author Contributor

This class is an implementation which can be used to start dragging.
for example:

DataObject dobj = new DataObject();
dobj.Set(DataFormats.Text, "Foobar");
var dragResult = await DragDrop.DoDragDrop(dobj, DragDropEffects.Copy|DragDropEffects.Move|DragDropEffects.Link);

boombuler added some commits Apr 11, 2018

moved Drag+Drop sources to Avalonia.Input
Also the windows specific DragSource is
only used when the application runs as STA
@boombuler

This comment has been minimized.

Copy link
Contributor Author

boombuler commented Apr 11, 2018

Hi,

@grokys I moved most of the parts to Avalonia.Input. The only thing thats left in Avalonia.Controls is the "Fallback DragSource" which can be used on any platform which does not have a specific implementation but only works "In-Process" and not "Cross-Process"

The Windows specific DragSource is now only registered when the process runs as STA so it will at least work with in-process D+D when the process runs as MTA.

The sample code you have posted for the BorderPage example works fine on my machine. (Win32)
At least when I drag over one of the borders.

If you want I could rename AcceptDrag to AllowDrop I'm fine with both names.

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 12, 2018

The sample code you have posted for the BorderPage example works fine on my machine. (Win32)
At least when I drag over one of the borders.

Ah, you're right, I wasn't dragging over a border, d'oh.

@grokys
Copy link
Member

grokys left a comment

Looking good! I'm not able to test on OSX however, and my win32 knowledge is basic at best, so would be best if someone else could take a look too? If no-one else is available though, lets get this merged anyway.

Just a few small comments, and one request: do you think we could get some doc comments in the source for the public API? I can help out with that too if you like.


namespace Avalonia.Platform
{
class DragSource : IPlatformDragSource

This comment has been minimized.

@grokys

grokys Apr 12, 2018

Member

So this is the "in-process" drag source that is used when there is no OS drag source available. It should probably be called DefaultDragSource or InProcessDragSource or something?


namespace Avalonia.Input.DragDrop
{
public class DataObject : IDataObject

This comment has been minimized.

@grokys

grokys Apr 12, 2018

Member

I still don't see this class being used?

boombuler added some commits Apr 13, 2018

don't use OSX-Platform DragSource
this should be used when the DragSource does work with any object.
For now only strings and filenames work.
@boombuler

This comment has been minimized.

Copy link
Contributor Author

boombuler commented Apr 13, 2018

Hi,

@grokys I've added a sample to the control-catalog which shows the usage of the DataObject which you mentioned.
In addition to that:

  • I've added some documentation
  • Disabled the OSX Platform DragSource until it is finished. (OSX can still recieve drag events from other applications)
  • Renamed AcceptDrag to AllowDrop to comply with WPF.
@grokys

grokys approved these changes Apr 16, 2018

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 16, 2018

@boombuler I think this looks ready to merge, however there's still WIP in the title. Is there still work to do on it?

@boombuler boombuler changed the title WIP: Drag drop Drag drop Apr 17, 2018

@boombuler

This comment has been minimized.

Copy link
Contributor Author

boombuler commented Apr 17, 2018

Hi,

I've left the WIP because the OSX DragSource needs some work to send any object from one process to another. Its currently not in the service registration so OSX is only able to work with in-process d+d and receive infos from other processes. That can be done in a later PR (imho).
I would love to implement that but my trail version of rider expired...

@grokys grokys merged commit 30c4987 into AvaloniaUI:master Apr 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 17, 2018

Ok, thanks @boombuler - merged!

@boombuler boombuler deleted the boombuler:DragDrop branch Apr 17, 2018

@grokys grokys referenced this pull request Apr 17, 2018

Closed

Add Drag and Drop support #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.