From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#3468: drag and drop text Date: Sun, 29 Sep 2024 10:19:48 +0300 Message-ID: <86wmiv3p0r.fsf@gnu.org> References: <20090604070321.177690@gmx.net> <8ba4e567-550e-4ac2-96f4-c6f7bacd78d0@imayhem.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33839"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 3468@debbugs.gnu.org To: Cecilio Pardo Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Sep 29 09:20:56 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1suoEi-0008d5-Bj for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 29 Sep 2024 09:20:56 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1suoEK-0006G9-0Y; Sun, 29 Sep 2024 03:20:32 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1suoEJ-0006G0-6u for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 03:20:31 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1suoEI-0007Ca-UU for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 03:20:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=References:In-Reply-To:From:Date:To:Subject; bh=1j/5yWI6AVQ0M3WzOqg9XSbmk8oElsyN2ttq7cSJicM=; b=hm0fKuYI+29544FwUuHzK5ogwYVoDBqWuP0MzWkIxlbymnAP6/vz9EmT7IgpAAPL7tQ6rhXeM6caGQNLAz0WgujNtyJ1Uf04NetQezZNL0Gg77T2Ce9hUeC9rbg/Djy6xzMKMuLsuqJNslclOoVBG+wjYd6cB4NQjkgWuz+cJ8nNKMD2BB0JfuKl3aXjFFR9J500AIjfLzaCY2RveS/gPpLzXaKE9dI5S/l3nIoG5X2PYKFaEv9/tkyWHA23Oqj41e/jbVAacACWpkbgeeSn4k1K50lBs9T4i8Y6OWo7VR3PT588uFB+4YvmvpJPAHJbOYNMGERslvoQWFRAS+mLDw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1suoEn-0002xl-SK for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 03:21:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 29 Sep 2024 07:21:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 3468 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: help Original-Received: via spool by 3468-submit@debbugs.gnu.org id=B3468.172759443610896 (code B ref 3468); Sun, 29 Sep 2024 07:21:01 +0000 Original-Received: (at 3468) by debbugs.gnu.org; 29 Sep 2024 07:20:36 +0000 Original-Received: from localhost ([127.0.0.1]:37924 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1suoEM-0002nk-85 for submit@debbugs.gnu.org; Sun, 29 Sep 2024 03:20:36 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:42652) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1suoEJ-0002mm-OJ for 3468@debbugs.gnu.org; Sun, 29 Sep 2024 03:20:33 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1suoDi-0006sF-04; Sun, 29 Sep 2024 03:19:54 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=1j/5yWI6AVQ0M3WzOqg9XSbmk8oElsyN2ttq7cSJicM=; b=ccdfxGXXXsJ5 ECaVGEgIYI1d5+8HTOFLNVUQ4m/aOzm6dVBgfQJyGTPzYhKoVVdaGIgQlG1g6y2w+XE84gV4FkbX6 tPGB4cHuE1merjGUmZnlEGk7UD0ZhCmELpJpoW6DYJO8da+QyhValsZg+cQWweG1M04FgM/XylV9Q xC2++y6VdOFGAXmtwKS0NXNN6VYIbC3/KLRSb69QvfVc04VNGPWwSyBZ99JirNttSud7MCg9YqEX9 iPKo7DDhHxOOLmpjAV1z6NYi6tWwl1DyRRN2a+G7FC1ErGvtreSWS0l5OMxpefOLece3fBX+3elUn 342p10O3rNKu1zQcz5bYwg==; In-Reply-To: <8ba4e567-550e-4ac2-96f4-c6f7bacd78d0@imayhem.com> (message from Cecilio Pardo on Sat, 28 Sep 2024 23:52:45 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:292617 Archived-At: > Date: Sat, 28 Sep 2024 23:52:45 +0200 > From: Cecilio Pardo > > This patch implements drag-and-drop for w32 for files and text, using C, > not C++. > This should work from Windows 95, but I can't test it. Because of this, > the prior implementation with WM_DROPFILES has been removed with '#if > 0', and can probably be completely removed. I think we should remove those "#if 0" parts, yes. If and when someone is able to test this on Windows 9X, they will report problems they encounter, and we can take it from there. The removed code is available in Git, so it is not lost. > Text is inserted at point position, not at drop position. I don't know > how this works on other platforms. Po Lu, any comments to this particular aspect? I have some comments to the code below. > Implement dnd with IDropTarget This should explicitly mention MS-Windows. > * lisp/term/w32-win.el (w32-drag-n-drop): changed to handle files or strings > * src/w32fns.c > (process_dropfiles): new, convert DROPFILES struct to array of strings > (w32_createwindow): assign an IDropTarget to each new frame > (w32_name_of_message): added new messages > (w32_msg_pump): Changed CoInitialize to OleInitialize, needed by the > drag-n-drop functions > (w32_wnd_proc): new struct w32_drop_target, and w32_drop_target_* > functions to implement the IDropTarget interface > * src/w32term.c (w32_read_socket): handle WM_EMACS_DROPFILES, > WM_EMACS_DROPSTRING, skip WM_EMACS_DROPFILES > * src/w32term.h (): add WM_EMACS_DROPFILES, WM_EMACS_DROPSTRING Some of these lines are too long (try limiting to 64 columns). Also, descriptions of changes should be complete sentences: start with a capital letter and end in a period, like this: (w32_createwindow): Assign an IDropTarget to each new frame. > (defun w32-drag-n-drop (event &optional new-frame) > - "Edit the files listed in the drag-n-drop EVENT. > -Switch to a buffer editing the last file dropped." > + "If the drag-n-drop EVENT is for a file or files, edit those > +files. Switch to a buffer editing the last file dropped. The first line of a doc string should be a single complete sentence. In this case, I suggest: Perform drag-n-drop action according to data in EVENT. If EVENT is for one or more files, visit those files in corresponding buffers, and switch to the buffer that visits the last dropped file. If EVENT is for text, insert that text at point into the buffer shown in the window that is the target of the drop; if that buffer is read-only, add the dropped text to kill-ring. If the optional argument NEW-FRAME is non-nil, perform the drag-n-drop action in a newly-created frame using its selected-window and that window's buffer. > + (if (stringp arg) > + (dnd-insert-text window 'copy arg) > + (dnd-handle-multiple-urls > + window > + (mapcar #'w32-dropped-file-to-url arg) > + 'private))))) Please add a comment here saying that the payload in the EVENT should be either a string (meaning the text to drop) or a list of strings (meaning names of files to drop). This is important because a file name is also a string, so it is not obvious how we distinguish between the two possible payloads. > +/* From the DROPFILES struct, extract the list of filenames. Returns a > + NULL terminated malloc array of malloc strings that should be freed ^^^^^^ ^^^^^^ "malloced" > +static char ** > +process_dropfiles (DROPFILES *files) > +{ > + char *start_of_files = (char*)files + files->pFiles; ^^^^^^^^^^^^ Style: "(char *) files". > + int count = 0; > + char filename[MAX_PATH]; ^^^^^^^^ This should be MAX_UTF8_PATH, since filename[] is UTF-8 encoded. > + if (files->fWide) What determines whether we get "wide" file names or ANSI file names? We want to get "wide" file names where possible, but I don't see where we request that? > + { > + WCHAR *p = (WCHAR*)start_of_files; ^^^^^^^^^^^^^^^^^^^^^^ "(WCHAR *) start_of_files". > + p = (WCHAR*)start_of_files; Likewise. > + for ( int i = 0; *p; p += wcslen (p) + 1, i++) ^^ Style: no space there. > + filenames = malloc ((count+1) * sizeof (char*)); You call malloc here, but the function which frees uses xfree. We should probably call xmalloc here, but xmalloc could signal an error, and this code runs in a non-main thread, doesn't it? If so, I think the function which frees needs to call 'free', nor 'xfree'. And we need to handle the case malloc returns NULL. > + filenames [i] = xstrdup (filename); For the same reason, we cannot call xstrdup here, and neither can we call strdup (because it comes from MSVCRT, and will call a different version of malloc). We need to provide a private version of strdup. But see below about moving all this processing to the main thread. Also, "filenames[i]", without the space before the brackets. > + struct w32_drop_target *target = (struct w32_drop_target *)This; > + xfree( target->i_drop_target.lpVtbl ); > + xfree( target ); ^^^^ Style: missing and extra space again. > +static HRESULT STDMETHODCALLTYPE > +w32_drop_target_DragEnter( IDropTarget *This, IDataObject *pDataObj, DWORD grfKeyState, POINTL pt, DWORD *pdwEffect) Please break this long list of arguments into several lines. Also, our style is to write w32_drop_target_DragEnter (IDropTarget *This, That is, one space between the function name and the opening parenthesis, and no space between the parenthesis and the first argument. > +static HRESULT STDMETHODCALLTYPE > +w32_drop_target_DragOver ( IDropTarget *This, DWORD grfKeyState, POINTL pt, DWORD *pdwEffect) Likewise here (and elsewhere in the patch). > + formatetc.cfFormat = CF_UNICODETEXT; > + if (SUCCEEDED (IDataObject_GetData (pDataObj, &formatetc, &stgmedium))) > + { > + if (stgmedium.tymed == TYMED_HGLOBAL) > + { > + WCHAR *text = (WCHAR*)GlobalLock (stgmedium.hGlobal); > + Lisp_Object text_string = from_unicode_buffer (text); > + char *utf8 = xstrdup (SSDATA (ENCODE_UTF_8 (text_string))); > + my_post_msg (&msg, target->hwnd, WM_EMACS_DROPSTRING, > + 0, (LPARAM)utf8 ); > + GlobalUnlock (stgmedium.hGlobal); > + } > + ReleaseStgMedium (&stgmedium); > + return S_OK; > + } If this runs in a non-main thread, then we cannot do this: calling functions that manipulate Lisp data in non-main threads is a no-no. Can we instead delegate the actual processing to where WM_EMACS_DROPSTRING is processed? If not, why not? To tell the truth, I'd be happier if we could do the same with WM_EMACS_DROPFILES, i.e. convert the file names and allocate memory for them in the main thread: that would remove all the issues mentioned above with xmalloc, xstrdup, etc., and be in general much safer. Also, there's no need to call ENCODE_UTF_8, since from_unicode_buffer already returns the text in the internal representation. You are basically decoding strings from UTF-16, then encode them in UTF-8, only to decode them again in the main thread. (This will also be avoided if we do everything in the main thread.) > + formatetc.cfFormat = CF_TEXT; > + if (SUCCEEDED (IDataObject_GetData (pDataObj, &formatetc, &stgmedium))) > + { > + if (stgmedium.tymed == TYMED_HGLOBAL) > + { > + char *text = (char*)GlobalLock (stgmedium.hGlobal); > + > + int l = strlen (text); > + WCHAR *text_utf16 = xmalloc (sizeof (WCHAR) * l + 1 );; > + if (MultiByteToWideChar (CP_ACP, 0, text, l, text_utf16, l + 1)) We cannot call MultiByteToWideChar directly: it will prevent Emacs from running on Windows 9X, where that function is available only after loading a special DLL. We need to call this function via a function pointer, pMultiByteToWideChar. > + { > + Lisp_Object text_string = from_unicode_buffer (text_utf16); > + char *utf8 = xstrdup (SSDATA (ENCODE_UTF_8 (text_string))); > + my_post_msg (&msg, target->hwnd, WM_EMACS_DROPSTRING, > + 0, (LPARAM)utf8 ); This is a very convoluted way of converting ANSI encoded string to UTF-8. I would simply use DECODE_SYSTEM here (subject to the same restriction regarding doing that in a non-main thread). > @@ -2548,7 +2720,23 @@ w32_createwindow (struct frame *f, int *coords) > SetWindowLong (hwnd, WND_BACKGROUND_INDEX, FRAME_BACKGROUND_PIXEL (f)); > > /* Enable drag-n-drop. */ > + struct w32_drop_target *drop_target = xmalloc (sizeof (struct w32_drop_target)); > + drop_target-> hwnd = hwnd; > + > + IDropTargetVtbl *vtbl = xmalloc (sizeof (IDropTargetVtbl)); ^^^^^^^ w32_createwindow runs in the input thread, so it cannot safely call xmalloc. > case WM_EMACS_CREATEWINDOW: > - /* Initialize COM for this window. Even though we don't use it, > - some third party shell extensions can cause it to be used in > + /* Initialize COM for this window. Needed for RegisterDragDrop. ^^ Two spaces between sentences, please. > + case WM_EMACS_DROPSTRING: > + f = w32_window_to_frame (dpyinfo, msg.msg.hwnd); > + if (!f) > + break; > + XSETFRAME (inev.frame_or_window, f); > + inev.kind = DRAG_N_DROP_EVENT; > + inev.code = 0; > + inev.timestamp = msg.msg.time; > + inev.modifiers = msg.dwModifiers; > + ScreenToClient (msg.msg.hwnd, &msg.msg.pt); > + XSETINT (inev.x, msg.msg.pt.x); > + XSETINT (inev.y, msg.msg.pt.y); > + inev.arg = make_string ((char*)msg.msg.lParam, strlen((char*)msg.msg.lParam)); Since we are (hopefully) going to move the prcessing and decoding of the dropped string to this place, just decoding it from UTF-18 or ANSI (via DECODE_SYSTEM) should be enough. > + xfree ((void*)msg.msg.lParam); This may need to be 'free', see above. > + case WM_EMACS_DROPFILES: > + { > + f = w32_window_to_frame (dpyinfo, msg.msg.hwnd); > + if (!f) > + break; > + XSETFRAME (inev.frame_or_window, f); > + inev.kind = DRAG_N_DROP_EVENT; > + inev.code = 0; > + inev.timestamp = msg.msg.time; > + inev.modifiers = msg.dwModifiers; > + ScreenToClient (msg.msg.hwnd, &msg.msg.pt); > + XSETINT (inev.x, msg.msg.pt.x); > + XSETINT (inev.y, msg.msg.pt.y); > + > + Lisp_Object files = Qnil; > + char **filenames = (char**)msg.msg.lParam; > + for (int n = 0; filenames[n]; n++ ) > + { > + files = Fcons (DECODE_FILE (build_string (filenames[n])), files ); > + xfree (filenames[n]); > + } > + xfree (filenames); Similarly here. Finally, we need a NEWS entry (near the end of NEWS) announcing this new capability. Thanks!