Introduce drag and drop handling

[martincapello]

This PR contains the drag and drop implementation in each platform for laf. Also exposes the API that can be used by laf clients to use drag and drop in a platform-independent way.

[martincapello]

@dacap this is just to iterate over the design of the solution. This is not complete, just has the basic elements I come up with to draft a first approach to drag and drop handling. Let me know if you had something different in mind, because I also had another idea involving observers, but since we are not using observers in laf I had discarded that idea. The drag_and_drop.cpp file is a copy of multiple_windows.cpp example with a basic-empty implementation of the still-unfinished DnD API.

[dacap]

Although it's not a bad idea to use these kind of delegates, I think it will make a little harder to process the events in the same order in Aseprite ui::Manager::generateMessagesFromOSEvents().

I think it would be better to keep all event handling as a queue of events (so we know the exact order of events).

[martincapello]

Actually my first approach was just about translating the event to laf as it is done for every other event. But (and maybe I should have mentioned this before) I ran into the following issue when trying to implement it for draggingUpdated:

This method is called periodically for the destination window when the user is dragging an "image" (as they call it on the docs) around. The destination window that implements the draggingUpdated must return the NSDragOperation value depending if it wants to accept the future drop or not, so when I realized that by creating a laf event and sending it to the queue it would be treated asynchronously, thus loosing the chance of returning the appropriate NSDragOperation at the requested time, I've decided to change the approach.

Also I believe the same issue exists when implementing IDropTarget::DragOver method for Windows.

Maybe there is a workaround for this that I'm not aware of, or I am missing something.

[dacap]

It looks like a similar case to Window::handleHitTest or System::handleWindowResize (you will see a comment about a possible future SystemDelegate to replace handleWindowResize). These public std::function fields should be replaced in a future with some kind of delegate (probably everything in the window as a possible WindowDelegate).

We could offer some others handleDndSomething function fields but doing the delegate solution directly will be the best for the whole drag and drop mechanism. We can give a try and if it does work leave it as it is. (And if it doesn't work, then refactor it with events.)

I was also thinking that the drag and drop events will require several new fields that regular os::Events don't (and the queue of events is an array of os::Events), so probably it will make sense to just create a os::DndEvent for all these member functions of the delegate with all the drag and drop info required.

Some comments:

1. Don't base your example on multiple_windows, drag_and_drop example could just have one window (make the example as small as possible to show only the drag and drop mechanism)
2. Not sure if using DnD or Dnd as prefix or things like DragSource and DropTarget as class names for the delegates (same for the event, it could be os::DragEvent)
3. The header file could be called os/dnd.h including everything needed for drag and drop
4. os/os.h should include os/dnd.h so examples don't require to include this specific header
5. I'm doing several refactors on the beta branch, this shouldn't be a problem as I'll merge/fix conflicts later with your changes

[martincapello]

It looks like a similar case to Window::handleHitTest or System::handleWindowResize (you will see a comment about a possible future SystemDelegate to replace handleWindowResize). These public std::function fields should be replaced in a future with some kind of delegate (probably everything in the window as a possible WindowDelegate).

Yeah, I saw those and I imagined that there were there because of a similar reason, in fact I've considered using functions instead of the delegates, but I think I saw the comment you mention so I decided to go for delegates at the end.

We could offer some others handleDndSomething function fields but doing the delegate solution directly will be the best for the whole drag and drop mechanism. We can give a try and if it does work leave it as it is. (And if it doesn't work, then refactor it with events.)

Ok, I'll go ahead with the proposed delegates then.

I was also thinking that the drag and drop events will require several new fields that regular os::Events don't (and the queue of events is an array of os::Events), so probably it will make sense to just create a os::DndEvent for all these member functions of the delegate with all the drag and drop info required.

Alright, will create a new os::DndEvent class that doesn't inherit from os::Event.

Some comments:

1. Don't base your example on multiple_windows, drag_and_drop example could just have one window (make the example as small as possible to show only the drag and drop mechanism)

Sure, that is the idea. Making a copy of multiple_windows just was the quicker thing to do for me at the time.

2. Not sure if using DnD or Dnd as prefix or things like DragSource and DropTarget as class names for the delegates (same for the event, it could be os::DragEvent)

Sure. We could also create a dnd namespace I guess, and use Source and Target as class names. But it is up to you, I don't have any preference. Just let me know if you want to go ahead with the names you proposed.

3. The header file could be called os/dnd.h including everything needed for drag and drop

Sounds good.

4. os/os.h should include os/dnd.h so examples don't require to include this specific header

Perfect.

5. I'm doing several refactors on the beta branch, this shouldn't be a problem as I'll merge/fix conflicts later with your changes

👍

[martincapello]

@dacap This is still in draft but I want you to take a look at the current status. You can run the drag_and_drop example (which is not finished yet) to see some of things in action. Things implemented (just for macOS right now):

• DragTarget definition.
• Realtime drag feedback when the user moves the dragged data.
• Support for dropping file paths from finder (this is similar as we have in the main branch, but using the DragTarget interface instead).
• Introduction of DragDataProvider interface and its implementation for macOS. This is used to get data from the dragged payload, i.e. the Pasteboard in macOS.

Things missing:

• DragSource definition.
• Support for other kind of data items (images, text, pdf, RTF, strings, etc).
• Changing the cursor at will.

However, I think that with the current status (and once we got this implemented in Windows and Linux) we have the basic blocks in place to start with https://github.com/aseprite/aseprite/issues/131 in Aseprite. Then we can implement the rest in laf in the future. What do you think?

[dacap]

The first thing I tried was to drag this box and didn't work:

I'll try to review this soon.

[martincapello]

Hehe...yeah, it can't be dragged...the DragSource is not implemented. So just try to drop files from finder...or any other DnD source app that provides NSFilenamesPboardType items.

[dacap]

Probably we should create an example showing only the functionality we're implementing/need for the feature. Drop paths and images, nothing else.

[martincapello]

Yes, I agree. Once we are okay with the approach taken I will remove the things that are not functional from the example. Images are not supported at the moment. There are some commented code just to show how they could be approached, but nothing else.

[martincapello]

My idea was to implement the minimum necessary for supporting drag & dropping of files with realtime update of the UI while dragging them.

[dacap]

About data types, we need only filenames/paths and images. Generally the "drag and drop" functions are related to the clipboard functions, so it might be possible that some functions from the clip library can be used (which might require changes in the clip library).

[dacap]

One last time at the moment is if we can merge dropResult/acceptDrop in some way, or explain when each field must be set/used on each event (and if we need a DropEvent class to separate fields for a drop-only event).

[martincapello]

One last time at the moment is if we can merge dropResult/acceptDrop in some way, or explain when each field must be set/used on each event (and if we need a DropEvent class to separate fields for a drop-only event).

We can remove the acceptDrop field and let DragTarget::drop return a boolean that indicates if the drop was accepted or not. Something similar could be done for DragTarget::dragEnter and DragTarget::draggingUpdated by making them return a DropOperation and then removing the dropResult field from DragEvent.

[dacap]

We can remove the acceptDrop field and let DragTarget::drop return a boolean that indicates if the drop was accepted or not. Something similar could be done for DragTarget::dragEnter and DragTarget::draggingUpdated by making them return a DropOperation and then removing the dropResult field from DragEvent.

They can be as they are, better to keep then as fields of the event. Probably the acceptDrop is confusing in the example because there is other acceptDrop (the one from the data structure).

I would try to keep the example as simple as possible (one window, removing the std::map, and with less option).

[martincapello]

About data types, we need only filenames/paths and images. Generally the "drag and drop" functions are related to the clipboard functions, so it might be possible that some functions from the clip library can be used (which might require changes in the clip library).

I've been looking the clip library, and yes, all the image-related functions could be adapted to reuse them in laf. Will see what can I do.

[martincapello]

Made the suggested changes and added the ability to get images from the dragged data. For this I copied and adapted code from the clip library (for macOs) to use laf surfaces. It was also needed to add some things to laf.

[martincapello]

Just pushed some progress on the windows implementation. It is not finished and also broke something in the macOs implementation because of some changes that I didn't have the chance to make today.

The example is half-working for windows, it lacks the dragged data reading/showing, right now it just shows the mouse position and receives the events appropriately.

[dacap]

As we talked the other time, probably we can make the laf library depends on the clip library. I can prepare this change on aseprite and laf main branch moving the clip submodule from aseprite to laf.

[martincapello]

As we talked the other time, probably we can make the laf library depends on the clip library. I can prepare this change on aseprite and laf main branch moving the clip submodule from aseprite to laf.

Sure, but the thing was that the code in the clip library doesn't use os::Surface. So I think I will have to figure out how to interface from laf to the clip library without making clip aware of anything from laf.

[dacap]

the thing was that the code in the clip library doesn't use os::Surface. So I think I will have to figure out how to interface from laf to the clip library without making clip aware of anything from laf.

Yes, but that shouldn't be a problem, as the clip library has a 32-bit image format with custom mask/shift (just like the os::Surface, the same pixel data could be used directly between clip/laf-os).

I'll try to move the submodule between the projects so we have clip already available in laf.

[dacap]

The clip module is available in the main branch now.

[martincapello]

Oh, just in case, in the clip library there is another comptr implementation in clip_win_wic.h...which seems like incomplete or something (for instance it doesn't call AddRef() in any method).

[dacap]

Oh, just in case, in the clip library there is another comptr implementation in clip_win_wic.h...which seems like incomplete or something (for instance it doesn't call AddRef() in any method).

The clip library is an independent library and has its own wrappers (comptr, coinit, hmodule, Hglobal) with all the methods that it needs. AFAIR as the clip comptr is a wrapper only to release created resources, there is no need for add ref (the reference is added by each creation functions, e.g. CoCreateInstance() adds the reference, and we then release it).

In the best case, we should avoid any interaction between laf <-> clip using COM pointer wrappers (just use raw pointers between boundaries if it's necessary, just like any other COM API).

[dacap]

I fear that this PR might be impossible to review if gets to big. It might be interesting to think an approach were we merge first the macOS impl, then the Windows one, and finally the Linux one. (Even if some refactors to the events must be done for each impl.)

[martincapello]

I fear that this PR might be impossible to review if gets to big. It might be interesting to think an approach were we merge first the macOS impl, then the Windows one, and finally the Linux one. (Even if some refactors to the events must be done for each impl.)

Sure, no problem. But I think that first I would like to merge the windows implementation. Because I'm currently working on it along some clip lib changes (since now laf depends on clip). Then I will have to refactor some things of the macOS impl to make it uses clip (instead of the copy & paste I did before the clip dependency).

[martincapello]

Okay, I've organized a bit the commits history hopefully. These changes need this PR merged into aseprite fork: https://github.com/dacap/clip/pull/69

Also, the macOS implementation was removed for now, I have to make some adjustments and then I will commit it separately, maybe in a different PR after we've merged this.

There is an inconvenience about the windows implementation...to test the image drop in the drag_and_drop example you will have to use some application that is able to provide drag of clipboard formats such as "PNG", DIBV5, or DIB. I implemented a (basic and incomplete) win32 program for testing which allows the user to drag the last thing copied into the clipboard. If you want this program please let me know. In macOS this was different, I was able to use Chrome to drag images from the browser itself, the same doesn't happen in windows, it doesn't drag the image data in any of the format we use.

[martincapello]

Had to force push the last two commits to introduce the changes mentioned here: https://github.com/dacap/clip/pull/69#issuecomment-2050846596

[martincapello]

Sorry to force push again, but it really makes sense to me to have that clip submodule update within the windows implementation changes.

[dacap]

I'll start reviewing this right now. So in some way I don't expect new commits until it's reviewed or merged.

[dacap]

@martincapello how can I test dropping an image on Windows? it looks like it's not working, I'm trying to drop an image from Chrome.

Also I'd change the location of the drop zone to this corner (on macOS tested dropping an image and it might move the drop zone out of the window area):

[martincapello]

@martincapello how can I test dropping an image on Windows? it looks like it's not working, I'm trying to drop an image from Chrome.

As I mentioned here on Windows you have to use some app that let you drag images, I had to create one for this because Chrome doesn't do the same as in macOS.

Also I'd change the location of the drop zone to this corner (on macOS tested dropping an image and it might move the drop zone out of the window area):

Sure, I'll change it.

[dacap]

As I mentioned here on Windows you have to use some app that let you drag images, I had to create one for this because Chrome doesn't do the same as in macOS.

I think it should work with some standard program at least (Chrome, Edge, etc.).

[martincapello]

As I mentioned here on Windows you have to use some app that let you drag images, I had to create one for this because Chrome doesn't do the same as in macOS.

I think it should work with some standard program at least (Chrome, Edge, etc.).

Then we should add support for more Clipboard formats on windows. Because if Chrome (or Edge) only sends the URL of the image, we will need to support getting the URL, and the work of getting the actual image data will be responsibility of the library client. Right?

[dacap]

Then we should add support for more Clipboard formats ~on windows~. Because if Chrome (or Edge) only sends the URL of the image, we will need to support getting the URL, and the work of getting the actual image data will be responsibility of the library client. Right?

If that is what Chrome reports in the drop operation we should do that (or report the drop operation as an URL data type, so the app can download the URL). I think this drag-and-drop from Chrome/web browser to the program will be the most common (and probably unique) use case dragging images directly into Aseprite.

[martincapello]

This force push was to keep all the changes of the IDataObject wrapper simplification in one commit.

[martincapello]

This is ready for review. I'm going to add support for dragging URLs (https://github.com/aseprite/laf/pull/83#issuecomment-2056959996). But maybe we can merge this as is after revision.

[dacap]

Just in case I'll start reviewing this PR.

[dacap]

Something about 3f13e5f867017cb73204bb53bdec1a10ea981d32 I'm not sure why the SurfaceFormat enum was refactored to a enum class. It looks like something unnecessary or at least, when we refactor an enum to a enum class it's better to remove the suffix part, e.g. instead of kRgbaSurfaceFormat we could use:

enum class SurfaceFormat {
  kRgba,
};

I'll leave this as it is in the PR and see how we finally rename these enumerations in #11 (its a task of a bigger API redesign).

[martincapello]

Something about 3f13e5f I'm not sure why the SurfaceFormat enum was refactored to a enum class.

Because of this comment: https://github.com/aseprite/laf/pull/83#discussion_r1532500638

I took the chance to change it since I was making changes already.

[dacap]

Because of this comment: #83 (comment)

I took the chance to change it since I was making changes already.

We have 3 kind of enums:

enum Something { SomethingOne, SomethingTwo, SomethingThree };
enum Something { OneSomething, TwoSomething, ThreeSomething };
enum class Something { One, Two, Three };

But we don't have (at least I don't remember, or we should try to avoid) these ones:

enum class Something { OneSomething, TwoSomething, ThreeSomething };
enum class Something { SomethingOne, SomethingTwo, SomethingThree };

Because this force us to write Something two times, e.g. Something::OneSomething, etc. Anyway I've just rebased the dnd branch skipping 3f13e5f867017cb73204bb53bdec1a10ea981d32 and everything went just fine (and we can close #4414 as none of these changes are required).

[dacap]

Good job @martincapello! merged just right now 🍾

[martincapello]

Good job @martincapello! merged just right now 🍾

😅 tough one