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.