Re: patch about moving file (or directory) to the Recycle Bin on Windows NT series

["Toru TSUNEYOSHI" <t_tuneyosi@hotmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4890 bytes --]

Thanks, everyone, especially Eli Zaretskii.

Just Now I checked new suggestion from Stefan Monnier.
But I have already fixed as follows, I will send it.

----- Original Message ----- 
From: "Eli Zaretskii" <eliz@gnu.org>
To: "Toru TSUNEYOSHI" <t_tuneyosi@hotmail.com>
Cc: <emacs-devel@gnu.org>
Sent: Wednesday, April 23, 2008 3:19 AM
Subject: Re: patch about moving file (or directory) to the Recycle Bin on Windows NT series

> > From: "Toru TSUNEYOSHI" <t_tuneyosi@hotmail.com>
> > Date: Tue, 22 Apr 2008 06:32:12 +0900
> > 
> > I made a revised edition (including w32.h).
> 
> Thank you.
> 
> I think this contribution is large enough to require legal papers, but
> I'll let Stefan and Yidong decide that.
> 

You are welcome.

> > +#ifdef W32_SYS_UNLINK_USE_SHELLAPI
> > +extern void syms_of_w32 (void);
> > +#endif /* W32_SYS_UNLINK_USE_SHELLAPI */
> 
> If we are going to support this feature, we don't need this
> conditional compilation, so please remove the #ifdef's (here and
> everywhere else).
> 

I removed all of them.

> > +  if (w32_sys_unlink_use_shellapi)
> > +    {
> > +      /* Each file name must be terminated by a single NULL
> > + character. An additional NULL character must be appended to the
> > + end of the final name to indicate the end of pFrom. */
> > +      TCHAR tmp_path[MAX_PATH + 1 + 1];
> 
> AFAIU from the Microsoft documentation, there's no need for the second
> "+1", as MAX_PATH already accounts for one terminating null character.
> 

Oops. You are right. I fixed it.

> > +      SHFILEOPSTRUCT stFileOp;
> > +
> > +      path = map_w32_filename (path, NULL);
> 
> MSDN advises to use only absolute file names with SHFileOperation,
> otherwise they say that recycling will not work (see
> http://msdn2.microsoft.com/en-us/library/bb759795(VS.85).aspx for the
> details). So I think we need to make the file name absolute here.
> 

Yes, I added statements for it.

> > +      /* On Unix, unlink works without write permission. */
> > +      _chmod (path, 0666);
> 
> Can this somehow leave the file writable without deleting it, if the
> code is interrupted before it gets to undo this _chmod call?  If so,
> we need to guard against that somehow.
> 

I don't know what to do about it. So, I do nothing.

> > +      ZeroMemory(&stFileOp, sizeof(SHFILEOPSTRUCT));
> > +      stFileOp.hwnd = HWND_DESKTOP;
> > +      stFileOp.wFunc = FO_DELETE;
> > +      stFileOp.pFrom = tmp_path;
> > +      stFileOp.pTo = NULL;
> > +      stFileOp.fFlags = FOF_ALLOWUNDO | FOF_NOCONFIRMATION | FOF_SILENT;
> 
> Do we want to use the FOF_NO_CONNECTED_ELEMENTS flag here?  Deleting
> connected files, which is the default with shell API, is not what
> happens with the normal unlink, is it?  So it might surprise the user
> if we do that when using the shell API.
> 

OK, I added it. And, I build Emacs by MS VC++ 6.0. SHELLAPI.H in MS VC++
6.0 don't include `FOF_NO_CONNECTED_ELEMENTS', so I added it to w32.c.

> How about FOF_NOERRORUI? do we want it?  I understand that without it,
> a dialog will be presented to the user in case of any errors during
> the deletion.
> 

Yes, I added it.

> > +      /* value returned by SHFileOperation() must match value returned by _unlink() */
> > +      return (SHFileOperation(&stFileOp) == 0 ? 0 : -1);
> 
> Do we want to set errno here, at least for some popular reasons that
> fail the deletion, using the DE_* error codes documented on the MSDN?
> The caller of this function may wish to look at errno and provide
> diagnostics.
> 

I don't know what to do about it, because the caller of this function
may assume that _unlink() or _rmdir() returns 0 or -1 in the existing
code.

> > +void
> > +syms_of_w32 ()
> > +{
> > +  DEFVAR_BOOL ("w32-sys-unlink-use-shellapi", &w32_sys_unlink_use_shellapi,
> > +        "Non-nil means using shellapi for sys_unlink(), sys_rmdir().");
> > +  w32_sys_unlink_use_shellapi = 1;
> > +}
> 
> Please use a more descriptive name, such as w32-delete-to-recycle-bin
> or something similar.  The point is that the name should tell a user
> what this option will do, not which API it will use.  (I imagine that
> many Emacs users don't even know what is the shell API.)
> 
> Also, please make this option off by default.
> 

I fixed it as you wrote.

> In addition, we need an entry for NEWS about this new feature.  Could
> you please write it?
> 

Yes, I wrote it. But I don't know document format for NEWS, so I
followed examples in etc/NEWS.

> Finally, using this feature requires to add -lshellapi to the linker
> switches in nt/?make.defs, right?  Or is that linked in by default?
> 

It already existed.

There is variable SHELL32 in nt/nmake.defs.
SHELL32  = shell32.lib

There is variable SHELL32 in nt/gmake.defs
SHELL32  = -lshell32

And, varialbe LIBS in src/makefile.w32-in includes variable SHELL32.

> Thanks again for working on this.
> 

Thanks a lot for your helping.

[-- Attachment #2: w32.h_w32.c_emacs.c.diff --]
[-- Type: application/octet-stream, Size: 4002 bytes --]

--- src/w32.h.original	2008-01-10 21:16:16.000000000 +0900
+++ src/w32.h	2008-04-24 00:44:33.769063800 +0900
@@ -129,6 +129,7 @@
 
 extern void init_ntproc (void);
 extern void term_ntproc (void);
+extern void syms_of_w32 (void);
 extern void globals_of_w32 (void);
 extern void syms_of_w32term (void);
 extern void syms_of_w32fns (void);
--- src/w32.c.original	2008-02-23 22:49:09.000000000 +0900
+++ src/w32.c	2008-04-24 00:42:22.761991200 +0900
@@ -75,6 +75,11 @@
 #include <windows.h>
 #include <shlobj.h>
 
+#include <shellapi.h>
+#ifndef FOF_NO_CONNECTED_ELEMENTS
+#define FOF_NO_CONNECTED_ELEMENTS 0x2000 /* don't operate on connected elements. */
+#endif /* FOF_NO_CONNECTED_ELEMENTS */
+
 #ifdef HAVE_SOCKETS	/* TCP connection support, if kernel can do it */
 #include <sys/socket.h>
 #undef socket
@@ -104,6 +109,7 @@
 typedef HRESULT (WINAPI * ShGetFolderPath_fn)
   (IN HWND, IN int, IN HANDLE, IN DWORD, OUT char *);
 
+void syms_of_w32 ();
 void globals_of_w32 ();
 
 extern Lisp_Object Vw32_downcase_file_names;
@@ -111,6 +117,8 @@
 extern Lisp_Object Vw32_get_true_file_attributes;
 extern int w32_num_mouse_buttons;
 
+int w32_delete_to_recycle_bin;
+
 \f
 /*
   Initialization states
@@ -2243,17 +2251,58 @@
 int
 sys_rmdir (const char * path)
 {
-  return _rmdir (map_w32_filename (path, NULL));
+  if (w32_delete_to_recycle_bin)
+    return (sys_unlink (path));
+  else
+    return _rmdir (map_w32_filename (path, NULL));
 }
 
 int
 sys_unlink (const char * path)
 {
-  path = map_w32_filename (path, NULL);
+  if (w32_delete_to_recycle_bin)
+    {
+      SHFILEOPSTRUCT file_op;
+      /* `pFrom' member of struct SHFILEOPSTRUCT:
+	 Each file name must be terminated by a single NULL
+	 character. An additional NULL character must be appended to the
+	 end of the final name to indicate the end of pFrom. */
+      char tmp_path[MAX_PATH + 1];
+      Lisp_Object tmp;
+
+      /* Must use only absolute file names with SHFileOperation(). */
+      tmp = Fexpand_file_name (build_string (path), Qnil);
+
+      path = map_w32_filename (SDATA (tmp), NULL);
+
+      /* On Unix, unlink works without write permission. */
+      _chmod (path, 0666);
+
+      memset (tmp_path, 0, sizeof (tmp_path));
+      strcpy(tmp_path, path);
+
+      memset(&file_op, 0, sizeof (file_op));
+      file_op.hwnd = HWND_DESKTOP;
+      file_op.wFunc = FO_DELETE;
+      file_op.pFrom = tmp_path;
+      file_op.pTo = NULL;
+      file_op.fFlags = FOF_SILENT | FOF_NOCONFIRMATION | FOF_ALLOWUNDO
+	| FOF_NOERRORUI | FOF_NO_CONNECTED_ELEMENTS;
+      file_op.fAnyOperationsAborted = FALSE;
+      file_op.hNameMappings = NULL;
+      file_op.lpszProgressTitle = NULL;
+
+      /* value returned by SHFileOperation() must match value returned by _unlink() */
+      return (SHFileOperation(&file_op) == 0 ? 0 : -1);
+    }
+  else
+    {
+      path = map_w32_filename (path, NULL);
 
-  /* On Unix, unlink works without write permission. */
-  _chmod (path, 0666);
-  return _unlink (path);
+      /* On Unix, unlink works without write permission. */
+      _chmod (path, 0666);
+      return _unlink (path);
+    }
 }
 
 static FILETIME utc_base_ft;
@@ -4160,6 +4209,16 @@
   SetConsoleCtrlHandler(shutdown_handler, TRUE);
 }
 
+void
+syms_of_w32 ()
+{
+  DEFVAR_BOOL ("w32-delete-to-recycle-bin",
+	       &w32_delete_to_recycle_bin,
+	       doc: /* Non-nil means delete file or directory to recycle bin.
+Default is nil, which means just delete it.  */);
+  w32_delete_to_recycle_bin = 0;
+}
+
 /* end of w32.c */
 
 /* arch-tag: 90442dd3-37be-482b-b272-ac752e3049f1
--- src/emacs.c.original	2008-01-10 21:16:14.000000000 +0900
+++ src/emacs.c	2008-04-24 00:43:32.060947200 +0900
@@ -1588,6 +1588,7 @@
       syms_of_vmsproc ();
 #endif /* VMS */
 #ifdef WINDOWSNT
+      syms_of_w32 ();
       syms_of_ntproc ();
 #endif /* WINDOWSNT */
       syms_of_window ();

[-- Attachment #3: NEWS --]
[-- Type: application/octet-stream, Size: 283 bytes --]

** Deleting files to the Recycle Bin is now supported on MS Windows.

Now Emacs can delete files or directories to the Recycle Bin on MS
Windows.
This feature is disabled by default. To enable it, put

  (setq w32-delete-to-recycle-bin t)

in your .emacs or some other startup file.