[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: Shell32 File Property Dialog
From:       Andreas Mohr <andi () rhlx01 ! fht-esslingen ! de>
Date:       2005-10-30 7:48:28
Message-ID: 20051030074828.GA3098 () rhlx01 ! fht-esslingen ! de
[Download RAW message or body]

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.


[prev in list] [next in list] [prev in thread] [next in thread]