Re: Winecfg -> browse for folder

[Mike Hearn]

On Wed, 2004-12-01 at 17:08 +0100, Robert van Herk wrote:
> Anyways: I already implemented it (as you perhaps saw) using UNIX api 
> calls. I think I did OK, though I am not very experienced in neither C 
> programming or win32. Mike, perhaps you could take a look at my patch?

Sure. Here you go.
> @@ -46,7 +48,8 @@
>    shpolicy.c \
>    shv_bg_cmenu.c \
>    shv_item_cmenu.c \
> - systray.c
> - systray.c \
> - unixTools.c

s/unixTools.c/unix_tools.c/ for consistency

+/*************************************************************************
+ * wine_shell_browse_for_UNIX_directory [SHELL32.@]
+ */
+BOOL WINAPI wine_shell_browse_for_UNIX_directoryA (LPBROWSEINFOA lpbi)
+{
+        if (!lpbi)
+	  return FALSE;

UNIX here should be lower case IMHO, makes it look nicer.

Also you want to crash or do an ERR if a wine API is abused like this, 
silently returning false can lead to hard to find bugs.

+  
+        /*For UNIX browsing, pidls are not used. Hence, if the
+          user passes a pidl that should be root for this browse
+          anyway, we refuse to show the dialog:*/

Could you put spaces between the comment markers and the text?

Also consistent indentation would be good.

+        /*For UNIX browsing, we always use the pszDiplayName to RETURN 
+          the chosen path in, thus:*/
+        if (!lpbi->pszDisplayName)
+        {
+          ERR("pszDisplayName must be set for UNIX browsing.\n");
+          return FALSE;
+        }

How does this work? Is the pszDisplayName pointing to a preallocated
buffer (if so how do you know how big it should be?).

+@ stdcall wine_shell_browse_for_UNIX_directory(ptr) wine_shell_browse_for_UNIX_directoryA
+@ stdcall wine_shell_browse_for_UNIX_directoryA(ptr)
+@ stdcall wine_shell_browse_for_UNIX_directoryW(ptr)

Don't bother with a forwarder here, the WINELIB_NAME_AW macro 
should be sufficient.
> C_SRCS = \
>    appdefaults.c \
> @@ -16,7 +16,7 @@
>    main.c \
>    properties.c \
>    winecfg.c \
> - x11drvdlg.c
> - x11drvdlg.c \

That looks wrong, a \ is only needed when there's something on the next line.
> -
>                bi->hwndOwner = dialog;
> -
>                bi->pszDisplayName = HeapAlloc(GetProcessHeap(), 0, MAX_PATH);
> -
>                bi->lpszTitle = "Please choose a directory"; /*FIXME: Get this multilingual*/

Oh, I see. This is a rather unintuitive API design, why not simply return
the path or have a char ** out parameter instead of setting the input
structure?
> -
>      if ((dirp=readdir(dirdata->dp))!=NULL) {
> -
>        /* We are done, if we found an entry of the correct type: */            
> -
>        if (strcmp(dirp->d_name, ".") == 0) continue;  /*Ignore "." */
> -
>        if (strcmp(dirp->d_name, "..") == 0) continue; /*Ignore ".." */

Excellent! I was wondering if you'd make the mistake of assuming .
and .. were the first two elements in the list, good to see you didn't.
By the way, same comments as with Aneurin, could you please space things
out rather than jamming it all together, eg

if ((dirp = readdir(dirdata->dp) != NULL)
> +static void prepNextFile(LP_UNIXTOOLS_DIRDATA dirdata)
> +{
> - TRACE("hallo\n");
> -

Well, the patch clearly still needs a bit more work ;) Still, it looks
like it's on the right track. Good going!

thanks -mike