[PATCH] comctl32/taskdialog: Implement buttons.

Fabian Maurer dark.shadow4 at web.de
Fri Mar 24 10:50:38 CDT 2017


> > -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





More information about the wine-devel mailing list