[PATCH] comctl32/taskdialog: Implement buttons.
Fabian Maurer
dark.shadow4 at web.de
Mon Apr 3 05:53:22 CDT 2017
On Freitag, 24. März 2017 16:50:38 CEST Fabian Maurer wrote:
> > > -static const UINT DIALOG_MIN_WIDTH = 180;
> > > +static const UINT DIALOG_MIN_WIDTH = 240;
> > >
> > > static const UINT DIALOG_SPACING = 5;
> > >
> > > -static const UINT DIALOG_BUTTON_WIDTH = 50;
> > > -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> > > +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> > > +static const UINT DIALOG_BUTTON_HEIGHT = 13;
> >
> > This was taken from MSGBOX template, what's a point in making it
> > different?
>
> I just tested it on my Win7 box and tried to get it to look as similar as
> possible. Where do the MSGBOX template values come from? I can remove that
> change, if those values are better.
>
> > Did you test that buttons are supposed to support multiple lines of text?
>
> Normal buttons don't allow multiple lines of text. But we only use the right
> side of the rectangle anyways, so it works as expected. Problem is, on
> windows and on wine the buttons clips the text when it gets too long, so
> I'm not fully it's the correct way to implement this. Should I change this?
> There's also the fact that buttons get larger in height on windows, even if
> the text is a single line only. Should I reproduce this?
> Moreover, wine displays newlines as boxes while this isn't the case on
> windows. Not sure how to deal with this though.
>
> > > static unsigned int taskdialog_add_control(struct
> > > taskdialog_template_desc *desc, WORD id, const WCHAR *class,>
> > >
> > > - HINSTANCE hInstance, const WCHAR *text, short x, short y, short
> > > cx, short cy) + DWORD style, HINSTANCE hInstance, const WCHAR
> > > *text, short x, short y, short cx, short cy)>
> > >
> > > {
> > >
> > > struct taskdialog_control *control = Alloc(sizeof(*control));
> > > unsigned int size, class_size, text_size;
> > >
> > > @@ -117,7 +167,7 @@ static unsigned int taskdialog_add_control(struct
> > > taskdialog_template_desc *desc>
> > >
> > > control->template = template = Alloc(size);
> > > control->template_size = size;
> > >
> > > - template->style = WS_VISIBLE;
> > > + template->style = WS_VISIBLE | WS_CHILD | style;
> >
> > You don't use styles in this patch, this is unnecessary change.
>
> I'm using BS_PUSHBUTTON for the buttons, is this unnecessary?
>
> > > template->dwExtendedStyle = 0;
> > > template->x = x;
> > > template->y = y;
> > >
> > > @@ -136,78 +186,126 @@ static unsigned int taskdialog_add_control(struct
> > > taskdialog_template_desc *desc>
> > >
> > > static unsigned int taskdialog_add_main_instruction(struct
> > > taskdialog_template_desc *desc) {
> > >
> > > - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /*
> > > padding left and right of the control */ - const WCHAR *textW = NULL;
> > > - unsigned int size, length;
> > > - HFONT oldfont;
> > > - HDC hdc;
> > > + unsigned int size;
> > > + RECT rect;
> > >
> > > if (!desc->taskconfig->pszMainInstruction)
> > >
> > > return 0;
> > >
> > > - if (IS_INTRESOURCE(desc->taskconfig->pszMainInstruction))
> > > - {
> > > - if (!(length = LoadStringW(desc->taskconfig->hInstance,
> > > (UINT_PTR)desc->taskconfig->pszMainInstruction, - (WCHAR
> > > *)&textW, 0)))
> > > - {
> > > - WARN("Failed to load main instruction text\n");
> > > - return 0;
> > > - }
> > > - }
> > > - else
> > > - {
> > > - textW = desc->taskconfig->pszMainInstruction;
> > > - length = strlenW(textW);
> > > - }
> > > -
> > > - hdc = GetDC(0);
> > > - oldfont = SelectObject(hdc, desc->font);
> > > -
> > > - dialogunits_to_pixels(desc, &rect.right, NULL);
> > > - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS |
> > > DT_CALCRECT | DT_WORDBREAK); - pixels_to_dialogunits(desc,
> > > &rect.right, &rect.bottom);
> > > -
> > > - SelectObject(hdc, oldfont);
> > > - ReleaseDC(0, hdc);
> > > + rect = text_get_rect(desc, desc->taskconfig->pszMainInstruction);
> > >
> > > - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION,
> > > WC_STATICW,
> > > desc->taskconfig->hInstance, + size = taskdialog_add_control(desc,
> > > ID_MAIN_INSTRUCTION, WC_STATICW, 0, desc->taskconfig->hInstance,>
> > >
> > > desc->taskconfig->pszMainInstruction, DIALOG_SPACING,
> > > desc->dialog_height,
> > > rect.right, rect.bottom);
> > >
> > > - desc->dialog_height += rect.bottom;
> > > + desc->dialog_height += rect.bottom + DIALOG_SPACING;
> > >
> > > return size;
> > >
> > > }
> > >
> > > -static unsigned int taskdialog_add_common_buttons(struct
> > > taskdialog_template_desc *desc) +static button_info make_button(struct
> > > taskdialog_template_desc *desc, int id, const WCHAR *text) +{
> > > + RECT rect;
> > > + button_info button;
> > > +
> > > + button.id = id;
> > > + button.text = text;
> > > + rect = text_get_rect(desc, text);
> > > + button.width = rect.right + 10;
> > > + if (button.width < DIALOG_BUTTON_WIDTH_MIN)
> > > + button.width = DIALOG_BUTTON_WIDTH_MIN;
> > > +
> > > + return button;
> > > +}
> >
> > This does not handle "text" correctly.
>
> Sorry, but I don't see the problem here. Do you mind explaining?
> In both text_get_rect and later on taskdialog_add_control both text or a
> resource-ID are allowed.
>
> > > +static unsigned int taskdialog_add_buttons(struct
> > > taskdialog_template_desc *desc)>
> > >
> > > {
> > >
> > > - short button_x = desc->dialog_width - DIALOG_BUTTON_WIDTH -
> > > DIALOG_SPACING;>
> > >
> > > DWORD flags = desc->taskconfig->dwCommonButtons;
> > >
> > > + const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
> > > + UINT alignment = DIALOG_SPACING_BUTTONS_LEFT; /* minimum distance
> > > from the left dialog border */ + UINT location_x;
> > > + BOOL first_row = TRUE;
> > > + button_info *buttons;
> > > + int count = 0;
> > > + int i;
> > >
> > > unsigned int size = 0;
> > >
> > > -#define TASKDIALOG_ADD_COMMON_BUTTON(id) \
> > > - do { \
> > > - size += taskdialog_add_control(desc, ID##id, WC_BUTTONW,
> > > COMCTL32_hModule, MAKEINTRESOURCEW(IDS_BUTTON_##id), \ -
> > > button_x, desc->dialog_height + DIALOG_SPACING, DIALOG_BUTTON_WIDTH,
> > > DIALOG_BUTTON_HEIGHT); \ - button_x -= DIALOG_BUTTON_WIDTH +
> > > DIALOG_SPACING; \
> > > - } while(0)
> > > - if (flags & TDCBF_CLOSE_BUTTON)
> > > - TASKDIALOG_ADD_COMMON_BUTTON(CLOSE);
> > > - if (flags & TDCBF_CANCEL_BUTTON)
> > > - TASKDIALOG_ADD_COMMON_BUTTON(CANCEL);
> > > - if (flags & TDCBF_RETRY_BUTTON)
> > > - TASKDIALOG_ADD_COMMON_BUTTON(RETRY);
> > > - if (flags & TDCBF_NO_BUTTON)
> > > - TASKDIALOG_ADD_COMMON_BUTTON(NO);
> > > - if (flags & TDCBF_YES_BUTTON)
> > > - TASKDIALOG_ADD_COMMON_BUTTON(YES);
> > > + /* Allocate enough memory for the custom and the default buttons */
> > > + if (taskconfig->cButtons && taskconfig->pButtons)
> > > + buttons = HeapAlloc(GetProcessHeap(), 0, (taskconfig->cButtons
> > > +
> > > 6) * sizeof(button_info)); + else
> > > + {
> > > + buttons = HeapAlloc(GetProcessHeap(), 0, 6 *
> > > sizeof(button_info));
> > > + }
> > > +
> > > + /* Custom buttons */
> > > + if (taskconfig->cButtons && taskconfig->pButtons)
> > > + {
> > > + for (i = 0; i < taskconfig->cButtons; i++)
> > > + {
> > > + buttons[count++] = make_button(desc,
> > > taskconfig->pButtons[i].nButtonID,
> > > taskconfig->pButtons[i].pszButtonText); + }
> > > + }
> > > +
> > > + /* Default buttons */
> > >
> > > if (flags & TDCBF_OK_BUTTON)
> > >
> > > - TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > > - /* Always add OK button */
> > > - if (list_empty(&desc->controls))
> > > - TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > > -#undef TASKDIALOG_ADD_COMMON_BUTTON
> > > -
> > > - /* make room for common buttons row */
> > > - desc->dialog_height += DIALOG_BUTTON_HEIGHT + 2 * DIALOG_SPACING;
> > > + buttons[count++] = make_button(desc, IDOK,
> > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); + if (flags & TDCBF_YES_BUTTON)
> > > + buttons[count++] = make_button(desc, IDYES,
> > > MAKEINTRESOURCEW(IDS_BUTTON_YES)); + if (flags & TDCBF_NO_BUTTON)
> > > + buttons[count++] = make_button(desc, IDNO,
> > > MAKEINTRESOURCEW(IDS_BUTTON_NO)); + if (flags & TDCBF_RETRY_BUTTON)
> > > + buttons[count++] = make_button(desc, IDRETRY,
> > > MAKEINTRESOURCEW(IDS_BUTTON_RETRY)); + if (flags &
> > > TDCBF_CANCEL_BUTTON)
> > > + buttons[count++] = make_button(desc, IDCANCEL,
> > > MAKEINTRESOURCEW(IDS_BUTTON_CANCEL)); + if (flags &
> > > TDCBF_CLOSE_BUTTON)
> > > + buttons[count++] = make_button(desc, IDCLOSE,
> > > MAKEINTRESOURCEW(IDS_BUTTON_CLOSE)); +
> > > + /* There must be at least one button */
> > > + if (count == 0)
> > > + buttons[count++] = make_button(desc, IDOK,
> > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +
> > > + /* Position buttons */
> > > + location_x = alignment;
> > > + for (i = 0; i < count; i++)
> > > + {
> > > + /* When beginning new row, align the first */
> > > + if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W >
> > > desc->dialog_width) + {
> > > + if (first_row)
> > > + {
> > > + int diff = desc->dialog_width - location_x;
> > > +
> > > + first_row = FALSE;
> > > + for (int j = 0; j < i; j++) /* Align first row to the
> > > right */ + buttons[j].x += diff;
> > > + alignment = buttons[0].x; /* left-align all coming rows
> > > to the first row */ + }
> > > + location_x = alignment;
> > > + desc->dialog_height += DIALOG_BUTTON_HEIGHT +
> > > DIALOG_SPACING_BUTTON_H; + }
> > > +
> > > + buttons[i].x = location_x;
> > > + buttons[i].y = desc->dialog_height;
> > > +
> > > + location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> > > + }
> > > + if (first_row) /* Always align first row to the right */
> > > + {
> > > + int diff = desc->dialog_width - (buttons[count - 1].x +
> > > buttons[count - 1].width + DIALOG_SPACING_BUTTON_W); + for (int i
> > > = 0; i < count; i++)
> > > + buttons[i].x += diff;
> > > + }
> >
> > I don't like this. Why do you need temporary array, and iterating twice
> > seems excessive.
>
> The problem is, a later button might influence the position of the first
> button. That's why I created them as temporary array and then change their
> position once I know how.
> I also could iterate the loop twice I guess, but that sounded a bit ugly,
> too. If you have a better idea to address this problem, I'd be happy to
> hear it.
> > > + for (i = 0; i < count; i++)
> > > + {
> > > + size += taskdialog_add_control(desc, buttons[i].id, WC_BUTTONW,
> > > BS_PUSHBUTTON, COMCTL32_hModule, + buttons[i].text,
> > > buttons[i].x, buttons[i].y, buttons[i].width, DIALOG_BUTTON_HEIGHT); +
> > > }
> >
> > This won't work for custom buttons.
>
> Right, I missed that, thanks for pointing it out.
>
> > > - size += taskdialog_add_common_buttons(&desc);
> > > + size += taskdialog_add_buttons(&desc);
> >
> > I'd rather have it separated if possible. I'll do some testing myself,
> > before commenting further.
>
> I see. Just figured it would make sense to handle all buttons in one
> function, since they are aligned together - if no command-links are used.
> Is there a reason you want it separated?
>
>
> Regards,
> Fabian Maurer
Any update on this?
More information about the wine-devel
mailing list