| Summary: | SDL_DropEvent loses sourceApplication string when passing from AppDelegate to SDL. | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Nathan Daly <nhdaly> |
| Component: | events | Assignee: | Sam Lantinga <slouken> |
| Status: | ASSIGNED --- | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | amaranth72, nhdaly, philipp.wiesemann |
| Version: | HG 2.1 | ||
| Hardware: | iPhone/iPod touch | ||
| OS: | iOS (All) | ||
| Bug Depends on: | 2762 | ||
| Bug Blocks: | |||
| Attachments: |
SDL_DropEvent srcApplication Patch.
SDL_DropEvent srcApplication Patch + PC Updated Patch to add sourceApplication to DropEvent: Fixes comments. Updated patch -- merged in changes since 2016-02, including new SendDropText and SendDropComplete. |
||
|
Description
Nathan Daly
2015-11-15 22:23:44 UTC
I've added Philipp and re-assigned to Ryan since I saw you two were the ones who originally responded to Bug 2762. Thanks! :) Thank you for working on a patch. The calls to SDL_SendDropFile() also need to be updated (with NULL) for the desktop operating systems because this function now has two parameters. Created attachment 2311 [details]
SDL_DropEvent srcApplication Patch + PC
Adds NULL to the calls in the Mac & Windows implementations in addition to the Android and iOS I had already done.
Thanks Phillipp for pointing that out! For whatever reason I hadn't seen the calls from the desktop code in my source files when I first wrote the patch. I appreciate your quick response! :) Thank you for updating the patch. The documentation at SDL_DropEvent is now a bit misleading. Though the field will (currently) be NULL on Android, it may also be NULL on some the other systems. Maybe it should just be documented that it is NULL if the source is not known. This would also point users to always delete the string because they can not assume they need not (e.g. for only being on Android). Which again would improve upward compatibility of their programs a bit (with this feature creating a memory leak for older programs anyway :). Oh, good catch. I'm sorry I didn't update the comments too. Oh man, you're right though: I hadn't considered that it will create a memory leak for all older programs using SDL_DropEvents. Wow, writing a C library is so hard! How do you ever prevent this problem when updating an interface? (I'm coming from a mostly C++ world.) Is the right thing to instead introduce like a new SDL_DropEventWithSource and deprecate SDL_DropEvent? And then in the next release remove the old one? That way people will get a compile error and be forced to fix it, at least, and in the process of fixing it they can read about the new field. How do you normally proceed with something like this? I'll upload the corrected comments, but let me know if there is a better overall approach (maybe like I described above). Thanks! Created attachment 2314 [details]
Updated Patch to add sourceApplication to DropEvent: Fixes comments.
This patch fixes the comment to note that sourceApplication may be null if source is unknown.
Comment on attachment 2314 [details]
Updated Patch to add sourceApplication to DropEvent: Fixes comments.
Also, one more question, is there any style guide regarding 80char limits? The only lines that I modified that go over 80 chars are the actual SDL_SendDropFile() calls in /src/video/uikit/SDL_uikitappdelegate.m. Should I split those lines at the comma? I wasn't sure because the function signature is like 143 characters.
(In reply to Nathan Daly from comment #6) > How do you normally proceed with something like this? Backward incompatible changes are mostly prevented by adding a hint to explicitly activate a new behavior. But this might not be useful here because some systems add the drop event before the hint could be set from the application. Friendly Ping. I think barring any other decision about how to handle the memory leak (such as adding a new struct gated by a compile-time flag and deprecating the old one until it can be deleted in the future, forcing a compile-error) this is all set to be merged in. Thanks! :) Please let me know if you would like me to do anything different regarding the new memory leak. :) I think we're going to hold off on this for 2.0.4 (which hopefully is about to be finished) and deal with it in 2.0.5. --ryan. Okay sounds great. Thanks Ryan! (In reply to Ryan C. Gordon from comment #11) > I think we're going to hold off on this for 2.0.4 (which hopefully is about > to be finished) and deal with it in 2.0.5. > > --ryan. Looks like 2.0.4 was released successfully! :) Congrats. Did you still want to look at merging this in? Thanks! Friendly ping! :) Can you update this patch to work with the current SDL source? Created attachment 2762 [details]
Updated patch -- merged in changes since 2016-02, including new SendDropText and SendDropComplete.
I've updated the patch to be compatible with the new code, including the new functions SDL_SendDropText SDL_SendDropComplete.
There is one remaining question I have, and I left it as a TODO in the code:
What should we do for those two new functions. Should I also go through and add the sourceApplication to those? i don't know where it would come from, or if it's even meaningful (for now I have it sending NULL for those). If we do decide to do this, can we follow it up with a later patch?
Thanks!
Sorry it took me so long to get back to this!! :)
We can make this kind of API change in SDL 2.1. We could also potentially keep a string pool of application sources in the iOS video driver and have the event point at those strings, so it wouldn't need to be freed. Do you know how long those strings are? If they're relatively short, we can put them into the event directly so we don't need a separate string pointer. No sorry, I don't have any idea how long these strings are. It's been years since I was playing with this. :/ The pool could also make sense. |