Re: notepad patches (search/replace, etc)

[Vitaliy Margolen]

Thursday, June 8, 2006, 6:40:33 PM, Anoni Moose wrote:
> This is my first patch to an open source project... if anyone has any comments/suggestions, please tell me. :)
> These patches add full search, search next, replace, and goto line functionality to notepad. Too bad I missed the 0.9.15 release!
> Changelog:
> * programs/notepad/main.c, programs/notepad/dialog.c,
>       programs/notepad/main.h, programs/notepad/notepad_res.h,
>       programs/notepad/dialog.h:
>     notepad: Add/call functions to load/save globals settings (including font info) to registry.
>                 -Added full Search/Search Next functionality.
>                 -Added full Replace functionality.
>                 -Added full Goto Line functionality.
>                 -Load/Save to registry whether we want to wrap long lines or not.

Please one patch per email. You should combine all of your changes into one patch
if that's one logical change and can be applied by itself. Of course resultant code
should compile and work.
> @@ -638,6 +641,8 @@
>          Globals.hFont=CreateFontIndirect( &lf );
>          Globals.lfFont=lf;
>          SendMessage( Globals.hEdit, WM_SETFONT, (WPARAM)Globals.hFont, (LPARAM)TRUE );
> -
> SETTINGS_SaveSettings();
> -

Please respect indentation. Don't just copy&paste stuff all over the file.
> VOID DIALOG_Search(VOID)
>  {
> -
> -
> if (Globals.find.hwndOwner == NULL) {
>    ZeroMemory(&Globals.find, sizeof(Globals.find));
> -
> }
>    Globals.find.lStructSize      = sizeof(Globals.find);
>    Globals.find.hwndOwner        = Globals.hMainWnd;

Pretty much the same here: don't just insert stuff, indent it properly.
And please no extra blank lines nor needles curly brackets.
> -
> if (Globals.replace.hwndOwner == NULL) {
> -
>   ZeroMemory(&Globals.replace, sizeof(Globals.replace));
> -
> }

Respect indentation and style of the file you changing. In this case 4-spaces
not 2.
> -
> Globals.hFindReplaceDlg = ReplaceText(&Globals.replace);
> -
> assert(Globals.hFindReplaceDlg != 0);
>
> +}

Please don't use assert. Do a proper error checking instead.
> -
> if (result == -1) {     /* text not found. */
> -
>   MessageBoxA(Globals.hEdit, "Cannot find text.", NULL, MB_OK | MB_ICONINFORMATION);
> -
> } else {

You should put all the text into resource files.
> +WCHAR *GrabWindowTextW(HWND hWnd, DWORD *nbytes) {
> -
> -
> static WCHAR *data = NULL;
> -
> DWORD _nbytes = 0;
> -
> -
> if (nbytes == NULL) nbytes = &_nbytes;
> -
> -
> *nbytes = (*nbytes == 0 ? GetWindowTextLengthW(hWnd) + 1 : *nbytes);
> -
> data = HeapAlloc(GetProcessHeap(), 0, (*nbytes) * sizeof(WCHAR));
> -
> GetWindowTextW(hWnd, data, (*nbytes)+1);
> -
> return data;
>
> +}

Why do you need static if you return allocated buffer?
nbytes is misleading - it should be nchars.
(*nbytes)+1 is incorrect. You allocated enough memory for *nbytes only.
> +int SearchText(HWND hWnd, LPFINDREPLACE find, int pos);

Please use windows types so they would work right on 64-bit platforms.
> - "^R", CMD_REPLACE

Native notepad has it as ctrl+H.

Vitaliy Margolen

notepad patches (search/replace, etc)

[Anoni Moose]

Thanks for the input... I guess I've got a lot of fixing up to do. So, all
of the responses I have received, I guess mean that they need to be redone
before being accepted? If they don't get a response like "Accepted", they're
not accepted?

Maybe my strategy of making a bunch of changes and sending them all at once
wasn't too good of an idea. :)

I'll work on them and re-submit later.

On 6/8/06, Vitaliy Margolen  wrote:
> Thursday, June 8, 2006, 6:40:33 PM, Anoni Moose wrote:
> > This is my first patch to an open source project... if anyone has any
>
> comments/suggestions, please tell me. :)
> > These patches add full search, search next, replace, and goto line
>
> functionality to notepad. Too bad I missed the 0.9.15 release!
> > Changelog:
> > - programs/notepad/main.c, programs/notepad/dialog.c,
> >
> > programs/notepad/main.h, programs/notepad/notepad_res.h,
> > programs/notepad/dialog.h:
> > notepad: Add/call functions to load/save globals settings (including
>
> font info) to registry.
> > -Added full Search/Search Next functionality.
> > -Added full Replace functionality.
> > -Added full Goto Line functionality.
> > -Load/Save to registry whether we want to wrap long lines or not.
>
> Please one patch per email. You should combine all of your changes into
> one patch
> if that's one logical change and can be applied by itself. Of course
> resultant code
> should compile and work.
> > @@ -638,6 +641,8 @@
> >          Globals.hFont=CreateFontIndirect( &lf );
> >          Globals.lfFont=lf;
> >          SendMessage( Globals.hEdit, WM_SETFONT, (WPARAM)Globals.hFont,
>
> (LPARAM)TRUE );
> > -
> > SETTINGS_SaveSettings();
> > -
>
> Please respect indentation. Don't just copy&paste stuff all over the file.
> > VOID DIALOG_Search(VOID)
> >  {
> > -
> > -
> > if (Globals.find.hwndOwner == NULL) {
> >    ZeroMemory(&Globals.find, sizeof(Globals.find));
> > -
> > }
> >    Globals.find.lStructSize      = sizeof(Globals.find);
> >    Globals.find.hwndOwner        = Globals.hMainWnd;
>
> Pretty much the same here: don't just insert stuff, indent it properly.
> And please no extra blank lines nor needles curly brackets.
> > -
> > if (Globals.replace.hwndOwner == NULL) {
> > -
> >   ZeroMemory(&Globals.replace, sizeof(Globals.replace));
> > -
> > }
>
> Respect indentation and style of the file you changing. In this case
> 4-spaces
> not 2.
> > -
> > Globals.hFindReplaceDlg = ReplaceText(&Globals.replace);
> > -
> > assert(Globals.hFindReplaceDlg != 0);
> >
> > +}
>
> Please don't use assert. Do a proper error checking instead.
> > -
> > if (result == -1) {     /* text not found. */
> > -
> >   MessageBoxA(Globals.hEdit, "Cannot find text.", NULL, MB_OK |
>
> MB_ICONINFORMATION);
> > -
> > } else {
>
> You should put all the text into resource files.
> > +WCHAR *GrabWindowTextW(HWND hWnd, DWORD *nbytes) {
> > -
> > -
> > static WCHAR *data = NULL;
> > -
> > DWORD _nbytes = 0;
> > -
> > -
> > if (nbytes == NULL) nbytes = &_nbytes;
> > -
> > -
> > *nbytes = (*nbytes == 0 ? GetWindowTextLengthW(hWnd) + 1 :
>
> *nbytes);
> > -
> > data = HeapAlloc(GetProcessHeap(), 0, (*nbytes) * sizeof(WCHAR));
> > -
> > GetWindowTextW(hWnd, data, (*nbytes)+1);
> > -
> > return data;
> >
> > +}
>
> Why do you need static if you return allocated buffer?
> nbytes is misleading - it should be nchars.
> (*nbytes)+1 is incorrect. You allocated enough memory for *nbytes only.
> > +int SearchText(HWND hWnd, LPFINDREPLACE find, int pos);
>
> Please use windows types so they would work right on 64-bit platforms.
> > - "^R", CMD_REPLACE
>
> Native notepad has it as ctrl+H.
>
> Vitaliy Margolen

notepad patches (search/replace, etc)

[Anoni Moose]

Let's try again... I apologize for the bad indentation. It's indented fine
in emacs, which I use; however, when I use "diff -u file1 file2", it gives
me the entire file as a patch. Therefore, I must use "diff -uwB". Emacs must
be screwing it up...

ChangeLog:

* programs/notepad/main.c, programs/notepad/main.h,
  programs/notepad/dialog.c, programs/notepad/dialog.h,
  programs/notepad/notepad_res.h, programs/notepad/En.rc,
  programs/notepad/rsrc.rc:
notepad: Add search, replace, goto line functionality.

notepad patches (search/replace, etc)

[Vitaliy Margolen]

Friday, June 9, 2006, 11:49:44 AM, Anoni Moose wrote:
> Let's try again... I apologize for the bad indentation. It's indented fine in
> emacs, which I use; however, when I use "diff -u file1 file2", it gives me the
> entire file as a patch. Therefore, I must use "diff -uwB". Emacs must be screwing it up...

That will not work. Your patch look really bad with all types of indentation.
You need to use something that doesn't autoindent. Also please do not mix
spaces and tabs - you have that on each line and that's agreed NO-NO.
You either use tabs (8 spaces) or you use all spaces. But not tab & spaces on
the same line.

Vitaliy.

notepad patches (search/replace, etc)

[Marcus Meissner]

On Sat, Jun 10, 2006 at 09:52:50AM -0600, Vitaliy Margolen wrote:
> Friday, June 9, 2006, 11:49:44 AM, Anoni Moose wrote:
> > Let's try again... I apologize for the bad indentation. It's indented fine in
> > emacs, which I use; however, when I use "diff -u file1 file2", it gives me the
> > entire file as a patch. Therefore, I must use "diff -uwB". Emacs must be screwing it up...

Your editor adds DOS linefeeds for some reason.

Ciao, Marcus