[PATCH v3 0/11] MR2993: comdlg32: IFileDialog navigation bar control. Part 1.

Vladislav Timonin (＠vt)

11 Jun 2023 11 Jun '23

9:43 a.m.

Redo of !1857, as suggested, the plan is to split original MR in half, breadcrumbs in first (this) MR, and address edit in another. The changes are mostly cleanup (formatting, renaming, unused variables, missing free). Visually, it hasn't changed from original MR, part 1 changes are: ![](/uploads/42fa717ce6f7adb40d6b0dcbbe0878fa/changes-1-3.png) And part 2: ![](/uploads/cbdaa2d4b5b8945587e1f761a4609a08/changes-2.png) Part 2 branch can be found [here](https://gitlab.winehq.org/vt/wine/-/commits/fd-navbar-part2). Requires !2068, otherwise if application doesn't request comctl v6 (e.g. qapitrace), navigation bar will look like this: ![no-comctl-v6](/uploads/03e6a8b6dcf827114157e79640e49fea/no-comctl-v6.png) Closes: - https://bugs.winehq.org/show_bug.cgi?id=29912 - https://bugs.winehq.org/show_bug.cgi?id=54812 - https://bugs.winehq.org/show_bug.cgi?id=50338 (partially? fully with address edit?) MR changes: - v2 - Changed the gap between buttons to be scaled with DPI Description edits: - v2 - Updated the changes image (by splitting it) - Added MR changes - v3 - Added description edits - Updated the changes image again (forgot the overflow button change) -- v3: comdlg32: Redirect IFileDialog navigation bar buttons focus back to the previous window. comdlg32: Retain IExplorerBrowser view focus in IFileDialog when creating new view. comdlg32: Prevent arrow navigation of IFileDialog navigation bar buttons. comdlg32: Don't paint focus rect in IFileDialog navigation bar buttons. comdlg32: Add breadcrumb overflow menu to IFileDialog navigation bar. comdlg32: Add inner border to breadcrumbs in IFileDialog navigation bar. comdlg32: Always show at least 2 crumbs in IFileDialog navigation bar. comdlg32: Reuse address breadcrumbs in IFileDialog navigation bar. comdlg32: Add address breadcrumbs to IFileDialog navigation bar. https://gitlab.winehq.org/wine/wine/-/merge_requests/2993

Show replies by date

Vladislav Timonin

11 Jun 11 Jun

Also forgot file name edit box navigation is a thing as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35379

Vladislav Timonin (＠vt)

12 Jun 12 Jun

6:48 p.m.

On Fri Jun 9 21:06:57 2023 +0000, Esme Povirk wrote:

...

I think the "at least N in the list" check might be simpler as part of this loop. If `crumbs_visible_n < 2`, you can just give it the remaining space here and then break out of the loop. <details><summary>Sorry I'm being slow, something like this?</summary>

```c LIST_FOR_EACH_ENTRY_REV(crumb1, &info->crumbs, struct crumb, entry) { INT next_w = w + crumb1->full_w; crumb1->current_w = crumb1->full_w; if (next_w > max_crumbs_w) { /* always show at least 2 crumbs */ if (crumbs_visible_n < 2) { crumb1 = (struct crumb *)list_tail(&info->crumbs); crumb2 = (struct crumb *)list_tail(&crumb1->entry); if (&crumb2->entry == &info->crumbs) /* only 1 crumb in the list */ { if (crumbs_visible_n == 0) /* that doesn't fit on screen */ { crumb1->current_w = max_crumbs_w; crumbs_visible_n = 1; } prev_x = buttons_w + crumb1->current_w; } else { /* at least 2 crumbs in the list, 1 or 0 crumbs on screen */ /* last crumb should take as much space as possible */ INT min_crumb_w = MulDiv(56, info->dpi_x, USER_DEFAULT_SCREEN_DPI); INT crumb2_size = crumb2->full_w; if (crumb2_size > min_crumb_w) crumb2_size = min_crumb_w; /* calculate how much extra space is required to fit both crumbs */ w = (crumb1->full_w + crumb2_size) - max_crumbs_w; if (w > 0) { /* not enough space for last crumb, truncate it */ crumb1->current_w = max(0, crumb1->full_w - w); crumb2->current_w = crumb2_size; } else /* enough space for last crumb, let the previous crumb take leftover space */ crumb2->current_w = max_crumbs_w - crumb1->full_w; crumbs_visible_n = 2; prev_x = buttons_w + crumb1->current_w + crumb2->current_w; } } break; } w = next_w; crumbs_visible_n += 1; prev_x = buttons_w + w; } ``` </details> Moving the `if (crumbs_visible_n < 2)` branch into the `if (next_w > max_crumbs_w)`, and recalculating `else` of `crumbs_visible_n < 2` on each iteration. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35504

Esme Povirk (＠madewokherd)

13 Jun 13 Jun

6:23 p.m.

On Mon Jun 12 18:48:07 2023 +0000, Vladislav Timonin wrote:

...

<details><summary>Sorry I'm being slow, something like this?</summary> ```c LIST_FOR_EACH_ENTRY_REV(crumb1, &info->crumbs, struct crumb, entry) { INT next_w = w + crumb1->full_w; crumb1->current_w = crumb1->full_w; if (next_w > max_crumbs_w) { /* always show at least 2 crumbs */ if (crumbs_visible_n < 2) { crumb1 = (struct crumb *)list_tail(&info->crumbs); crumb2 = (struct crumb *)list_tail(&crumb1->entry); if (&crumb2->entry == &info->crumbs) /* only 1 crumb in the list */ { if (crumbs_visible_n == 0) /* that doesn't fit on screen */ { crumb1->current_w = max_crumbs_w; crumbs_visible_n = 1; } prev_x = buttons_w + crumb1->current_w; } else { /* at least 2 crumbs in the list, 1 or 0 crumbs on screen */ /* last crumb should take as much space as possible */ INT min_crumb_w = MulDiv(56, info->dpi_x, USER_DEFAULT_SCREEN_DPI); INT crumb2_size = crumb2->full_w; if (crumb2_size > min_crumb_w) crumb2_size = min_crumb_w; /* calculate how much extra space is required to fit both crumbs */ w = (crumb1->full_w + crumb2_size) - max_crumbs_w; if (w > 0) { /* not enough space for last crumb, truncate it */ crumb1->current_w = max(0, crumb1->full_w - w); crumb2->current_w = crumb2_size; } else /* enough space for last crumb, let the previous crumb take leftover space */ crumb2->current_w = max_crumbs_w - crumb1->full_w; crumbs_visible_n = 2; prev_x = buttons_w + crumb1->current_w + crumb2->current_w; } } break; } w = next_w; crumbs_visible_n += 1; prev_x = buttons_w + w; } ``` </details> Moving the `if (crumbs_visible_n < 2)` branch into the `if (next_w > max_crumbs_w)`, and recalculating `else` of `crumbs_visible_n < 2` on each iteration. Oh, I forgot how complicated the logic is there, guess it can't be simplified that way.

It would be really nice to simplify it though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35641

Esme Povirk (＠madewokherd)

7:10 p.m.

On Tue Jun 13 18:23:33 2023 +0000, Esme Povirk wrote:

...

Oh, I forgot how complicated the logic is there, guess it can't be simplified that way. It would be really nice to simplify it though. I made an attempt at simplification (untested):

    LIST_FOR_EACH_ENTRY_REV(crumb1, &info->crumbs, struct crumb, entry)
    {
        INT next_w = w + crumb1->full_w;
        crumb1->current_w = crumb1->full_w;

        if (next_w > max_crumbs_w)
        {
            if (crumbs_visible_n == 0)
            {
                /* fit as much of the last crumb as possible */
                crumb1->current_w = max_crumbs_w - w;
                w = max_crumbs_w;
                crumbs_visible_n += 1;

                /* try to also fit the next crumb */
                continue;
            }
            else if (crumbs_visible_n == 1)
            {
                INT min_crumb_width = (crumbs_visible_n == 1) ?
                    MulDiv(56, info->dpi_x, USER_DEFAULT_SCREEN_DPI) : 1;
                min_crumb_width = min(min_crumb_width, crumb1->full_w);
                if (w + min_crumb_width &lt;= max_crumbs_w)
                {
                    /* fit as much of this crumb as possible */
                    crumb1->current_w = max_crumbs_w - w;
                    w = max_crumbs_w;
                }
                else
                {
                    /* take space away from the last crumb to get this one to the minimum */
                    struct crumb *last_crumb = (struct crumb *)list_tail(&info->crumbs);
                    last_crumb->current_w = max(0, max_crumbs_w - min_crumb_width);
                    crumb1->current_w = min_crumb_width;
                    w = last_crumb->current_w + crumb1->current_w;
                }
                crumbs_visible_n += 1;
            }
            break;
        }

        w = next_w;
        crumbs_visible_n += 1;
    }

It's still complicated, but it can use some bits of the loop and reduce the special cases for fewer than 2 items. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35646

Vladislav Timonin (＠vt)

7:39 p.m.

On Tue Jun 13 19:10:31 2023 +0000, Esme Povirk wrote:

...

I made an attempt at simplification (untested): ```c LIST_FOR_EACH_ENTRY_REV(crumb1, &info->crumbs, struct crumb, entry) { INT next_w = w + crumb1->full_w; crumb1->current_w = crumb1->full_w; if (next_w > max_crumbs_w) { if (crumbs_visible_n == 0) { /* fit as much of the last crumb as possible */ crumb1->current_w = max_crumbs_w - w; w = max_crumbs_w; crumbs_visible_n += 1; /* try to also fit the next crumb */ continue; } else if (crumbs_visible_n == 1) { INT min_crumb_width = (crumbs_visible_n == 1) ? MulDiv(56, info->dpi_x, USER_DEFAULT_SCREEN_DPI) : 1; min_crumb_width = min(min_crumb_width, crumb1->full_w); if (w + min_crumb_width <= max_crumbs_w) { /* fit as much of this crumb as possible */ crumb1->current_w = max_crumbs_w - w; w = max_crumbs_w; } else { /* take space away from the last crumb to get this one to the minimum */ struct crumb *last_crumb = (struct crumb *)list_tail(&info->crumbs); last_crumb->current_w = max(0, max_crumbs_w - min_crumb_width); crumb1->current_w = min_crumb_width; w = last_crumb->current_w + crumb1->current_w; } crumbs_visible_n += 1; } break; } w = next_w; crumbs_visible_n += 1; } ``` It's still complicated, but it can use some bits of the loop and reduce the special cases for fewer than 2 items. Nice, starting point calculations (`prev_x`) are missing, but after including them and giving it a quick test, does seem to work.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35648

Esme Povirk (＠madewokherd)

8:11 p.m.

Esme Povirk (@madewokherd) commented about dlls/comdlg32/navbar.c:

...

switch (msg) { case WM_SETFOCUS: + { + /* redirect focus back to previous window */ + SetFocus((HWND)wparam); + /* don't paint the focus rect */ return 0; /* processed */ + }

This is going to be an accessibility problem because it prevents keyboard access of the toolbar. What I meant with my earlier suggestion was to restore the focus after passing WM_LBUTTONDOWN/WM_LBUTTONDBLCLK messages to the button's wndproc, but that's not ideal either because it still leaves us with spurious focus changes. My suggestion now is to have the subclass procedure handle WM_LBUTTONDOWN/WM_LBUTTONDBLCLK by passing a WM_KEYDOWN message with VK_SPACE to the button procedure. Based on reading the Wine source code and manual testing on Windows, this should give identical behavior without changing the focus. I would also suggest giving toolbar buttons the WS_TABSTOP style. Windows allows me to tab to those buttons in the item dialog. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2993#note_35651

921

Age (days ago)

923

Last active (days ago)

List overview

Download

17 comments

3 participants

participants (3)

Esme Povirk (＠madewokherd)
Vladislav Timonin
Vladislav Timonin (＠vt)

[PATCH v3 0/11] MR2993: comdlg32: IFileDialog navigation bar control. Part 1.

tags

participants (3)