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