Re: [PATCH] Add search functionality to WINE regedit

[Mike Hearn]

Jan Schiefer wrote:
> It's already fully cleaned up and ready to go! I know other people hate 
> my coding style... :)

Awww, it's not hate, you just need to be careful about how the code is 
laid out and what features you use. You'll need to fix this patch in a 
few places so here are some comments:

It's usually a good idea to submit the backend *first* and then the 
frontend, or better .. have them done all at once. Patches are supposed 
to be standalone, not stuff that'll only start working later (in the 
general case). For now I'd combine the two patches.
> - GROUPBOX        "Look at",IDC_FIND_GROUPBOX, 5, 25, 194, 46, BS_GROUPBOX
> - CHECKBOX	    "Keys", IDC_CHECKBOX_KEYS, 10, 36, 60, 10

Try and keep alignment consistent, otherwise there's just extra visual 
noise for no good reason!
> +LRESULT CALLBACK findedit_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam) {
> +if(uMsg != 193) { // if uMsg == 193, tmpLen is always 0!
> +int tmpLen = SendMessage(hWnd, EM_LINELENGTH, 0, 0);
> +if(tmpLen != searchStringLen) {
> +if(tmpLen > 0)EnableWindow(hFindButton, TRUE);
> +else EnableWindow(hFindButton, FALSE);
> +searchStringLen = tmpLen;
> +}
> +}
> +return CallWindowProc ((WNDPROC) prevWndProcEdit, hWnd, uMsg, wParam, lParam);
> +}

I'm afraid that unless my mail client is playing up that type of code 
isn't acceptable. You need to use indenting! Also please try and space 
stuff out, eg rather than:  if(tmpLen > 0)EnableWindow use if (tmpLen > 
0) EnableWindow(....);
> -
>
> +INT_PTR CALLBACK searchingdlg_proc(HWND hWnd, UINT uMsg, WPARAM wParam,LPARAM lParam) {
> - switch(uMsg) {
> -
>    case WM_INITDIALOG:
> - //SearchRegistryRecrusively("",HKEY_CLASSES_ROOT,TRUE);

No C++/C99 style comments unfortunately. Also please don't submit 
patches that have code commented out for no obvious reason.
> -
>    break;
> - case WM_COMMAND:
> -
>   	    switch(LOWORD(wParam)) {
> -
>    case IDCANCEL:
> -
>    HeapFree(GetProcessHeap(),0,searchString);
> -
>        EndDialog(hWnd, 0);
> -
>        return(TRUE);
> -
>    break;
> - }

Indenting is good, but indenting consistently is even better!
> - break;
> - }
> -
>
> +return(FALSE);
> +}
> -
>
> +INT_PTR CALLBACK finddlg_proc(HWND hWnd, UINT uMsg, WPARAM wParam,LPARAM lParam) {
> -
> - switch(uMsg) {
> -
>    case WM_INITDIALOG:
> -
>        hFindEdit = GetDlgItem(hWnd, IDC_FINDEDIT);
> -
>    hFindButton = GetDlgItem(hWnd, IDOK);
> -
>    prevWndProcEdit = SetWindowLongPtr (hFindEdit, GWLP_WNDPROC, (LONG_PTR) findedit_proc);
> -
>    EnableWindow(hFindButton, FALSE);      
> -
>    CheckDlgButton( hWnd, IDC_CHECKBOX_KEYS, BST_CHECKED );
> -
>        CheckDlgButton( hWnd, IDC_CHECKBOX_VALUES, BST_CHECKED );
> -
>    CheckDlgButton( hWnd, IDC_CHECKBOX_DATA, BST_CHECKED );
> -
>        return(TRUE);

I think you may be mixing tabs and spaces or something here. Also making 
return look like a function is rather non-standard for C, use it as a 
statement: return TRUE;
> - break;
> -
>    case WM_COMMAND:
> -
>        switch(LOWORD(wParam)) {
> -
>        case IDOK:		
> -
> searchString = (LPTSTR)HeapAlloc(GetProcessHeap(),0,searchStringLen + 1);
> -
> GetWindowText(hFindEdit, searchString, searchStringLen + 1 );
> -
> -
> searchForKeys = (IsDlgButtonChecked( hWnd, IDC_CHECKBOX_KEYS ) == BST_CHECKED);
> -
> searchForValues = (IsDlgButtonChecked( hWnd, IDC_CHECKBOX_VALUES ) == BST_CHECKED);
> -
> searchForData = (IsDlgButtonChecked( hWnd, IDC_CHECKBOX_DATA ) == BST_CHECKED);
> -
> searchMatchWholeString = (IsDlgButtonChecked( hWnd, IDC_CHECKBOX_MATCHSTRING ) == BST_CHECKED);

If you're going to fall through please put a comment like /* fall 
through */ otherwise it looks an awful lot like a mistake.

thanks -mike