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

[Toru TSUNEYOSHI]

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

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.

w32.h_w32.c_emacs.c.diff
Description: Binary data

NEWS
Description: Binary data