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