First patch: Regedit Multi-String Value

[Hedos]

Hello folks!

I'm new to Wine development, and I've just completed my first patch. I  
hope I could get some advices to make sure I'm heading in the right  
direction.

Just a bit myself now to clear up a few things: I'm a first year college  
student who realized this week-end the Google Summer of Code was back  
again. So, I thought I should definitly give it a try. I started to look  
around to eventually stumble on the Wine page, which immediatly kept my  
attention as the type of programming it requires interested me the most.

So, I already had experience using Wine, but none regarding Wine  
development. I downloaded the source and started exploring it since then.  
There seems to be a lot of exciting projects one could work on for Wine, I  
might send an application for a project, but so far, I only want to keep  
exploring before preparing anything else.

I tried to code something to get used to the project organization, so I  
started with something really simple, which is the patch I am proposing  
here. It allows Multi-String Value edits in Wine's regedit!

It is quite simple and definitly not concerning Wine's core code, but I  
thought it'd be a good start.

If anybody wants to try it and comment, I'd be really glad to hear from  
you!
I'll keep you updated as I adventure deeper in my Wine exploration!

Thanks a lot,
Philippe Paradis

[Mike McCormack]

Hedos wrote:
> It is quite simple and definitly not concerning Wine's core code, but I  
> thought it'd be a good start.

Hi Philippe,

I had a quick look at your patch, and here are my comments:

+        if( !(newString = (TCHAR*)HeapAlloc(GetProcessHeap(), 0, 
lenbytes)) )
+            return NULL;
+        memset((LPBYTE)newString, 0, lenbytes);
+        int i = 0, charcount = 0, newlinepos = 0;

There's no need to cast HeapAlloc returns or memset arguments.

You shouldn't declare "int i..." inline, as that isn't compatible with 
some compilers that Wine aims to support.

+                if(newLine) { //If last iteration was also a new line, 
this is an empty new line

We don't use C++ style comments for better compiler compatibility too.

+                HeapFree(GetProcessHeap(), 0, newString);
+                if (lRet == ERROR_SUCCESS) result = TRUE;
+                else error_code_messagebox(hwnd, lRet);
+            } else HeapFree(GetProcessHeap(), 0, newString);

The HeapFree() calls are common to both code pathes here, why not just 
do a single HeapFree() call?

+++ programs/regedit/main.c     24 Apr 2006 06:41:43 -0000
@@ -182,6 +182,8 @@ int APIENTRY WinMain(HINSTANCE hInstance
      }
      hAccel = LoadAccelerators(hInstance, (LPCTSTR)IDC_REGEDIT);

+       SetWindowText(hFrameWnd, "test v1.0");

Perhaps you meant to remove the above line before sending the patch?

Please try copy the coding style (curly brace placement, indentation and 
comments style) of the file you're changing, so we don't end up with a 
mismash of coding styles in each file.

Mike

[Mike Hearn]

On Mon, 24 Apr 2006 02:59:40 -0400, Hedos wrote:
> Just a bit myself now to clear up a few things: I'm a first year college  
> student

Cool! Let us know how you get on. Wine can be a slightly intimidating
codebase at first but I found (back when I was about to enter my first
year and started on Wine) that the community is really helpful.

Mike has already given you some standard comments on the patch, but none
are really to do with the contents or approach of the patch which is a
good sign. The compiler compatibility and coding style things are very
common things to watch out for.
> I'll keep you updated as I adventure deeper in my Wine exploration!

Let us know how it goes

thanks -mike

[Hedos]

Thanks a lot for the comments guys!
Here's a revised patch.. no more C++ style comments, no more inline
variable declarations and no more useless casting!

I think it should work pretty well.
I'll keep you updated with anything else I try soon as promised!

Philippe

[Segin]

Hedos wrote:
> Hello folks!
>
> I'm new to Wine development, and I've just completed my first patch. 
> I  hope I could get some advices to make sure I'm heading in the 
> right  direction.
>
> Just a bit myself now to clear up a few things: I'm a first year 
> college  student who realized this week-end the Google Summer of Code 
> was back  again. So, I thought I should definitly give it a try. I 
> started to look  around to eventually stumble on the Wine page, which 
> immediatly kept my  attention as the type of programming it requires 
> interested me the most.
>
> So, I already had experience using Wine, but none regarding Wine  
> development. I downloaded the source and started exploring it since 
> then.  There seems to be a lot of exciting projects one could work on 
> for Wine, I  might send an application for a project, but so far, I 
> only want to keep  exploring before preparing anything else.
>
> I tried to code something to get used to the project organization, so 
> I  started with something really simple, which is the patch I am 
> proposing  here. It allows Multi-String Value edits in Wine's regedit!
>
> It is quite simple and definitly not concerning Wine's core code, but 
> I  thought it'd be a good start.
>
> If anybody wants to try it and comment, I'd be really glad to hear 
> from  you!
> I'll keep you updated as I adventure deeper in my Wine exploration!
>
> Thanks a lot,
> Philippe Paradis

Please replace your "// comment" style comments with "/* comment */" 
ones, kthx.