Update osxgl.m #1664

MarekRatajczak2024 · 2025-08-20T17:06:32Z

Drag and drop for MacOS

pedro-w · 2025-08-20T20:21:45Z

src/macosx/osxgl.m

 #define MINIMUM_HEIGHT 48

+/* by MAREK */
+#define FILE_URL_SIZE 254


Is this used anywhere?

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.

pedro-w · 2025-08-20T20:22:11Z

src/macosx/osxgl.m

+/*
+ * Convert a NSString to a char pointer
+*/
+-(char *)toChar{


Is this used anywhere?

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.

pedro-w · 2025-08-20T20:22:45Z

src/macosx/osxgl.m

+        // Or other types like NSStringPboardType, NSFileContentsPboardType, etc.
+    }
+
+    NSString *cachesDirectory = [NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES) objectAtIndex:0];


Is this variable used?

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.

pedro-w · 2025-08-20T20:29:13Z

src/macosx/osxgl.m

+                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;


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.

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

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.

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.

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.

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:

allegro5/examples/ex_drag_and_drop.c

Line 183 in efb3129

al_free(event.drop.text);

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 · 2025-08-20T20:33:47Z

I was glad to see this. I have added some comments, hopefully helpful

…

On Wed, 20 Aug 2025 at 18:06, Marek Ratajczak ***@***.***> wrote: Drag and drop for MacOS ------------------------------ You can view, comment on, or merge this pull request online at: #1664 Commit Summary - bf862b8 <bf862b8> Update osxgl.m File Changes (1 file <https://github.com/liballeg/allegro5/pull/1664/files>) - *M* src/macosx/osxgl.m <https://github.com/liballeg/allegro5/pull/1664/files#diff-43ba1969dce14d57a6fc4b8214feaab2512999625824d1392fc0223decda63e2> (118) Patch Links: - https://github.com/liballeg/allegro5/pull/1664.patch - https://github.com/liballeg/allegro5/pull/1664.diff — Reply to this email directly, view it on GitHub <#1664>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFJFGZ4L5BWKBMCL7IMNICL3OSTLFAVCNFSM6AAAAACEMED6GCVHI2DSMVQWIX3LMV43ASLTON2WKOZTGMZTQOBZGQ3DKNQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

MarekRatajczak2024 · 2025-08-20T23:18:47Z

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 · 2025-08-22T10:15:41Z

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 · 2025-08-22T12:05:52Z

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 · 2025-08-22T13:23:11Z

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.

Update osxgl.m

bf862b8

pedro-w reviewed Aug 20, 2025

View reviewed changes

Add files via upload

64d32f8

Update osxgl.m #1664

Are you sure you want to change the base?

Update osxgl.m #1664

Uh oh!

Conversation

MarekRatajczak2024 commented Aug 20, 2025

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!

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!

pedro-w commented Aug 20, 2025 via email

Uh oh!

MarekRatajczak2024 commented Aug 20, 2025

Uh oh!

pedro-w commented Aug 22, 2025

Uh oh!

MarekRatajczak2024 commented Aug 22, 2025

Uh oh!

pedro-w commented Aug 22, 2025

Uh oh!

Uh oh!