shell32: Solve the problem about 'OK' button is always gray when call SHBrowseForFolder with flags including both BIF_BROWSEFORCOMPUTER and BIF_RETURNONLYFSDIRS

[许文龙]

shell32: Solve the problem about 'OK' button is always gray when call SHBrowseForFolder with flags including both BIF_BROWSEFORCOMPUTER and BIF_RETURNONLYFSDIRS

[Sebastian Lackner]

On 05.04.2017 07:25, 许文龙 wrote:
> shell32: Solve the problem about 'OK' button is always gray when call SHBrowseForFolder with flags including both BIF_BROWSEFORCOMPUTER and BIF_RETURNONLYFSDIRS
>
> 0001-shell32-Fix-the-bug-about-the-OK-button-in-folder-br.patch

Thank you for your contribution. The general idea is fine, but here are some
suggestions in order to improve your patch.
> From 65f06c86553e0c31aeef167637ef65af0f1c8754 Mon Sep 17 00:00:00 2001
> From: Xu Wenlong <myxuan475(a)126.com>
> Date: Wed, 29 Mar 2017 15:32:55 +0800
> Subject: shell32: Fix the bug about the 'OK' button in folder-browsing dialog
>  is always gray
> To: wine-patches <wine-patches(a)winehq.org>
> Reply-To: wine-devel <wine-devel(a)winehq.org>
>
> Signed-off-by: Xu Wenlong <myxuan475(a)126.com>
> ---
>  dlls/shell32/brsfolder.c       | 10 +++++++---
>  dlls/shell32/tests/brsfolder.c |  5 +++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/shell32/brsfolder.c b/dlls/shell32/brsfolder.c
> index 4040407..1728b1f 100644
> --- a/dlls/shell32/brsfolder.c
> +++ b/dlls/shell32/brsfolder.c
> @@ -476,9 +476,13 @@ static void BrsFolder_CheckValidSelection( browse_info *info, LPTV_ITEMDATA lptv
>      DWORD dwAttributes;
>      HRESULT r;
>
> -    if ((lpBrowseInfo->ulFlags & BIF_BROWSEFORCOMPUTER) &&
> -        !PIDLIsType(pidl, PT_COMP))
> -        bEnabled = FALSE;
> +    if (!(lpBrowseInfo->ulFlags & BIF_RETURNONLYFSDIRS))
> +    {
> +        if ((lpBrowseInfo->ulFlags & BIF_BROWSEFORCOMPUTER) &&
> +            !PIDLIsType(pidl, PT_COMP))
> +            bEnabled = FALSE;
> +    }

You can also write this condition in a single if() statement.
If possible, please also test other combination of flags. BIF_RETURNFSANCESTORS
for example seems to behave similar to BIF_RETURNONLYFSDIRS.
> +
>      if (lpBrowseInfo->ulFlags & BIF_RETURNFSANCESTORS)
>      {
>          dwAttributes = SFGAO_FILESYSANCESTOR | SFGAO_FILESYSTEM;
> diff --git a/dlls/shell32/tests/brsfolder.c b/dlls/shell32/tests/brsfolder.c
> index bf29d11..fecca81 100644
> --- a/dlls/shell32/tests/brsfolder.c
> +++ b/dlls/shell32/tests/brsfolder.c
> @@ -351,6 +351,11 @@ static void test_selection(void)
>      pidl = SHBrowseForFolderA(&bi);
>      CoTaskMemFree(pidl);
>
> +    /* test with flag */
> +    bi.ulFlags = BIF_BROWSEFORCOMPUTER | BIF_RETURNONLYFSDIRS;
> +    pidl = SHBrowseForFolderA(&bi);
> +    CoTaskMemFree(pidl);

The test passes here even without the fix above. In general it is better
to write the test in such a way, that it really confirms the change is
correct.
> +
>      IShellFolder_Release(desktop_object);
>
> CoUninitialize();
> -- 2.8.1

Best regards,
Sebastian