Skip to content

Conversation

MarekRatajczak2024
Copy link

Drag and drop for MacOS

#define MINIMUM_HEIGHT 48

/* by MAREK */
#define FILE_URL_SIZE 254
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, no, is not used anymore, there was another concept in my mind to send 1 single message and receive the event as a bunch of file names separated by \r\n so exactly like MacOS is doing, but eventually i changed minds and did at exactly like xdnd function for X11 in Allegro 5 is doing, so there is single message for each file name. So that definition is not used anymore. Remove it please.

/*
* Convert a NSString to a char pointer
*/
-(char *)toChar{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in my previous comment, this comes from previous concept, and NSString now is converted directly to char array, no need to use that function. Delete it please.

// Or other types like NSStringPboardType, NSFileContentsPboardType, etc.
}

NSString *cachesDirectory = [NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES) objectAtIndex:0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this variable used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm really sorry, it should be remarked with // like next rows below, what was a part of my tests i needed to divert errout to the file, originally in cachesDirectory, to check how events are received. so those 3 rows:

NSString *cachesDirectory = [NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES) objectAtIndex:0];

// NSString *logPath = @"allegro5.log";
// freopen([logPath cStringUsingEncoding:NSASCIIStringEncoding], "a+", stderr);

can be freely removed.

event.drop.x = (int)mousePos.x;
event.drop.y = (int)mousePos.y;
NSLog(@"Event with file (%d/%d) : %@", n+1, (int)fileURLs.count, filePath);
event.drop.text = (char*) cfileURL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cFileURL will get invalidated when filePath is released, which may happen when fileURLs is released. From the docs I'm not sure if it will happen at a time which could cause a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Allegro docs I think you need to copy cFileURL into allocated memory, which the user program must free.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my thought as well, but I made detail tests, on many files in one drop, and before filePath is released, in the function which takes the event message (which is listening to _al_event_source_emit_event(es, &event), the tile name, so event.drop.text is copied to internal buffer, independent from event.drop.text pointer. It works, because is is not released before the function finishes it's work, so until the very last filename. I will think about that, and maybe that would make sense to declare extra global variable for that, allocate in create_display_fs or create_display_win, and free it in destroy_display(ALLEGRO_DISPLAY* d) what basically I was doing when i created big buffer for all file names, to get independence from fileURLs pointer. But after experiments, eventually I resigned from it, because it is simply working. That's why I asked all of you guys to check it out and make it more safe, if it is not safe enough. But yes, if you like to copy cFileURL into allocated memory, please do that change.
Like always, disadvantage of extra text variable is necessity to copy it, but this is not a big deal. We are dealing with dozens (on top) string up to 254 characters or so (URL path length), not thousand of them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, i'm taking your suggestion seriously, and will do that change in my branch, to test it, even intentionally delaying message receiving, letting this module to smear the fileURLs pointer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, receiving the message sent from that osxgl.m file, is executed in the same thread, so I believe when loop is finished, all is already received and copied into internal buffer - in my case - in my program. But it's right, that could be in different thread, and message can wait for recipient, while loop is already over. Either way, we need to pick them up as soon as possible, as the next batch of files will probably arrive soon. Then even that variable would be modified. As far as I know, sending a message doesn't require waiting for a confirmation of receipt; the message remains in the message buffer. So we don't know if the last message received will be the one received. What we do know is who will receive the message, and there are other features for that, which is why dragged files or their icons with the + symbol are visible against the background of the listening window. Ok, that's what I know, please anyone correct me iff I'm wrong. I mostly base on me experience, and this is example:
https://nextcloud.vurplex.com/s/9GFBqmbRYD7X3pz
The last file name is also taken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you just need to al_malloc each individual dropped file name, and it is the application's responsibility to al_free it; see the ex_drag_and_drop.c for example:

al_free(event.drop.text);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's right, so in next my commit i'm allocating memory, freeing and allocating again what is needed to keep pointers working, to free them completely when display is closing. Yes, it can be replaced with al_malloc and al_free, actually just to save one or 3 lines of code, so instead of freeing those variables when display is closing, rather doing nothing because allegro would do that when its architecture is closing. I used malloc because i'm new in allegro 5, i come from the fading world of allegro4. Just let me know if I should change it again, so change the file in my fork, then pull commit to main branch, or you guys would do that as should be in official branch. Thanks.

@pedro-w
Copy link
Contributor

pedro-w commented Aug 20, 2025 via email

@MarekRatajczak2024
Copy link
Author

OK, i resent that file after all changes suggested. Now pointers are independent, and will exist until next drag n drop operation or display is closed.

@pedro-w
Copy link
Contributor

pedro-w commented Aug 22, 2025

It seems to me that you don't need to track the memory allocated to file names at all. In the X version

allegro5/src/x/xdnd.c

Lines 307 to 314 in efb3129

if (strcmp(name, "text/plain") == 0) {
_send_event(allegro_display, al_cstr_dup(token), false, row, complete);
}
else if (strcmp(name, "text/uri-list") == 0) {
char *filename = _uri_to_filename(al_cstr_dup(token));
_send_event(allegro_display, filename, true, row, complete);
}
else {

Just use the convenience function al_cstr_dup and put it into the event, then the application frees it later.

The stuff with cfileURLs and cfileURLs_n is not needed. What do you think @SiegeLord ?

Also the X code calls _al_event_source_needs_to_generate_event which may need adding here too; if we generate events when there is nothing listening then memory will leak.

@MarekRatajczak2024
Copy link
Author

Yes pedro-w, you are absolutely right, I omitted _al_event_source_needs_to_generate_event intentionally, however I discovered it in xdnd, to make sure it will send event message unconditionally, because I know someone is listening for sure, because it's powering application which is listening, but you are right, not always it must be like that. Very conscious observation, and that conditions should be restoewd, thanks. Regarding cfileURLs and cfileURLs_n: I did this in a homegrown way, assuming that pointers could be destroyed before each new drag and drop session, because if no one received the message, they were probably ignored. The event buffer doesn't exist forever. And since I'm allocating something, I'm cleaning up after myself, and I chose the moment when the display closes. You can, of course, use al_malloc and al_free, and if the Allegro architecture does al_free at the end, great, you can forget about al_free in the display's closing function. However, I wouldn't agree with creating endlessly new pointers and allocating memory for each new filename, especially when the drag and drop procedure is repeated multiple times, knowing that this memory will only be freed when the program closes. So al_free should be executed before sending each new packet. That's my opinion, but I'm sure you are able to select the best way.

@pedro-w
Copy link
Contributor

pedro-w commented Aug 22, 2025

However, I wouldn't agree with creating endlessly new pointers and allocating memory for each new filename, especially when the drag and drop procedure is repeated multiple times, knowing that this memory will only be freed when the program closes.

You're absolutely correct but in this case the API says that Allegro allocates the memory for the filename, and the application frees it (see the example ex_drag_and_drop I posted above )

Obviously if someone writes an app using D&D but doesn't bother to free that string they'll get a memory leak. Tough.

It would be great if Allegro could take this responsibility but I don't think it's possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants