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

[Eli Zaretskii]

> From: "Toru TSUNEYOSHI" <address@hidden>
> 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.

> +#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).

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

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

> +      /* 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.

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

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.

> +      /* 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.

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

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

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

Thanks again for working on this.