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