Introduce drag and drop handling #83

martincapello · 2024-03-04T21:00:38Z

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.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+  os::draw_text(s, nullptr, buf, gfx::Point(0, y), &paint);
+  y += 12;
+
+  for (auto line : lines) {


warning: 'auto line' can be declared as 'const auto *line' [readability-qualified-auto]

Suggested change

for (auto line : lines) {

for (const auto *line : lines) {

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+{
+  // If all windows are hidden, show them again
+  auto hidden = std::count_if(windows.begin(), windows.end(),
+                              [](os::WindowRef window){


warning: the parameter 'window' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change

[](os::WindowRef window){

[](const os::WindowRef& window){

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+                              });
+  if (hidden == windows.size()) {
+    std::for_each(windows.begin(), windows.end(),
+                  [](os::WindowRef window){


warning: the parameter 'window' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change

[](os::WindowRef window){

[](const os::WindowRef& window){

examples/drag_and_drop.cpp

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+  os::ScreenList screens;
+  system->listScreens(screens);
+  char chr = 'A';
+  for (os::ScreenRef& screen : screens) {


warning: variable 'screen' of type 'os::ScreenRef &' (aka 'Refos::Screen &') can be declared 'const' [misc-const-correctness]

Suggested change

for (os::ScreenRef& screen : screens) {

for (os::ScreenRef const& screen : screens) {

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+    for (auto& p : pos) {
+      os::WindowSpec s = spec;
+      gfx::Rect frame = s.frame();
+      frame.x += frame.w*p.x;


warning: narrowing conversion from 'double' to 'int' [bugprone-narrowing-conversions]

frame.x += frame.w*p.x; ^

aseprite-bot · 2024-03-04T21:03:42Z

examples/drag_and_drop.cpp

+      os::WindowSpec s = spec;
+      gfx::Rect frame = s.frame();
+      frame.x += frame.w*p.x;
+      frame.y += frame.h*p.y;


warning: narrowing conversion from 'double' to 'int' [bugprone-narrowing-conversions]

frame.y += frame.h*p.y; ^

aseprite-bot · 2024-03-04T21:03:43Z

examples/drag_and_drop.cpp

+
+          // Duplicate window
+          case os::kKeyD: {
+            std::string title = ev.window()->title();


warning: variable 'title' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change

std::string title = ev.window()->title();

std::string const title = ev.window()->title();

aseprite-bot · 2024-03-04T21:03:43Z

examples/drag_and_drop.cpp

+          case os::kKeyF:
+          case os::kKeyC:
+          case os::kKeyW: {
+            std::string title = ev.window()->title();


warning: variable 'title' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change

std::string title = ev.window()->title();

std::string const title = ev.window()->title();

martincapello · 2024-03-04T21:06:34Z

@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 · 2024-03-04T21:14:06Z

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 · 2024-03-05T12:11:16Z

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 · 2024-03-05T13:09:44Z

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:

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)
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)
The header file could be called os/dnd.h including everything needed for drag and drop
os/os.h should include os/dnd.h so examples don't require to include this specific header
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 · 2024-03-05T14:08:38Z

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:

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.

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.

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

Sounds good.

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

Perfect.

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

👍

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-03-05T20:54:59Z

examples/drag_and_drop.cpp

+      case os::Event::KeyDown:
+        switch (ev.scancode()) {
+
+          case os::kKeyT:


warning: switch has 2 consecutive identical branches [bugprone-branch-clone]

case os::kKeyT: ^

Additional context

examples/drag_and_drop.cpp:151: last of these clones ends here

break; ^

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-03-06T21:00:34Z

examples/drag_and_drop.cpp

+      case os::Event::KeyDown:
+        switch (ev.scancode()) {
+
+          case os::kKeyT:


warning: switch has 2 consecutive identical branches [bugprone-branch-clone]

case os::kKeyT: ^

Additional context

examples/drag_and_drop.cpp:203: last of these clones ends here

break; ^

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-03-08T20:39:40Z

examples/drag_and_drop.cpp

+      case os::Event::KeyDown:
+        switch (ev.scancode()) {
+
+          case os::kKeyT:


warning: switch has 2 consecutive identical branches [bugprone-branch-clone]

case os::kKeyT: ^

Additional context

examples/drag_and_drop.cpp:221: last of these clones ends here

break; ^

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-03-13T13:31:48Z

examples/drag_and_drop.cpp

+      case os::Event::KeyDown:
+        switch (ev.scancode()) {
+
+          case os::kKeyT:


warning: switch has 2 consecutive identical branches [bugprone-branch-clone]

case os::kKeyT: ^

Additional context

examples/drag_and_drop.cpp:232: last of these clones ends here

break; ^

martincapello · 2024-03-13T14:01:15Z

@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 aseprite/aseprite#131 in Aseprite. Then we can implement the rest in laf in the future. What do you think?

dacap · 2024-03-13T17:27:01Z

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

I'll try to review this soon.

martincapello · 2024-03-13T17:31:53Z

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 · 2024-03-13T17:35:01Z

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

martincapello · 2024-03-13T17:52:07Z

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 · 2024-03-13T17:53:43Z

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

dacap

So far it looks good, some minor changes to the API and still some decisions to take (e.g. should we use unique pointers for targets/sources or leave the owning decision to the client side).

The example could be simplified just showing a box where we can drop things (files/images) and accept or not the DragEvent if we drop things inside that box (to avoid switching flags so many flags).

os/window.h

os/dnd.h

dacap · 2024-03-13T18:07:26Z

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 · 2024-03-13T18:24:20Z

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 · 2024-03-13T20:50:41Z

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.

… the alpha component

…om a source image that doesn't support alpha

…mages and offer the possibility of configuring the decoding functions to be used This is achieved by looking at the clipboard for a file that contains data in some of the supported image formats, because in Windows, the Chrome implementation puts the image's file content in the clipboard. Then after determining that the file contents corresponds to one of our supported image formats, the configured decoding function is used (which might be the default one)

…tails from the public interface

This is how most examples are working right now, draw and invalidate the updated area in the same function just to avoid calling invalidate() each time.

We need drag & drop working with scale parameter (as Aseprite uses scale=2 by default), so I've made some changes to test: 1) draw_window() must use the surface bounds which are scaled (not the windows bounds, which aren't) 2) The drop zone must use the same scaled bounds 3) Moved all logic to create the window inside create_window()

… functions

…ixels

…sition

…l free functions

…sition

…f flags instead of any of the specified flags

dacap · 2024-05-30T14:09:51Z

Good job @martincapello! merged just right now 🍾

martincapello · 2024-05-30T14:25:32Z

Good job @martincapello! merged just right now 🍾

😅 tough one

martincapello requested a review from dacap March 4, 2024 21:00

aseprite-bot reviewed Mar 4, 2024

View reviewed changes

martincapello force-pushed the dnd branch 3 times, most recently from 66f9381 to 8408f84 Compare March 5, 2024 20:52

aseprite-bot reviewed Mar 5, 2024

View reviewed changes

martincapello force-pushed the dnd branch from 8408f84 to 7d3794b Compare March 6, 2024 20:58

aseprite-bot reviewed Mar 6, 2024

View reviewed changes

martincapello force-pushed the dnd branch from 7d3794b to 0dafb45 Compare March 8, 2024 20:36

aseprite-bot reviewed Mar 8, 2024

View reviewed changes

martincapello force-pushed the dnd branch from 0dafb45 to 84152a8 Compare March 13, 2024 13:29

aseprite-bot reviewed Mar 13, 2024

View reviewed changes

martincapello assigned dacap Mar 13, 2024

dacap requested changes Mar 13, 2024

View reviewed changes

dacap assigned martincapello and unassigned dacap Mar 13, 2024

martincapello and others added 23 commits May 9, 2024 09:22

Add support to drag and drop URLs

071ddb9

Fix makeSurface(clip::image&) to premultiply the source RGB values by…

449e978

… the alpha component

[osx] Add support to drag and drop URLs

c35366f

Set destination alpha component value to 255 when converting color fr…

5d44a52

…om a source image that doesn't support alpha

Fix function declarations and definitions to separate windows impl de…

5483568

…tails from the public interface

Update clip submodule

5cf80a0

Change colors of drag_and_drop example

a522a9e

Invalidate window inside draw_window() in drag_and_drop example

a5f0f0f

This is how most examples are working right now, draw and invalidate the updated area in the same function just to avoid calling invalidate() each time.

Minor formatting/clean up changes in drag_and_drop example

8473680

Move os::CommonSystem impl to its own .cpp file

fefb437

[osx] Fix includes to get MenusOSX declaration

672c903

Replace use of std::function and lambdas by function pointer and free…

643ec49

… functions

Use uint8_t* instead of char* for the address of the source's image p…

deeb2db

…ixels

Move decoding related functions into os namespace

44babba

[osx] Move free functions inside os namespace

269aefa

[osx] Take into account the window scale when calculating the drag po…

e0d4317

…sition

Convert some SkiaSurface private static member functions into interna…

7fcfeb0

…l free functions

[osx] Remove duplicated code to create DragEvent

0a2537d

[win] Remove duplicated code to create DragEvent

97f5b85

[win] Take into account the window scale when calculating the drag po…

26533b4

…sition

Change sourceSupports behavior to return true when it matches a set o…

53de63b

…f flags instead of any of the specified flags

martincapello force-pushed the dnd branch from c6e5e0c to 53de63b Compare May 9, 2024 12:23

martincapello assigned dacap and unassigned martincapello May 9, 2024

dacap merged commit cb15528 into main May 30, 2024
13 checks passed

dacap deleted the dnd branch May 30, 2024 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce drag and drop handling #83

Introduce drag and drop handling #83

martincapello commented Mar 4, 2024

aseprite-bot left a comment

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

aseprite-bot Mar 4, 2024

martincapello commented Mar 4, 2024

dacap commented Mar 4, 2024

martincapello commented Mar 5, 2024 •

edited

Loading

dacap commented Mar 5, 2024

martincapello commented Mar 5, 2024

aseprite-bot left a comment

aseprite-bot Mar 5, 2024

aseprite-bot left a comment

aseprite-bot Mar 6, 2024

aseprite-bot left a comment

aseprite-bot Mar 8, 2024

aseprite-bot left a comment

aseprite-bot Mar 13, 2024

martincapello commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap left a comment

dacap commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap commented May 30, 2024

martincapello commented May 30, 2024

	for (os::ScreenRef& screen : screens) {
	for (os::ScreenRef const& screen : screens) {

	std::string title = ev.window()->title();
	std::string const title = ev.window()->title();

Introduce drag and drop handling #83

Introduce drag and drop handling #83

Conversation

martincapello commented Mar 4, 2024

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martincapello commented Mar 4, 2024

dacap commented Mar 4, 2024

martincapello commented Mar 5, 2024 • edited Loading

dacap commented Mar 5, 2024

martincapello commented Mar 5, 2024

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martincapello commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap left a comment

Choose a reason for hiding this comment

dacap commented Mar 13, 2024

dacap commented Mar 13, 2024

martincapello commented Mar 13, 2024

dacap commented May 30, 2024

martincapello commented May 30, 2024

martincapello commented Mar 5, 2024 •

edited

Loading