implemented Event::FilesDropped for Windows#1222
Conversation
| //////////////////////////////////////////////////////////// | ||
| struct FileDropEvent | ||
| { | ||
| unsigned int count; ///< Number of dropped files |
There was a problem hiding this comment.
I'd rather see a std::vector<string> here, which would also make count obsolete.
There was a problem hiding this comment.
See the point above:
sf::Event::FileDropEvent is inevitably part of a union, so I couldn't use a vector but had to resort to a raw pointer. ...
There was a problem hiding this comment.
All event data is stored in a union, so using std::vector<string> is not possible. Since C++11, it would theoretically be allowed using placement new and manual destructor invocations. Alternatively, there's also std::variant/boost::variant (which is essentially a wrapper of the method described).
There was a problem hiding this comment.
Ah, right, the destructor getting broken data. Damn. :(
| Uint16 m_surrogate; ///< First half of the surrogate pair, in case we're receiving a Unicode character in two events | ||
| bool m_mouseInside; ///< Mouse is inside the window? | ||
| bool m_fullscreen; ///< Is the window fullscreen? | ||
| bool m_cursorGrabbed; ///< Is the mouse cursor trapped? |
There was a problem hiding this comment.
Is this member even needed? The list of files is in the event after all.
There was a problem hiding this comment.
No, the event only stores a pointer to the buffer. See the first two bullet points in my issues list.
| { | ||
| // This is the fallback for unsupported platforms. | ||
| // Trying to enable file dropping returns a bad value | ||
| // indicating an error, whereas disabling it is fine. |
There was a problem hiding this comment.
This is confusing. Why would it return !enabled? This indicates a success, if you fail to disable it.
There was a problem hiding this comment.
Yes, disabling a non-existent function is always possible. Only enabling it would be troublesome because it - well - doesn't exist.
There was a problem hiding this comment.
Ah, okay, from that perspective it makes sense. But I'm not 100% sure how it should react considering it's not implemented/available. You're making it fail silently.
There was a problem hiding this comment.
In my opinion, it doesn't fail silently. Disabling it is indistinguishable from it not being implemented, so why generate an error?
There was a problem hiding this comment.
Yeah, I can live with that when disabling.
| break; | ||
|
|
||
| // Expand the buffer list by one vector | ||
| FilesType& files = *m_droppedFiles.insert(m_droppedFiles.end(), FilesType()); |
There was a problem hiding this comment.
This doesn't make any sense either. First you append an element, then resize? Or did I miss something? And why the reference?
There was a problem hiding this comment.
m_droppedFiles is a std::list of std::vectors of sf::Strings. First append to the list, then resize the newly appended vector.
There was a problem hiding this comment.
Ah,, right, should probably read it twice next time. So you're appending a new list for every drag&drop operation. Too bad we can't free them unless we assume that it's only valid till the next event is polled.
There was a problem hiding this comment.
Exactly. I append a new vector to the list for every drop operation. I decided this way because I would rather waste some Kilobytes than get nasty SIGSEGVs. User friendliness is more important to me.
|
Question: do Windows/Linux support drag operation for other kinds of data (e.g. text, image, ...)? Mac does, that's why I'm asking. It actually works like a clipboard for that. So, like in #1221, we can start by only supporting one kind of data (i.e. paths) but it'd be nice to have a rough idea how things could be extended to support other data formats. In that respect, maybe a different API would be good. With 3.x we can think about using struct Data {
template <T> T get();
bool isPath(); // if true then get<String> is legal
bool isImage(); // if true then get<Image> is legal
// ...
// or maybe something like that:
struct Kind { Path, Image, ... };
Kind getKind();
};
/* implementation defined */ Cookie;
struct DropEvent {
Cookie cookie;
};
struct Window {
// Let's say the Data for a Cookie is valid at least until
// the next display/popEvent/whatever makes sense.
Data getDropData(Cookie);
void enableDropData(Kind kind, bool enable);
};What do you think? |
|
It looks like Linux does? It sends a MIME type with the data, from what I can tell here and the code from the link at the end of the issue text (although I'll admit I didn't understand most from either places). Windows does seem to, since I just dragged highlighted text from Firefox directory to Visual Studio and it put the text in the editor. I can also drag an image directly from Firefox into MS Paint. (I'm lost trying to find documentation on how this works though. It seems related to clipboard formats? From what I read here, Windows can actually give multiple formats at once.) The API just proposed can't support Windows having multiple types at once, so if that is chosen SFML would need to decide what is best and only give that (or generate an event for each supported type, but that doesn't sound good either). |
|
Linux supports multiple formats as well. A lot of software wrongly just chooses the first in the list, which causes things to not work properly quite often (try dragging some selected text to Dolphin for example, the first entry is a pointer to the text rather than the text itself (a later option), and it writes 4/8 bytes (the pointer itself) to a new file. Always give the option to specify the formats and give the list of all available on linux. |
|
Has there been any progress? Fytch's idea seems outstanding. |
|
A bit late here, but instead of storing a raw pointer (not very user friendly) why not wrapping it into a custom class, which will manage it properly without any risks? I don't know how much could be implemented of hat I said, or even if it is relevant since I am not a maintainer of the library, just a random user sorry in advance |
|
Think this is superseded by #2265 so can be closed |
|
Superseded by #2265 |
Disclaimer : this feature is not ready to be merged yet
Related forum thread: https://en.sfml-dev.org/forums/index.php?topic=19096
I have implemented this feature for personal use. I write simple map editors and other small tools with the aid of SFML because I don't want to bring out the big guns like Qt. For something like a map editor, the drag and drop functionality is very handy, especially for textures and materials.
As can be seen in the linked forum thread, I am not the only one wanting this feature. Therefore, I went ahead and created a pull request. This feature is not complete yet but it already works on Windows. I figured that once I take the first step maybe others are more inclined to chime in and collaborate.
Progress:
sf::Eventto store the relevant informationsf::Window(seesetFileDroppingEnabled)Issues:
sf::Event::FileDropEventis inevitably part of aunion, so I couldn't use a vector but had to resort to a raw pointer. Personally, I think this is not a big deal because the usage is still very straight forward:for (int i=0; i<event.droppedFiles.count; ++i) event.droppedFiles.files[i];But maybe there's a better solution?
sf::Event::FileDropEventonly stores pointers but not the filenames themselves, their lifetime is bound to the window, not to the event. Is there any case in which this window-specific buffer can cause trouble?WindowImplWin32::processEventdoesn't need to be thread-safe? Otherwise the aforementioned buffer would need to be protected by a mutex, ensuring that no concurrent accesses are possible.Unfortunately, I'm not versed well enough in X11 to implement this feature for Linux, but I have found this which might be handy for anyone wanting to implement this feature.