[PATCH] comctl32/taskdialog: Implement buttons.
Nikolay Sivov
bunglehead at gmail.com
Mon Apr 3 12:09:39 CDT 2017
On 03.04.2017 13:53, Fabian Maurer wrote:
> 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?
>
In addition to issues I mentioned already, I don't think this works
properly (or at all) for custom buttons. What will happen if custom
button has common id, or several custom buttons share the same id?
>
>
More information about the wine-devel
mailing list