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

-- 
GNU/Linux. It's not the software that's free, it's you.