Re: Shell32 File Property Dialog

[Andreas Mohr]

Hi,

On Sat, Oct 29, 2005 at 08:59:51PM +0200, Johannes Anderwald wrote:
> This patch adds file property dialog to shell32
> - LTEXT "Type of file:", 14004, 10, 30, 50, 10

Space missing??
> - LTEXT "Modied: ", 14016, 10, 90, 45, 10

"Modified: "
>          FIXME("Unhandled Verb %xl\n",LOWORD(lpcmi->lpVerb));

What kind of format specifier is that supposed to be?
I don't know that one...
Maybe use %p instead? (or %lx??)
>  HWND hDlgCtrl = GetDlgItem(hwndDlg, 14005);
>
>  if (filext == NULL || hDlgCtrl == NULL)
>      return FALSE;

Wasteful invocation of GetDlgItem() here, although the failure check is
a bit "prettier" that way...
>  result = RegEnumValueW(hKey,0, name, &lname, NULL, NULL, (LPBYTE)value, &lvalue);

space missing after hkey ;)
> BOOL 
> SHFileGeneralGetFileTimeString(LPFILETIME lpFileTime, WCHAR * lpResult)
> {
>     FILETIME ft;
>     SYSTEMTIME dt;
>     WORD wYear;
>     WCHAR wFormat[] = {'%','0','2','d','/','%','0','2','d','/','%','0','4','d',' ',' ','%','0','2','d',':','%','0','2','u',0};

static const WCHAR
> TRACE("result %s \n",debug_strw(lpResult));

Superfluous space.
>     WARN("failed to open file \n");
>     WARN("GetFileTime failed \n");
>     TRACE("hDlgCtrl %x text %s \n",hDlgCtrl, debug_strw(resultstr));
>     WARN("failed to open file \n");
>     WARN("GetFileSize failed \n");

Dito (I prefer Wine to be smaller rather than the error message to be a bit
"more readable" ;).
(and probably more instances of superfluous space in this patch)
> if (GetFileSize(hFile, NULL) == 0xFFFFFFFF)
> {
>     CloseHandle(hFile);
>     return FALSE;
> }
>
> dwFileLo = GetFileSize(hFile, &dwFileHi);
> CloseHandle(hFile);
>
> if (dwFileLo == 0xFFFFFFFF && GetLastError() != NO_ERROR) 
>     return FALSE;

This whole check sounds very weird.
Why are you checking with NULL hiword when there might be a > 4GB file?
(and to make it worse, even one with e.g. a size of 0x12345678FFFFFFFF !!!!!!)
And directly after that even doing a full large file check **again**?
Not to mention that you're not using the INVALID_FILE_SIZE macro that I really,
really think should be used since it's been created *exactly* for this purpose
(and for the even better purpose of *never* managing to write/edit/delete a
0xFFFFFF or 0xFFFFFFF instead...)
>     FIXME("directory / drive resources dont exist yet \n");

' missing ;)

Sorry for this VERY anal review (it's got to be my most thorough WP review),
and thanks very much for this large patch :)

Andreas

Shell32 File Property Dialog

[Johannes Anderwald]

Andreas Mohr wrote:
> >         FIXME("Unhandled Verb %xl\n",LOWORD(lpcmi->lpVerb));
>
> What kind of format specifier is that supposed to be?
> I don't know that one...
> Maybe use %p instead? (or %lx??)

This statement should be %lx. However, this statement is _not_ part of 
my patch.
> > if (GetFileSize(hFile, NULL) == 0xFFFFFFFF)
> >    {
> >        CloseHandle(hFile);
> >        return FALSE;
> >    }
> >
> > dwFileLo = GetFileSize(hFile, &dwFileHi);
> >    CloseHandle(hFile);
> >
> > if (dwFileLo == 0xFFFFFFFF && GetLastError() != NO_ERROR) 
> >        return FALSE;
>
> This whole check sounds very weird.
> Why are you checking with NULL hiword when there might be a > 4GB file?
> (and to make it worse, even one with e.g. a size of 0x12345678FFFFFFFF !!!!!!)
> And directly after that even doing a full large file check **again**?
> Not to mention that you're not using the INVALID_FILE_SIZE macro that I really,
> really think should be used since it's been created *exactly* for this purpose
> (and for the even better purpose of *never* managing to write/edit/delete a
> 0xFFFFFF or 0xFFFFFFF instead...)

I agree that the INVALID_FILE_SIZE should be used. However, 
INVALID_FILE_SIZE macro is exactly 0xFFFFFFFF. Furthermore, this code 
does work with file sizes of 0x12345678FFFFFFFF. Have a look at the MSDN 
documentation[1]. Alternatively, GetFileSizeEx could be used.
> Andreas

[1] 
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/fs/g...

regards,

Shell32 File Property Dialog

[Andreas Mohr]

Hi,

On Wed, Nov 02, 2005 at 01:15:32AM +0100, Johannes Anderwald wrote:
> Andreas Mohr wrote:
> > >        FIXME("Unhandled Verb %xl\n",LOWORD(lpcmi->lpVerb));
> >
> > What kind of format specifier is that supposed to be?
> > I don't know that one...
> > Maybe use %p instead? (or %lx??)
>
> This statement should be %lx. However, this statement is _not_ part of 
> my patch.

Doh, sorry, didn't realize that.
> > > dwFileLo = GetFileSize(hFile, &dwFileHi);
> > >   CloseHandle(hFile);
> > >
> > > if (dwFileLo == 0xFFFFFFFF && GetLastError() != NO_ERROR) 
> > >       return FALSE;
> >
> > This whole check sounds very weird.
> > Why are you checking with NULL hiword when there might be a > 4GB file?
> > (and to make it worse, even one with e.g. a size of 0x12345678FFFFFFFF 
> > !!!!!!)
> > And directly after that even doing a full large file check **again**?
> > Not to mention that you're not using the INVALID_FILE_SIZE macro that I 
> > really,
> > really think should be used since it's been created *exactly* for this 
> > purpose
> > (and for the even better purpose of *never* managing to write/edit/delete 
> > a
> > 0xFFFFFF or 0xFFFFFFF instead...)
>
> I agree that the INVALID_FILE_SIZE should be used. However, 
> INVALID_FILE_SIZE macro is exactly 0xFFFFFFFF. Furthermore, this code 
> does work with file sizes of 0x12345678FFFFFFFF. Have a look at the MSDN 
> documentation[1]. Alternatively, GetFileSizeEx could be used.

My point was that using INVALID_FILE_SIZE as intended by the system *ensures*
that you may never potentially mis-write or mis-modify 0xffffffff.

And yes, I had a look at the MSDN page, incidentially that's exactly why
I was commenting on it since I knew how horrible GetFileSize() is...
And it appears your code was (almost) right indeed, minus the problem that
you're calling GetLastError() after CloseHandle() (which will also modify
last error on failure).
But you probably intended to call CloseHandle() before the check in order
to avoid an extra CloseHandle() in the code...

Argh, whoever doesn't hate this particular API please raise your hands now...

Thanks for your work!

Andreas