Re: Shwapi updates

[James Hawkins]

Can this patch be broken up into smaller, independent parts?  It's
more likely to be committed if that is the case.

On Sun, 12 Sep 2004 08:55:27 -0700 (PDT), Jon Griffiths
 wrote:
> Cheers,
> Jon
>
> +dlls/shlwapi/shlwapi.spec dlls/shlwapi/ordinal.c
>     Implement SHSimulateDrop
>
> +dlls/shlwapi/reg.c
>     Implement MIME_GetExtensionA/W
>
> +dlls/shlwapi/string.c
>     Implement StrCpyNXA/W, SHAnsiToAnsi, SHUnicodeToUnicode
>
> +dlls/shlwapi/tests/string.c
>     Test StrCpyNXA/W, SHAnsiToAnsi, SHUnicodeToUnicode
>
> =====
> "Don't wait for the seas to part, or messiahs to come;
>  Don't you sit around and waste this chance..." - Live
>
> [e-mail]
>
> Do you Yahoo!?
> Shop for Back-to-School deals on Yahoo! Shopping.
> http://shopping.yahoo.com/backtoschool
>
> diff -ur wine/dlls/shlwapi/ordinal.c wine-develop/dlls/shlwapi/ordinal.c
> --- wine/dlls/shlwapi/ordinal.c 2004-09-05 08:36:36.000000000 +0000
> +++ wine-develop/dlls/shlwapi/ordinal.c 2004-09-11 12:38:46.000000000 +0000
> @@ -1829,6 +1829,30 @@
>  }
>
> /*************************************************************************
> -
> -
>  @      [SHLWAPI.186]
> - */
>
> +BOOL WINAPI SHSimulateDrop(IDropTarget *pDrop, IDataObject *pDataObj,
> -
>                       DWORD grfKeyState, PPOINTL lpPt, DWORD* pdwEffect)
>
> +{
> - DWORD dwEffect = DROPEFFECT_LINK | DROPEFFECT_MOVE | DROPEFFECT_COPY;
> - POINTL pt = { 0, 0 };
> -
> - if (!lpPt)
> - lpPt = &pt;
> -
> - if (!pdwEffect)
> - pdwEffect = &dwEffect;
> -
> - IDropTarget_DragEnter(pDrop, pDataObj, grfKeyState, *lpPt, pdwEffect);
> -
> - if (*pdwEffect)
> - return IDropTarget_Drop(pDrop, pDataObj, grfKeyState, *lpPt, pdwEffect);
> -
> - IDropTarget_DragLeave(pDrop);
> - return TRUE;
>
> +}
> -
>
> +/*************************************************************************
> -
>  @      [SHLWAPI.187]
> -
> - Call IPersistPropertyBag_Load() on an object.
>
> @@ -3000,19 +3024,6 @@
>  }
>
> /*************************************************************************
> -
> -
>  @      [SHLWAPI.346]
> - */
>
> -DWORD WINAPI SHUnicodeToUnicode(
> -
>   LPCWSTR src,
> -
>   LPWSTR dest,
> -
>   int len)
>
> -{
> -
>   FIXME("(%s %p 0x%08x)stub\n",debugstr_w(src),dest,len);
> -
>   lstrcpynW(dest, src, len);
> -
>   return lstrlenW(dest)+1;
>
> -}
> -
>
> -/*************************************************************************
> -
>  @      [SHLWAPI.350]
> -
> - See GetFileVersionInfoSizeW.
>
> diff -ur wine/dlls/shlwapi/reg.c wine-develop/dlls/shlwapi/reg.c
> --- wine/dlls/shlwapi/reg.c     2004-07-05 20:27:24.000000000 +0000
> +++ wine-develop/dlls/shlwapi/reg.c     2004-09-11 12:41:58.000000000 +0000
> @@ -1833,6 +1833,77 @@
>  }
>
> /*************************************************************************
> -
> - @   [SHLWAPI.330]
> -
> -
> -
> - Get the files extension for a given Mime type.
> -
> -
> -
> - PARAMS
> -
> - lpszType [I] Mime type to get the file extension for
> -
> - lpExt    [O] Destingtion for the resulting extension
> -
> - iLen     [I] Length og lpExt in characters
> -
> -
> -
> - RETURNS
> -
> - Success: TRUE. lpExt contains the file extension.
> -
> - Failure: FALSE, if any paramater is invalid or the extension cannot be
> -
> -
>       retrieved. If iLen > 0, lpExt is set to an empty string.
> -
> -
> -
> - NOTES
> -
> -
> - The extension returned in lpExt always has a leading '.' character, even
> -
> - if the registry Mime database entry does not.
> -
> -
> - iLen must be long enough for the file extension for this function to succeed.
> - */
>
> +BOOL WINAPI MIME_GetExtensionA(LPCSTR lpszType, LPSTR lpExt, INT iLen)
> +{
> - char szSubKey[MAX_PATH];
> - DWORD dwlen = iLen - 1, dwType;
> - BOOL bRet = FALSE;
> -
> - if (iLen > 0 && lpExt)
> - *lpExt = '\0';
> -
> - if (lpszType && lpExt && iLen > 2 &&
> -
>  GetMIMETypeSubKeyA(lpszType, szSubKey, MAX_PATH) &&
> -
>  !SHGetValueA(HKEY_CLASSES_ROOT, szSubKey, szExtensionA, &dwType, lpExt + 1, &dwlen) &&
> -
>  lpExt[1])
> - {
> - if (lpExt[1] == '.')
> -
>  memmove(lpExt, lpExt + 1, strlen(lpExt + 1) + 1);
> - else
> -
>  *lpExt = '.'; /* Supply a '.' */
> - bRet = TRUE;
> - }
> - return bRet;
>
> +}
> -
>
> +/*************************************************************************
> -
> - @   [SHLWAPI.331]
> -
> -
> -
> - Unicode version of MIME_GetExtensionA.
> - */
>
> +BOOL WINAPI MIME_GetExtensionW(LPCWSTR lpszType, LPWSTR lpExt, INT iLen)
> +{
> - WCHAR szSubKey[MAX_PATH];
> - DWORD dwlen = iLen - 1, dwType;
> - BOOL bRet = FALSE;
> -
> - if (iLen > 0 && lpExt)
> - *lpExt = '\0';
> -
> - if (lpszType && lpExt && iLen > 2 &&
> -
>  GetMIMETypeSubKeyW(lpszType, szSubKey, MAX_PATH) &&
> -
>  !SHGetValueW(HKEY_CLASSES_ROOT, szSubKey, szExtensionW, &dwType, lpExt + 1, &dwlen) &&
> -
>  lpExt[1])
> - {
> - if (lpExt[1] == '.')
> -
>  memmove(lpExt, lpExt + 1, (strlenW(lpExt + 1) + 1) * sizeof(WCHAR));
> - else
> -
>  *lpExt = '.'; /* Supply a '.' */
> - bRet = TRUE;
> - }
> - return bRet;
>
> +}
> -
>
> +/*************************************************************************
> - @   [SHLWAPI.324]
> -
> - Set the file extension for a MIME content key.
>
> diff -ur wine/dlls/shlwapi/shlwapi.spec wine-develop/dlls/shlwapi/shlwapi.spec
> --- wine/dlls/shlwapi/shlwapi.spec      2004-09-05 08:36:36.000000000 +0000
> +++ wine-develop/dlls/shlwapi/shlwapi.spec      2004-09-11 12:42:10.000000000 +0000
> @@ -183,7 +183,7 @@
>  183 stdcall -noname SHRegisterClassA(ptr)
>  184 stdcall @(ptr ptr long) SHLWAPI_184
>  185 stdcall -noname SHMessageBoxCheckA(ptr str str long long str)
> -186 stub -noname SHSimulateDrop
> +186 stdcall -noname SHSimulateDrop(ptr ptr long ptr ptr)
>  187 stdcall -noname SHLoadFromPropertyBag(ptr ptr)
>  188 stub -noname IUnknown_TranslateAcceleratorOCS
>  189 stdcall -noname IUnknown_OnFocusOCS(ptr ptr)
> @@ -327,8 +327,8 @@
>  327 stdcall -noname UnregisterExtensionForMIMETypeW(wstr)
>  328 stdcall -noname GetMIMETypeSubKeyA(str ptr long)
>  329 stdcall -noname GetMIMETypeSubKeyW(wstr ptr long)
> -330 stub -noname MIME_GetExtensionA
> -331 stub -noname MIME_GetExtensionW
> +330 stdcall -noname MIME_GetExtensionA(str ptr long)
> +331 stdcall -noname MIME_GetExtensionW(wstr ptr long)
>  332 stdcall @(ptr long) user32.CallMsgFilterW
>  333 stdcall -noname SHBrowseForFolderWrapW(ptr)
>  334 stdcall -noname SHGetPathFromIDListWrapW(ptr ptr)
> @@ -342,7 +342,7 @@
>  342 stdcall -noname SHInterlockedCompareExchange(ptr long long)
>  343 stdcall -noname SHRegGetCLSIDKeyA(ptr str long long ptr)
>  344 stdcall -noname SHRegGetCLSIDKeyW(ptr wstr long long ptr)
> -345 stub -noname SHAnsiToAnsi
> +345 stdcall -noname SHAnsiToAnsi(str ptr long)
>  346 stdcall -noname SHUnicodeToUnicode(wstr ptr long)
>  347 stdcall @(long wstr) advapi32.RegDeleteValueW
>  348 stub -noname SHGetFileDescriptionW
> diff -ur wine/dlls/shlwapi/string.c wine-develop/dlls/shlwapi/string.c
> --- wine/dlls/shlwapi/string.c  2004-09-05 08:36:36.000000000 +0000
> +++ wine-develop/dlls/shlwapi/string.c  2004-09-11 12:43:37.000000000 +0000
> @@ -2098,7 +2098,7 @@
> - iLen     [I] Maximum number of chars to copy
> -
> - RETURNS
> -
> - Success: A pointer to the last character written.
> -
> - Success: A pointer to the last character written to lpszDest..
> - Failure: lpszDest, if any arguments are invalid.
> */
>
> LPSTR WINAPI StrCpyNXA(LPSTR lpszDest, LPCSTR lpszSrc, int iLen)
> @@ -2623,6 +2623,48 @@
>  }
>
> /*************************************************************************
> -
> -
>  @      [SHLWAPI.345]
> -
> -
> -
> - Copy one string to another.
> -
> -
> -
> - PARAMS
> -
> - lpszSrc [I] Source string to copy
> -
> - lpszDst [O] Destination for copy
> -
> - iLen    [I] Length of lpszDst in characters
> -
> -
> -
> - RETURNS
> -
> - The length of the copied string, including the terminating NUL. lpszDst
> -
> - contains iLen characters of lpszSrc.
> - */
>
> +DWORD WINAPI SHAnsiToAnsi(LPCSTR lpszSrc, LPSTR lpszDst, int iLen)
> +{
> - LPSTR lpszRet;
> -
> - TRACE("(%s,%p,0x%08x)\n", debugstr_a(lpszSrc), lpszDst, iLen);
> -
> - /* Our original version used lstrncpy/lstrlen, incorrectly filling up all
> -
> * of lpszDst with extra NULs. This version is correct, and faster too.
> -
> */
> - lpszRet = StrCpyNXA(lpszDst, lpszSrc, iLen);
> - return lpszRet - lpszDst + 1;
>
> +}
> -
>
> +/*************************************************************************
> -
> -
>  @      [SHLWAPI.346]
> -
> -
> -
> - Unicode version of SSHAnsiToAnsi.
> - */
>
> +DWORD WINAPI SHUnicodeToUnicode(LPCWSTR lpszSrc, LPWSTR lpszDst, int iLen)
> +{
> - LPWSTR lpszRet;
> -
> - TRACE("(%s,%p,0x%08x)\n", debugstr_w(lpszSrc), lpszDst, iLen);
> -
> - lpszRet = StrCpyNXW(lpszDst, lpszSrc, iLen);
> - return lpszRet - lpszDst + 1;
>
> +}
> -
>
> +/*************************************************************************
> -
>  @      [SHLWAPI.364]
> -
> - Determine if an Ascii string converts to Unicode and back identically.
>
> diff -ur wine/dlls/shlwapi/tests/string.c wine-develop/dlls/shlwapi/tests/string.c
> --- wine/dlls/shlwapi/tests/string.c    2004-07-21 12:17:03.000000000 +0000
> +++ wine-develop/dlls/shlwapi/tests/string.c    2004-09-11 12:47:14.000000000 +0000
> @@ -33,7 +33,12 @@
>  #include "shlwapi.h"
>  #include "shtypes.h"
>
> -static HRESULT (WINAPI *ptr_StrRetToBSTR) (STRRET*, void*, BSTR*);
> +static HMODULE hShlwapi;
> +static LPSTR   (WINAPI *pStrCpyNXA)(LPSTR,LPCSTR,int);
> +static LPWSTR  (WINAPI *pStrCpyNXW)(LPWSTR,LPCWSTR,int);
> +static HRESULT (WINAPI *pStrRetToBSTR)(STRRET*,void*,BSTR*);
> +static DWORD   (WINAPI *pSHAnsiToAnsi)(LPCSTR,LPSTR,int);
> +static DWORD   (WINAPI *pSHUnicodeToUnicode)(LPCWSTR,LPWSTR,int);
>
> static inline int strcmpW(const WCHAR *str1, const WCHAR *str2)
>  {
> @@ -592,22 +597,19 @@
>
> static void test_StrRetToBSTR(void)
>  {
> -
>
> HMODULE module;
> static const WCHAR szTestW[] = { 'T','e','s','t','\0' };
> ITEMIDLIST iidl[10];
> BSTR bstr;
> STRRET strret;
> HRESULT ret;
> -
>
> module = GetModuleHandleA("shlwapi");
> -
>
> if (!module) return;
> -
>
> ptr_StrRetToBSTR = (void *)GetProcAddress(module, "StrRetToBSTR");
> -
>
> if (!ptr_StrRetToBSTR) return;
> -
>
> pStrRetToBSTR = (void *)GetProcAddress(hShlwapi, "StrRetToBSTR");
> -
>
> if (!pStrRetToBSTR) return;
>
> strret.uType = STRRET_WSTR;
> strret.u.pOleStr = CoDupStrW("Test");
> bstr = 0;
> - ret = ptr_StrRetToBSTR(&strret, NULL, &bstr);
> - ret = pStrRetToBSTR(&strret, NULL, &bstr);
> ok(ret == S_OK && bstr && !strcmpW(bstr, szTestW),
>    "STRRET_WSTR: dup failed, ret=0x%08lx, bstr %p\n", ret, bstr);
> if (bstr)
>
> @@ -615,7 +617,7 @@
>  strret.uType = STRRET_CSTR;
>  lstrcpyA(strret.u.cStr, "Test");
> - ret = ptr_StrRetToBSTR(&strret, NULL, &bstr);
> - ret = pStrRetToBSTR(&strret, NULL, &bstr);
> ok(ret == S_OK && bstr && !strcmpW(bstr, szTestW),
>    "STRRET_CSTR: dup failed, ret=0x%08lx, bstr %p\n", ret, bstr);
> if (bstr)
>
> @@ -624,7 +626,7 @@
>      strret.uType = STRRET_OFFSET;
>      strret.u.uOffset = 1;
>      strcpy((char*)&iidl, " Test");
> - ret = ptr_StrRetToBSTR(&strret, iidl, &bstr);
> - ret = pStrRetToBSTR(&strret, iidl, &bstr);
> ok(ret == S_OK && bstr && !strcmpW(bstr, szTestW),
>    "STRRET_OFFSET: dup failed, ret=0x%08lx, bstr %p\n", ret, bstr);
> if (bstr)
>
> @@ -633,10 +635,85 @@
>      /* Native crashes if str is NULL */
>  }
>
> +static void test_StrCpyNXA(void)
> +{
> - LPCSTR lpSrc = "hello";
> - LPSTR lpszRes;
> - char dest[8];
> -
> - pStrCpyNXA = (void *)GetProcAddress(hShlwapi, (LPSTR)399);
> - if (!pStrCpyNXA)
> - return;
> -
> - memset(dest, '\n', sizeof(dest));
> - lpszRes = pStrCpyNXA(dest, lpSrc, sizeof(dest)/sizeof(dest[0]));
> - ok(lpszRes == dest + 5 && !memcmp(dest, "hello\0\n\n", sizeof(dest)),
> -
>   "StrCpyNXA: expected %p, \"hello\\0\\n\\n\", got %p, \"%d,%d,%d,%d,%d,%d,%d,%d\"\n",
> -
>   dest + 5, lpszRes, dest[0], dest[1], dest[2], dest[3], dest[4], dest[5], dest[6], dest[7]);
>
> +}
> -
>
> +static void test_StrCpyNXW(void)
> +{
> - static const WCHAR lpInit[] = { '\n','\n','\n','\n','\n','\n','\n','\n' };
> - static const WCHAR lpSrc[] = { 'h','e','l','l','o','\0' };
> - static const WCHAR lpRes[] = { 'h','e','l','l','o','\0','\n','\n' };
> - LPWSTR lpszRes;
> - WCHAR dest[8];
> -
> - pStrCpyNXW = (void *)GetProcAddress(hShlwapi, (LPSTR)400);
> - if (!pStrCpyNXW)
> - return;
> -
> - memcpy(dest, lpInit, sizeof(lpInit));
> - lpszRes = pStrCpyNXW(dest, lpSrc, sizeof(dest)/sizeof(dest[0]));
> - ok(lpszRes == dest + 5 && !memcmp(dest, lpRes, sizeof(dest)),
> -
>   "StrCpyNXA: expected %p, \"hello\\0\\n\\n\", got %p, \"%d,%d,%d,%d,%d,%d,%d,%d\"\n",
> -
>   dest + 5, lpszRes, dest[0], dest[1], dest[2], dest[3], dest[4], dest[5], dest[6], dest[7]);
>
> +}
> -
>
> +static void test_SHAnsiToAnsi(void)
> +{
> - char dest[8];
> - DWORD dwRet;
> -
> - pSHAnsiToAnsi = (void *)GetProcAddress(hShlwapi, (LPSTR)345);
> - if (!pSHAnsiToAnsi)
> - return;
> -
> - memset(dest, '\n', sizeof(dest));
> - dwRet = pSHAnsiToAnsi("hello", dest, sizeof(dest)/sizeof(dest[0]));
> - ok(dwRet == 6 && !memcmp(dest, "hello\0\n\n", sizeof(dest)),
> -
> "SHAnsiToAnsi: expected 6, \"hello\\0\\n\\n\", got %ld, \"%d,%d,%d,%d,%d,%d,%d,%d\"\n",
> -
> dwRet, dest[0], dest[1], dest[2], dest[3], dest[4], dest[5], dest[6], dest[7]);
>
> +}
> -
>
> +static void test_SHUnicodeToUnicode(void)
> +{
> - static const WCHAR lpInit[] = { '\n','\n','\n','\n','\n','\n','\n','\n' };
> - static const WCHAR lpSrc[] = { 'h','e','l','l','o','\0' };
> - static const WCHAR lpRes[] = { 'h','e','l','l','o','\0','\n','\n' };
> - WCHAR dest[8];
> - DWORD dwRet;
> -
> - pSHUnicodeToUnicode = (void *)GetProcAddress(hShlwapi, (LPSTR)346);
> - if (!pSHUnicodeToUnicode)
> - return;
> -
> - memcpy(dest, lpInit, sizeof(lpInit));
> - dwRet = pSHUnicodeToUnicode(lpSrc, dest, sizeof(dest)/sizeof(dest[0]));
> - ok(dwRet == 6 && !memcmp(dest, lpRes, sizeof(dest)),
> -
> "SHUnicodeToUnicode: expected 6, \"hello\\0\\n\\n\", got %ld, \"%d,%d,%d,%d,%d,%d,%d,%d\"\n",
> -
> dwRet, dest[0], dest[1], dest[2], dest[3], dest[4], dest[5], dest[6], dest[7]);
>
> +}
> -
>
> START_TEST(string)
>  {
>    CoInitialize(0);
> - hShlwapi = GetModuleHandleA("shlwapi");
> - if (!hShlwapi)
> -
> return;
> - test_StrChrA();
>  test_StrChrW();
>  test_StrChrIA();
>
> @@ -656,4 +733,8 @@
>    test_StrCmpA();
>    test_StrCmpW();
>    test_StrRetToBSTR();
> - test_StrCpyNXA();
> - test_StrCpyNXW();
> - test_SHAnsiToAnsi();
> - test_SHUnicodeToUnicode();
>
> }

Shwapi updates

[Jon Griffiths]

Hi James,
> Can this patch be broken up into smaller, independent parts?  It's
> more likely to be committed if that is the case.

Aside from the tests, what constitutes independent? These are all new
functions so there is no possibility of regressions, which is the
main reason to split up patches. Ease of understanding/reviewing the
patches is the other main reason, but each function is only 5-10
lines.

Since each added function requires the spec file to be updated its
much easier to add the functions together (It helps me clean up the
diffs from my tree to wine-cvs faster too).

Cheers,
Jon

=====
"Don't wait for the seas to part, or messiahs to come;
 Don't you sit around and waste this chance..." - Live

[e-mail]

_______________________________
Do you Yahoo!?
Shop for Back-to-School deals on Yahoo! Shopping.
http://shopping.yahoo.com/backtoschool

Shwapi updates

[Alexandre Julliard]

Jon Griffiths  writes:
> Aside from the tests, what constitutes independent? These are all new
> functions so there is no possibility of regressions, which is the
> main reason to split up patches. Ease of understanding/reviewing the
> patches is the other main reason, but each function is only 5-10
> lines.
>
> Since each added function requires the spec file to be updated its
> much easier to add the functions together (It helps me clean up the
> diffs from my tree to wine-cvs faster too).

It's OK to send them in the same patch, but then please also send a
single Changelog entry for the whole patch, not one per file.

Shwapi updates

[James Hawkins]

> Aside from the tests, what constitutes independent?

If the functions do not rely on each other, then they are independent
of each other.  You can leave it the way it is, but it was a
suggestion to help from the case where say one of the functions has an
error in it.  If that is the case then none of th patch is committed
where if it was split into seperate patches, 4/5 would be committed
and only a minor change is required to finish it, but this is all
preference of author/reviewer.

On Mon, 13 Sep 2004 02:38:14 -0700 (PDT), Jon Griffiths
 wrote:
> Hi James,
> > Can this patch be broken up into smaller, independent parts?  It's
> > more likely to be committed if that is the case.
>
> Aside from the tests, what constitutes independent? These are all new
> functions so there is no possibility of regressions, which is the
> main reason to split up patches. Ease of understanding/reviewing the
> patches is the other main reason, but each function is only 5-10
> lines.
>
> Since each added function requires the spec file to be updated its
> much easier to add the functions together (It helps me clean up the
> diffs from my tree to wine-cvs faster too).
>
> Cheers,
> Jon
>
> =====
> "Don't wait for the seas to part, or messiahs to come;
> Don't you sit around and waste this chance..." - Live
>
> [e-mail]
>
> Do you Yahoo!?
> Shop for Back-to-School deals on Yahoo! Shopping.
> http://shopping.yahoo.com/backtoschool