Mac file drag and drop

[monomadic]

Here's an implementation for drag and dropping files on Mac OS. I needed to add two events for HoveredFile and HoveredFileCancelled, as without them how are you supposed to update your gui? :)

At the moment, the DropFile only supports one FileBuf, and not an array... which is harder to deal with and you can't reject the drop during hover based on a file filter (say the user drops four valid files and one invalid one). But since there's a windows implementation of this (though it seems buggy/limited to 255 chars) I decided to leave it for now.

I wasn't too sure of winits code conventions, it seems to repeat code in places (like dispatching events) so I have stayed away from adding convenience methods etc. Please let me know if I should do something else to conform to winits approach more.

[monomadic]

...anyone? Just a confirmation or comment would be super appreciated, I'm afraid of branch divergence as it's a fairly active area of the code.

[LPGhatguy]

I'm afraid that most of winit's contributors are centered on either Linux or Windows, which is also demonstrated by the lack of activity on #229.

I don't know who the best person to review Mac changes is.

[monomadic]

Thanks for the reply.

That's an issue then, and it might make more people reluctant to make changes to improve it - I have a few PRs I wanted to put time into for the mac side of things, but I don't want to wait weeks for approval as #229 has - which by the way is another feature I also need. Who has been doing most of the Mac work? How can we change this situation, in my mind at least, having nobody accept these PRs is the reason you're not getting many PRs for mac, it's a catch-22 cycle.

[tomaka]

It's not like it's a solvable problem.

In the past I've been merging people's PRs for X11 without carefully reviewing them, and now we are in a situation where the X11 backend is a mess that's difficult to read. It would be nice if the same didn't happen to MacOS.

Unfortunately I have zero knowledge about cocoa (I don't even know what that msg_send macro is for), so I can't review myself.

[monomadic]

The msg_send macro is an objc call - cocoa-rs is primary a very shallow bunch of wrapper traits that each include a bunch of wrapper functions that contain calls to the msg_send macro (almost always just 1-line calls actually). However, cocoa-rs is woefully incomplete, so this PR simply calls the objc ffi bridge directly. Pretty much zero difference, but we can of course put a PR into cocoa-rs to add that call (though who knows how long that would take), and it would purely be an organisational change and wouldn't provide any more safety. I would suggest including a msg_send for now and I can make a PR to cocoa-rs if you like, and when that comes in we switch to that.

I believe @mitchmindtree actually sketched out a suggestion for implementing this which is very close to what I've done... perhaps if he's around he might have some insight?

I don't understand what you mean by 'It's not like it's a solvable problem' - are we talking PR requests for the mac module are unsolvable? I was considering adding the mac equivalent of the with_parent functionality recently merged for windows (in the platform specific stuff), but that's a MUCH bigger topic...

I find the response disheartening if that's what you meant, it IS a solvable problem, and if you were put in the same position re: X11 again, you should make the same decision - a few junior-ish devs maintaining a module poorly is better than a rotting module that doesn't work at all and can't keep feature parity, which is what happens if PRs are not able to be merged. Do you really need to personally review every change to each one of your projects? Because the entire rust community has a lot to fear if that's the case.

[tomaka]

a few junior-ish devs maintaining a module poorly is better than a rotting module that doesn't work at all and can't keep feature parity

It's not like the code "isn't working at all", as you say. It does work right now, and any pull request may break it inadvertently.

Instant A: the X11 module is working fine. Instant B: someone merges a PR that wasn't reviewed carefully enough. Instant C: the X11 module is broken, people open issues or ask me questions on IRC about it, and I have to respond to them. I guess that's my punishment for breaking things, so I have decided that I wouldn't merge non-trivial PRs unless they are reviewed first.

Winit contains unsafe code targetting poorly-designed and poorly-implemented legacy systems that are full of stupid undocumented flaws. If there's one repository whose changes need to be reviewed, it's this one. I'm not asking for a very careful review with stamp saying "this PR is bug-free". All I want is a second eye to catch potential issues.

Do you really need to personally review every change to each one of your projects?

I don't need to personally review changes, but I can't ask for other people to spend their time doing it.

[monomadic]

But you've said that you can't vouch for windows or linux rust code, so you're creating your own paradox here. You need to accept having someone to review changes you can't, anyone is better than no one. Perhaps have a minimum of two people you loosely trust check over each non-windows pr. If unreasonable bugs DO pop up after two people have checked it, you can't blame yourself for that, and you reassess those people involved in the check.

I understand your concerns about quality, but you can't let your past experiences sink the non-windows aspects of these projects. If any company on earth worked on the basis of one person reviewing all code that shipped from the company, that person would immediately become the single point of failure, burn out asap and leave a crater behind. And you're not even being paid :P

I'm trying to be constructive here, and challenge your idea of this being unsolvable, if you truly believe that, maybe revoke the mac and linux modules and focus the library on windows instead. The mac version works for now, but what about in a few months when the new MacOS comes out? What about two years from now? Choosing to support it but not support it is a very turbulent situation to be in.

[tomaka]

you don't understand even the most basic of windows or linux rust code

No, I only said I wasn't familiar with the MacOS code. I think I'm competent for Windows, Android and Emscripten, and I have the basics of X11. I wrote the initial implementation of X11 by following tutorials.

Perhaps have a minimum of two people you loosely trust check over each non-windows pr. If unreasonable bugs DO pop up after two people have checked it, you can't blame yourself for that, and you reassess those people involved in the check.

I'm totally in favour of this! The problem is that I can't ask people "hey can you please commit to reviewing all winit PRs for platform X"? There's already @Ralith and @mitchmindtree doing that from time to time, and it's great. Hopefully I'm not doing anything that deters any possible contributor, and if I do it's not on purpose.

Choosing to support it but not support it is a very turbulent situation to be in.

To be fair glutin (winit's ancestor) didn't support MacOS at the beginning. The MacOS backend was written by @mitchmindtree and other people, and I merged it. It would unreasonable to remove it now.

[monomadic]

What we used to do at a company I worked at (fairly large team) was have a tag for QA. If a PR had that, the issue would be on hold until two people min had checked it and posted in the thread that they'd checked it and any recommendations before push. Once it had gotten two those people partially owned the issue, so anything that went wrong it was their responsibility to help fix or rewind, as they'd let it slip past.

Every afternoon if we had nothing else to do or had finished work, we'd filter by QA and get issues over the line.

This is open source so it might not be feasible to just let just anyone QA an issue, but perhaps just give some of those who have any history with winit a chance, and newbies to QA-ing can still add their bit (but perhaps just two newbies posting a QA response might not qualify for a PR being pushed). Another rule was of course that you can't QA your own PR.

[tomaka]

I agree with all these rules, the problem is only manpower. As you said I'm not getting paid, and people who review don't get paid either.

I should probably add a CONTRIBUTING file or something to encourage people to do so, but I'm really not familiar with that. I personally tend to watch the repos I submitted PRs to and then read/review the PRs that touch the code I wrote, but most people probably don't do that.

[monomadic]

Perhaps reduce it to one other person QA the code then, until you have more active mac devs working on it? And then the QA contributor is responsible for helping patch bugs as a rule (otherwise won't really be trusted to QA in the future). Like I could QA that other guys PR on #229 and he could check over mine.

I'm not really sure how to proceed here, I have a lot of things I want in winit on the mac side, and know a great deal about rust-objc, cocoa-rs and winit now (I re-wrote the entire mac module from scratch in my own branch to work with vsts), but don't know how to contribute this code to the main repository. I'm writing my own gui lib and drag and drop is fundamental to that, for example...

[tomaka]

Well, I guess I could trust your code. If you wrote multiple PRs and even rewrote the entire backend, then you are probably familiar enough with the code to know what you're doing.
I said I wanted code quality, but I'm not too strict either. I just want to avoid PRs that break things.

Anyway, I'm going to leave this PR open for one more day in case someone shows up after this discussion. If it's not the case, please ping and I'll merge it.

[monomadic]

Thanks man, I appreciate that. I'll take one more look at it tonight and see if I can improve it before I ping, but it's quite small and doesn't do much (basically just responds to two events). I've documented it but I'll see if I can add anything or wrap any IdRefs which might need wrappers.

[elinorbgr]

Just some though, they are not related to the MacOS implementation (as I know nothing of MacOS), but regarding the impact of this PR on the other backends.

I completely see the value of the two new events you introduced, and I believe they make sense on most platforms. As such the other platforms will likely implement them too at some point.

I'm just wondering: I believe files are not the only thing you can drag&drop into a window. You can also drag text or images or stuff from window to window, can't you? (I reckon this may be quite platform specific here...).

In such case, wouldn't it make sense to directly handle the generic case, for example by putting an enum in the fashion of Event::HoveredContent(ContentType) with something like

pub enum ContentType {
    File(PathBuf),
    Text(String),
    /* maybe other formats ? */
    Other(String /* mime type  */, Vec<u8> /* raw contents */)
}

This idea comes from my understanding of how drag&drop works on wayland. I don't know how it can translate to other platforms, hence my question here.

[mitchmindtree]

I just tested this on my old mbp running OS X 10.10.5 and it seems to work nicely for multiple windows and handling multiple files. Looks good to me. Thanks!

[mitchmindtree]

I should mention that my mac is no longer my main work environment so it's not as easy for me to review these PRs as it used to be, however I'll attempt to continue to do so when I can while I have one.

[Ralith]

I'm just wondering: I believe files are not the only thing you can drag&drop into a window. You can also drag text or images or stuff from window to window, can't you? (I reckon this may be quite platform specific here...).

Qt implements drag and drop as "mime type, data" much like the last case of your example, fwiw. Maybe a fully general and forwards-compatible approach would be more like struct Drag { type: String, data: Vec<u8> } with a bunch of From and TryInto impls for common cases?

[monomadic]

@Ralith the events returned by the code I wrote are also used by the windows code, so I was as minimally invasive as possible, but left empty stub functions for the future in case we do move to a more general approach. I didn't want to break the windows stuff in some way or write code that did things exceptionally different to the existing stuff.

[Ralith]

Fair enough!

[monomadic]

@vberger Mac basically does drag and drop the same way copy-paste works, yep! I believe (totally up for re-thinking though) the best approach for the first PR is to get feature parity with the windows version, so that if you implement the existing events it will work across both platforms. I am hoping the next step is for someone to take the 'hovered' event and make that one work on windows too (but supporting it on mac won't break windows support, for example, it just won't have a hover effect for now so I thought that was acceptable).

Macs drag and drop behaviour is more sophisticated than windows (last I checked), it works with a Pasteboard system so receiving a drag-drop event is like having a file pasted onto it and it can contain all kinds of metadata and decide what files it wants to accept or reject or inspect... so the winit implementation could easily get wacky. For example, there's actually also a situation where you can get MULTIPLE types of data (for example, rich text and plan text)...

In the future I'd love to build some way (through a builder method) to register what filetypes you'd like to accept for dropping. Thoughts?

In summary, I feel simple / compatible first is better, I mean the basic framework of extending the event delegate and exposing callbacks is here, so perhaps if we can get this in, the next step could be to experiment in a separate PR and see how we go? Those extensions I imagine would be much more subjective to discussion.

[elinorbgr]

Macs drag and drop behaviour is more sophisticated than windows (last I checked), it works with a Pasteboard system so receiving a drag-drop event is like having a file pasted onto it and it can contain all kinds of metadata and decide what files it wants to accept or reject or inspect... so the winit implementation could easily get wacky. For example, there's actually also a situation where you can get MULTIPLE types of data (for example, rich text and plan text)...

From my understanding of it, Wayland works in a very similar way, this is why I raised this question at first.

I have mixed feelings regarding how to handle that for the future:

• Either merge it this way, to have things working nicely first, and then upgrade to the full solution handling every possible drag&drop later. This would be yet an other breaking change, but we could get things advance quickly.
• Either discuss and have a clean future-proof solution now, but it'll necessarily take more time.

I'd tend in favor of the first solution (merging this as is and refining later) as winit is clearly still in a fast-moving phase, and breaking changes are already kind of regular... Thoughts @tomaka @Ralith ?

[monomadic]

blink
any updates here? :d

[trishume]

I reviewed the code and tested this on my OSX Sierra Macbook with both one and multiple files. Everything looks good to me.

In the future, if you tag me in an issue, I'm happy to review and test OSX-related PRs.

[tomaka]

Either merge it this way, to have things working nicely first, and then upgrade to the full solution handling every possible drag&drop later. This would be yet an other breaking change, but we could get things advance quickly.

Let's go with this.

[monomadic]

Thanks, @trishume - if you make PRs for mac please feel free to do the same.