Add drag and drop support by justanobdy · Pull Request #2265 · SFML/SFML

justanobdy · 2022-10-30T04:09:17Z

Description

Edited to reflect new changes

This pull request adds file dropping support on Windows, X11, and MacOS. Unfortunately, this does not work on Wayland, even under xWayland.

Notes:

File drops are manually split into std::vectors on X11, which is less than ideal.
The file dropping API used on MacOS is the old, deprecated version, but because OpenGL is deprecated on MacOS, we are forced to use it.

Example

#include <SFML/Graphics.hpp>
#include <iostream>

int main() {
    sf::RenderWindow window(sf::VideoMode(sf::Vector2u{300, 300}), "dnd test", sf::State::Windowed);

    window.setFileDroppingEnabled(true);

    while(window.isOpen()) {
        while(const std::optional event = window.pollEvent()) {
            if(event->is<sf::Event::Closed>()) {
                window.close();
            }
            
            if(const auto dropped = event->getIf<sf::Event::FilesDropped>()) {
                for (const auto& item : dropped->filenames)
                {
                    std::cout << "This file was dropped: " << item.toAnsiString() << std::endl;
                }
            }
        }

        window.clear();

        window.display();
    }
}

codecov · 2022-10-30T04:17:40Z

Codecov Report

Merging #2265 (a3555f1) into master (7faa550) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   14.98%   14.83%   -0.15%     
==========================================
  Files         213      208       -5     
  Lines       15255    15317      +62     
  Branches     4072     4087      +15     
==========================================
- Hits         2286     2273      -13     
- Misses      12821    12914      +93     
+ Partials      148      130      -18

Impacted Files	Coverage Δ
include/SFML/Window/Window.hpp	`0.00% <ø> (ø)`
src/SFML/Window/Unix/WindowImplX11.cpp	`0.18% <0.00%> (-0.01%)`	⬇️
src/SFML/Window/Win32/WindowImplWin32.cpp	`0.00% <0.00%> (ø)`
src/SFML/Window/Win32/WindowImplWin32.hpp	`0.00% <ø> (ø)`
src/SFML/Window/Window.cpp	`0.00% <0.00%> (ø)`
src/SFML/Window/WindowImpl.cpp	`0.00% <0.00%> (ø)`
src/SFML/Window/WindowImpl.hpp	`0.00% <ø> (ø)`
include/SFML/System/String.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
include/SFML/System/Clock.hpp	`50.00% <0.00%> (-50.00%)`	⬇️
include/SFML/Graphics/Shape.hpp	`0.00% <0.00%> (-50.00%)`	⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7faa550...a3555f1. Read the comment docs.

JonnyPtn · 2022-11-16T11:05:38Z

I think the main thing needed here (along with #1222) is a decision on whether to support multiple types or not. Otherwise it looks reasonable.

For what it's worth I would suggesting starting with support for files and text, then it can be expanded later if desired

justanobdy · 2022-11-17T20:29:44Z

I agree, only supporting files and text is enough for most people right now. The only thing we still need to do is agree on an API which works on all platforms, and make sure file dropping works on all the platforms that support it.

Bromeon

Thanks for the PR!

This seems like yet another PR which might suffer from having no modern sf::Event API (see comment). I would advocate we do not block it on that issue though, since that may take forever. Also, SFML 3 APIs are in flux and can be modified at any time.

But we should probably write a TODO and possibly a GitHub issue, so we don't forget this.

Bromeon · 2022-11-18T14:50:29Z

src/SFML/Window/Unix/WindowImplX11.cpp

+                // Get the dropped item by getting the XdndSelection property
+                m_droppedItem = getWindowStringProperty(m_display, m_window, windowEvent.xselection.property);
+
+                Event event;
+                event.type             = Event::ItemDropped;
+                event.itemDropped.item = &m_droppedItem;
+                event.itemDropped.x    = m_dropPosition.x;
+                event.itemDropped.y    = m_dropPosition.y;


Storing the event string in the window is asking for trouble.

If another ItemDroppedEvent is processed while the first one is still alive, the string is suddenly wrong. In other words, it's possible that a const ItemDroppedEvent suddenly changes contents only by external means. You will also get UB through dangling pointer if e.g. the underlying sf::Window is moved while the event is still around.

Granted, these are edge cases, but ideally we'd have an API that's robust enough to not fall apart when a user wants to do more unconventional things.

I'm aware that the union approach we're using here is very limited, and this is actually a good example to show why. I don't know yet what a good API would look like. std::variant is unfortunately verbose as hell and really not fun to work with.

I'm not sure if it is possible to get multiple drop events at once, because I don't think XdndDrop can be called multiple times per frame. (It might be possible if the user is deliberately trying, but I'm not really sure what could be done about that)

About the dangling pointer, I don't really know what could be done about that.

Bromeon · 2022-11-18T15:29:04Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    // Signal the selection owner that we have successfully read the data.
+    XDeleteProperty(dpy, w, p);


We have two XGetWindowProperty calls, but only one XDeleteProperty.

Is this correct? Do we not leak memory?

see https://github.com/SFML/SFML/pull/2265/files/f49594a978e8c1307ace93a505c9a49cbf37433f#r1026700494

Bromeon · 2022-11-18T15:29:58Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    XGetWindowProperty(dpy, w, p, 0, static_cast<long>(size), false, AnyPropertyType, &da, &di, &dul, &dul, &prop_ret);
+
+    String value = reinterpret_cast<char*>(prop_ret);
+
+    XFree(prop_ret);


These statements could be grouped (no empty lines) so we know they are closely related.

see https://github.com/SFML/SFML/pull/2265/files/f49594a978e8c1307ace93a505c9a49cbf37433f#r1026700494

Bromeon · 2022-11-18T15:31:44Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    Atom           da, incr, type;
+    int            di;
+    unsigned long  size, dul;
+    unsigned char* prop_ret = NULL;


Please declare variables as late as possible -- only once you need them. If possible, initialize them immediately during declaration.

Declaring everything at the beginning of the function is relatively outdated C practice, either mandated by old compilers or a way to measure stack memory usage.

I took this code from https://www.uninformativ.de/blog/postings/2017-04-02/0/POSTING-en.html, and i didn't test it very thoroughly, I just wanted to make a quick draft PR so we could figure out how to get it to work on multiple platforms, and then I would go and polish it up before merging it into master.

I'd also like to see Bromeon's comments be addressed even if this is just a draft PR.

Ok, I've completely changed how the selection is converted. I modded the conversion code from https://github.com/SFML/SFML/blob/master/src/SFML/Window/Unix/ClipboardImpl.cpp to work with mine.

ChrisThrasher · 2022-11-18T21:47:54Z

I see you're using clang-format-15. Sadly that version breaks lots of vertical alignment rules so it produces very different results from versions 12, 13, and 14 which we're using in CI. I don't have a great solution other than try to use one of those 3 versions if you can your hands on an installation of it.

justanobdy · 2022-11-18T22:13:39Z

Ah, that's why it changed some random stuff. I upgraded from Leap 15.4 to Tumbleweed, so that's why it changed. I'll try to see if I can get 14 on my machine.

justanobdy · 2022-11-18T22:19:51Z

Ok, I've used clang-format-13 and pushed the changes.

ChrisThrasher · 2022-12-04T22:06:20Z

src/SFML/Window/OSX/SFKeyboardModifiersHelper.h

 class WindowImplCocoa;
 }
-}
+} // namespace sf


I'm guessing these got added because you used clang-format-15 to format. You can remove these since we now disallow using clang-format-15.

Visual Studio formatted everything for me, and I guess it used clang-format-15.

ChrisThrasher · 2022-12-04T22:06:39Z

src/SFML/Window/Unix/WindowImplX11.cpp

+                                                &remainingBytes,
+                                                &data);
+
+                sf::String value;


Where does this get used?

src/SFML/Window/Win32/WindowImplWin32.cpp

ChrisThrasher

The previous CI run succeeded minus formatting failing. Please try to get a copy of clang-format 12, 13, or 14 and build the format target to fix this.

ChrisThrasher · 2022-12-05T03:12:29Z

include/SFML/Window/Event.hpp

 #include <SFML/Window/Mouse.hpp>
 #include <SFML/Window/Sensor.hpp>

+#include <memory>


I think you can remove this header

src/SFML/Window/OSX/CursorImpl.mm

src/SFML/Window/OSX/SFOpenGLView.h

src/SFML/Window/OSX/SFWindowController.h

src/SFML/Window/OSX/WindowImplDelegateProtocol.h

src/SFML/Window/Win32/WindowImplWin32.cpp

src/SFML/Window/iOS/EaglContext.mm

src/SFML/Window/iOS/SFAppDelegate.mm

src/SFML/Window/iOS/SensorImpl.mm

justanobdy · 2022-12-05T03:59:20Z

is there a way to get clang-format to reformat the whole codebase? because clang-format-13 isn't fixing everything, and I don't want to mess with visual studio's clang-format version

justanobdy · 2022-12-05T04:00:31Z

should I merge all the commits together and start a new pr on a new branch? (not named X11DnD)

ChrisThrasher · 2022-12-05T04:01:47Z

is there a way to get clang-format to reformat the whole codebase?

Yes, that's what happens when you build the format target.

EDIT: The formatting CI jobs are passing now so I think you're good.

should I merge all the commits together and start a new pr on a new branch?

Please don't make a new branch or PR. If you want to clean up the commit history then go for it, but you can wait to do that later if you want. If you do that, you can just git push --force to update this PR with your new commits.

justanobdy · 2022-12-05T04:14:19Z

Please don't make a new branch or PR. If you want to clean up the commit history then go for it, but you can wait to do that later if you want. If you do that, you can just git push --force to update this PR with your new commits.

Ok, I wasn't sure if it was fine that a branch named "X11DnD" also had windows support.

ChrisThrasher · 2022-12-05T04:15:24Z

You're good. Branches names aren't that important since they don't appear in the commit history after a merge.

coveralls · 2024-09-03T22:11:26Z

Pull Request Test Coverage Report for Build 10754038072

Details

7 of 160 (4.38%) changed or added relevant lines in 7 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.5%) to 56.171%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Window/WindowBase.cpp	0	2	0.0%
src/SFML/Window/macOS/SFViewController.mm	0	6	0.0%
src/SFML/Window/macOS/SFWindowController.mm	0	6	0.0%
src/SFML/Window/macOS/WindowImplCocoa.mm	0	6	0.0%
src/SFML/Window/Win32/WindowImplWin32.cpp	0	16	0.0%
src/SFML/Window/macOS/SFOpenGLView.mm	0	25	0.0%
src/SFML/Window/Unix/WindowImplX11.cpp	7	99	7.07%

Files with Coverage Reduction	New Missed Lines	%
src/SFML/Window/WindowImpl.cpp	1	58.17%
src/SFML/Window/Unix/WindowImplX11.cpp	4	40.25%

Totals
Change from base Build 10732212885:	-0.5%
Covered Lines:	11909
Relevant Lines:	20080

💛 - Coveralls

ChrisThrasher · 2025-03-05T23:53:39Z

src/SFML/Window/macOS/SFOpenGLView.mm

    [self updateCursorGrabbed]; // update for fullscreen
+
+    // Register the drag types that this view can accept
+    //[self registerForDraggedTypes:[NSArray arrayWithObjects:NSFilenamesPboardType, nil]];


Do you recall why this is commented out?

justanobdy force-pushed the X11DnD branch 2 times, most recently from 2f18789 to d51b787 Compare October 30, 2022 04:41

Bromeon added feature m:sfml-window p:linux labels Oct 30, 2022

justanobdy force-pushed the X11DnD branch 2 times, most recently from e6552cf to f49594a Compare October 30, 2022 15:55

Bromeon reviewed Nov 18, 2022

View reviewed changes

justanobdy force-pushed the X11DnD branch 2 times, most recently from 0182953 to 66c75c1 Compare November 18, 2022 21:46

justanobdy force-pushed the X11DnD branch from 66c75c1 to d1af9bc Compare November 18, 2022 22:19

ChrisThrasher requested changes Dec 4, 2022

View reviewed changes

ChrisThrasher requested changes Dec 5, 2022

View reviewed changes

justanobdy force-pushed the X11DnD branch from a3555f1 to 31e6c05 Compare September 2, 2024 18:31

justanobdy changed the title ~~Add drag and drop on X11~~ Add drag and drop support Sep 2, 2024

justanobdy force-pushed the X11DnD branch from 31e6c05 to 24a5b36 Compare September 2, 2024 18:40

ChrisThrasher force-pushed the X11DnD branch 2 times, most recently from da762ae to 60b6dc3 Compare September 2, 2024 20:34

justanobdy force-pushed the X11DnD branch from 60b6dc3 to 2de97d7 Compare September 3, 2024 21:07

justanobdy force-pushed the X11DnD branch from 2de97d7 to 000fe32 Compare September 4, 2024 04:29

eXpl0it3r added this to the 3.1 milestone Sep 4, 2024

justanobdy force-pushed the X11DnD branch from 000fe32 to 89a23ea Compare September 7, 2024 19:57

ChrisThrasher force-pushed the X11DnD branch from 89a23ea to 0c5b6ab Compare March 5, 2025 23:56

ChrisThrasher reviewed Mar 5, 2025

View reviewed changes

Add file dropping support

2dede20

ChrisThrasher force-pushed the X11DnD branch from 0c5b6ab to 2dede20 Compare March 6, 2025 21:01

eXpl0it3r removed this from the 3.1 milestone Jun 19, 2025

eXpl0it3r added this to SFML 3.2.0 Jun 19, 2025

eXpl0it3r removed this from SFML 3.1.0 Jun 19, 2025

github-project-automation bot moved this to Planned in SFML 3.2.0 Jun 19, 2025

eXpl0it3r added this to the 3.2 milestone Aug 28, 2025

eXpl0it3r mentioned this pull request Aug 28, 2025

Add Support for Drag & Drop Events #3565

Open

eXpl0it3r linked an issue Aug 28, 2025 that may be closed by this pull request

Add Support for Drag & Drop Events #3565

Open

This was referenced Nov 26, 2025

implemented Event::FilesDropped for Windows #1222

Closed

Support for file dropping #3615

Open

		// Signal the selection owner that we have successfully read the data.
		XDeleteProperty(dpy, w, p);

Uh oh!

Conversation

justanobdy commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example

Uh oh!

codecov bot commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JonnyPtn commented Nov 16, 2022

Uh oh!

justanobdy commented Nov 17, 2022

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisThrasher commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justanobdy commented Nov 18, 2022

Uh oh!

justanobdy commented Nov 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChrisThrasher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justanobdy commented Dec 5, 2022

Uh oh!

justanobdy commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisThrasher commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justanobdy commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justanobdy commented Oct 30, 2022 •

edited

Loading

codecov bot commented Oct 30, 2022 •

edited

Loading

Bromeon Nov 18, 2022 •

edited

Loading

ChrisThrasher commented Nov 18, 2022 •

edited

Loading

justanobdy commented Dec 5, 2022 •

edited

Loading

ChrisThrasher commented Dec 5, 2022 •

edited

Loading

justanobdy commented Dec 5, 2022 •

edited

Loading

coveralls commented Sep 3, 2024 •

edited

Loading