[PATCH 0/1] MR2348: comdlg32: Add parent dir button to "Item" file dialog.

[Florian Will (＠w-flo)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29912

Now it looks like this (this patch adds the separator & "up" button):

![Bildschirmfoto_vom_2023-03-08_13-34-36](/uploads/cd30de5b3fa5f39accb4dd14a3f54421/Bildschirmfoto_vom_2023-03-08_13-34-36.png)

[PATCH 1/1] comdlg32: Add parent dir button to "Item" file dialog.

[Florian Will]

From: Florian Will

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29912
---
 dlls/comdlg32/itemdlg.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c
index 95778c2f358..256d3fe99dd 100644
--- a/dlls/comdlg32/itemdlg.c
+++ b/dlls/comdlg32/itemdlg.c
@@ -40,6 +40,7 @@
 #define IDC_NAV_TOOLBAR      200
 #define IDC_NAVBACK          201
 #define IDC_NAVFORWARD       202
+#define IDC_NAVPARENTFOLDER  203

#include <initguid.h>
 /* This seems to be another version of IID_IFileDialogCustomize. If
@@ -1895,7 +1896,8 @@ static void init_toolbar(FileDialogImpl *This, HWND hwnd)
 {
     HWND htoolbar;
     TBADDBITMAP tbab;
-    TBBUTTON button[2];
+    int view_icons_index, num_toolbar_items = 2;
+    TBBUTTON button[4];

htoolbar = CreateWindowExW(0, TOOLBARCLASSNAMEW, NULL, TBSTYLE_FLAT | WS_CHILD | WS_VISIBLE,
                                0, 0, 0, 0,
@@ -1905,6 +1907,9 @@ static void init_toolbar(FileDialogImpl *This, HWND hwnd)
     tbab.nID = IDB_HIST_LARGE_COLOR;
     SendMessageW(htoolbar, TB_ADDBITMAP, 0, (LPARAM)&tbab);

+    tbab.nID = IDB_VIEW_LARGE_COLOR;
+    view_icons_index = SendMessageW(htoolbar, TB_ADDBITMAP, 0, (LPARAM)&tbab);
+
     button[0].iBitmap = HIST_BACK;
     button[0].idCommand = IDC_NAVBACK;
     button[0].fsState = TBSTATE_ENABLED;
@@ -1919,7 +1924,26 @@ static void init_toolbar(FileDialogImpl *This, HWND hwnd)
     button[1].dwData = 0;
     button[1].iString = 0;

-    SendMessageW(htoolbar, TB_ADDBUTTONSW, 2, (LPARAM)button);
+    if (view_icons_index >= 0)
+    {
+        button[2].iBitmap = 0;
+        button[2].idCommand = 0;
+        button[2].fsState = TBSTATE_ENABLED;
+        button[2].fsStyle = BTNS_SEP;
+        button[2].dwData = 0;
+        button[2].iString = 0;
+
+        button[3].iBitmap = view_icons_index + VIEW_PARENTFOLDER;
+        button[3].idCommand = IDC_NAVPARENTFOLDER;
+        button[3].fsState = TBSTATE_ENABLED;
+        button[3].fsStyle = BTNS_BUTTON;
+        button[3].dwData = 0;
+        button[3].iString = 0;
+
+        num_toolbar_items += 2;
+    }
+
+    SendMessageW(htoolbar, TB_ADDBUTTONSW, num_toolbar_items, (LPARAM)button);
     SendMessageW(htoolbar, TB_SETBUTTONSIZE, 0, MAKELPARAM(24,24));
     SendMessageW(htoolbar, TB_AUTOSIZE, 0, 0);
 }
@@ -2160,6 +2184,13 @@ static LRESULT on_browse_forward(FileDialogImpl *This)
     return FALSE;
 }

+static LRESULT on_browse_parentfolder(FileDialogImpl *This)
+{
+    TRACE("%p\n", This);
+    IExplorerBrowser_BrowseToIDList(This->peb, NULL, SBSP_PARENT);
+    return FALSE;
+}
+
 static LRESULT on_command_filetype(FileDialogImpl *This, WPARAM wparam, LPARAM lparam)
 {
     if(HIWORD(wparam) == CBN_SELCHANGE)
@@ -2218,6 +2249,7 @@ static LRESULT on_wm_command(FileDialogImpl *This, WPARAM wparam, LPARAM lparam)
     case psh1:                return on_command_opendropdown(This, wparam, lparam);
     case IDC_NAVBACK:         return on_browse_back(This);
     case IDC_NAVFORWARD:      return on_browse_forward(This);
+    case IDC_NAVPARENTFOLDER: return on_browse_parentfolder(This);
     case IDC_FILETYPE:        return on_command_filetype(This, wparam, lparam);
     default:                  TRACE("Unknown command.\n");
     }

[Florian Will (＠w-flo)]

I just noticed there is !672, which does the same thing (among other things). It seems inactive though and apparently has some issues.

My patch loads/uses the "parent folder" icon a bit differently, I'm not really sure how/why it works in the other patch. Otherwise, that portion of !672 seems to be very similar to my patch, with slightly different naming.

Since the "parent dir" button is probably the most pressing usability issue with that dialog and the other commit might be a bit too large, I'm keeping this merge request open.

[Esme Povirk (＠madewokherd)]

Esme Povirk (@madewokherd) commented about dlls/comdlg32/itemdlg.c:
>  button[1].dwData = 0;
>  button[1].iString = 0;
>
> - SendMessageW(htoolbar, TB_ADDBUTTONSW, 2, (LPARAM)button);
> - if (view_icons_index >= 0)

In what situation does the earlier TB_ADDBITMAP call fail?

[Florian Will (＠w-flo)]

On Wed Mar  8 17:59:26 2023 +0000, Esme Povirk wrote:
> In what situation does the earlier TB_ADDBITMAP call fail?

I'm not sure, I implemented this by just reading the online docs that say it might return -1 on error, so I thought I should check that case.

Looking at wine's implementation, it might fail when `ImageList_Create` fails because of an allocation failure e.g. in `ImageListImpl_CreateInstance` or `CreateCompatibleDC`? I'm not quite sure how the control flow works in `TOOLBAR_AddBitmap` when using the standard icon sets.

Now that you mention it, the other SendMessageW has no error check. I guess failure is extremely unlikely here, so if it's okay to ignore, I'd be happy to drop that error handling. :-)

[Esme Povirk (＠madewokherd)]

On Wed Mar  8 18:43:23 2023 +0000, Florian Will wrote:
> I'm not sure, I implemented this by just reading the online docs that
> say it might return -1 on error, so I thought I should check that case.
> Looking at wine's implementation, it might fail when `ImageList_Create`
> fails because of an allocation failure e.g. in
> `ImageListImpl_CreateInstance` or `CreateCompatibleDC`? I'm not quite
> sure how the control flow works in `TOOLBAR_AddBitmap` when using the
> standard icon sets.
> Now that you mention it, the other SendMessageW has no error check. I
> guess failure is extremely unlikely here, so if it's okay to ignore, I'd
> be happy to drop that error handling. :-)

Given that the only thing we can do is silently ignore it anyway, and failure is not expected, I think it's not worth checking.