[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: patch about moving file (or directory) to the Recycle Bin on Windows
From: |
Toru TSUNEYOSHI |
Subject: |
Re: patch about moving file (or directory) to the Recycle Bin on Windows NT series |
Date: |
Thu, 24 Apr 2008 01:45:00 +0900 |
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
Re: patch about moving file (or directory) to the Recycle Bin on Windows NT series, Stefan Monnier, 2008/04/23
Re: patch about moving file (or directory) to the Recycle Bin on Windows NT series, Toru TSUNEYOSHI, 2008/04/26