taskmodal messagebox implementation, take 3

[Juan Lang]

Hi Ivan.

--- Ivan Leo Puoti <puoti at inwind.it> wrote:
> Please consider I'm no programming wiz and I'm learning as I go along.

No problem.  I'll give a lengthy reply in the hope it can help other folks
dealing with this sort of thing too.

> Now, I don't see how else I could get MSGBOX_EnumProc() to remember the
> values
> of those that now are static variables,  apart from using the heap for
> everything, that seems a bit inefficent as we know how much data we want
> to
> store, or global variables, do you think I should use global ones
> instead?
> If not suggestions are welcome.

Globals are also not thread-safe.

Generally for thread safety, I'd suggest the following: declare a
structure that has all the data you want to modify in MSGBOX_EnumProc,
something like:
struct MsgBoxEnumProcParams
{
  int pass;
  int counter1;
  int counter2;
  int heapcheck;
  HWND *handles;
};

In MessageBoxIndirectW, create one of these and pass its address as the
LPARAM value to EnumThreadWindows().

However, this ignores style issues.  I have problems with names like
"counter1" and "counter2"; they don't tell me what they mean.  Also, I
don't like the magic values 0, 1, 2, 3, 4 for the LPARAM value; at the
least these should be symbolic constants.

Pass 3 doesn't modify the parameters at all, so it really should get its
own callback function.

It looks like heapcheck's purpose is to see whether handles has in fact
been allocated.  You could instead make sure to set handles to NULL when
it isn't allocated.  Also, there's the possibility of a memory leak in
pass 4.  I'd suggest the caller should be responsible for memory
allocation.

With these suggestions, I've come up with the following changes.  I
haven't checked whether this code works (or even compiles), it's just my
attempt at rewriting what you're trying to do:

struct ThreadWindows
{
    UINT  numHandles;
    UINT  handlesLen;
    HWND *handles;
};

BOOL CALLBACK MSGBOX_CountDisabledThreadWindows(HWND hwnd, LPARAM lParam)
{
    if (!lParam)
        return FALSE;
    if (!IsWindowEnabled(hwnd))
        *(UINT *)lParam++;
    return TRUE;
}

BOOL CALLBACK MSGBOX_EnumDisabledThreadWindows(HWND hwnd, LPARAM lParam)
{
    if (!lParam)
        return FALSE;
    if (!IsWindowEnabled(hwnd))
    {
        struct ThreadWindows *threadWindows =
         (struct ThreadWindows *)lParam;

        if (threadWindows->numHandles < threadWindows->handlesLen)
            threadWindows->handles[threadWindows->numHandles++] = hwnd;
    }
    return TRUE;
}

INT WINAPI MessageBoxIndirectW( LPMSGBOXPARAMSW msgbox )
{
    LPVOID tmplate;
    HRSRC hRes;
    int ret;
    UINT i;
    struct ThreadWindows threadWindows = { 0, 0, 0 };
    static const WCHAR msg_box_res_nameW[] =
     { 'M','S','G','B','O','X',0 };

    if (!(hRes = FindResourceExW(user32_module, (LPWSTR)RT_DIALOG,
     msg_box_res_nameW, msgbox->dwLanguageId)))
        return 0;
    if (!(tmplate = (LPVOID)LoadResource(user32_module, hRes)))
        return 0;

    /*handle task modal message boxes*/
    if ((msgbox->dwStyle & MB_TASKMODAL) && (msgbox->hwndOwner==NULL))
    {
        EnumThreadWindows(GetCurrentThreadId(),
         MSGBOX_CountDisabledThreadWindows,
         (LPARAM)&threadWindows.numHandles);
        if (threadWindows.numHandles)
        {
            threadWindows.handles = (HWND *)HeapAlloc(GetProcessHeap(),
             0, threadWindows.numHandles * sizeof(HWND));
            if (threadWindows.handles)
                threadWindows.handlesLen = threadWindows.numHandles;
            threadWindows.numHandles = 0;
        }
        EnumThreadWindows(GetCurrentThreadId(),
         MSGBOX_EnumDisabledThreadWindows, (LPARAM)&threadWindows);
        for (i = 0; i < threadWindows.numHandles; i++)
            DIALOG_DisableOwner(threadWindows.handles[i]);
    }

    ret=DialogBoxIndirectParamW(msgbox->hInstance, tmplate,
     msgbox->hwndOwner, MSGBOX_DlgProc, (LPARAM)msgbox);

    if ((msgbox->dwStyle & MB_TASKMODAL) && (msgbox->hwndOwner==NULL))
    {
        for (i = 0; i < threadWindows.numHandles; i++)
            DIALOG_EnableOwner(threadWindows.handles[i]);
        if (threadWindows.handles)
            HeapFree(GetProcessHeap(), 0, threadWindows.handles);
    }

    return ret;
}

The changes I've made:

I've renamed counter1 and counter2 to numHandles and handlesLen. 
handlesLen is the allocated number of handles, while numHandles is the
number of handles actually stored.  (Your code already dealt with the
possibility of their being different: well done!)

I've also used handles as its own guard; if it's NULL, it's not allocated.
 Otherwise it is.

Like I suggested, I pass a pointer as an LPARAM to the two
EnumThreadWindows callback functions.

It seems two calls to EnumThreadWindows were unnecessary, since we already
knew the window handles, so I just iterate over the values in handles
rather than calling EnumThreadWindows again.  If I'm incorrect in my
understanding, of course change it.

And, the caller is always responsible for memory allocation and freeing. 
I think this is safer than having the callback function do it.

> >Any reason you aren't using HeapAlloc instead?
> As this heap is only used by MSGBOX_EnumProc(), is there any reason why
> I should
> use HeapAlloc?

It's sometimes easier to debug memory problems with the Windows heaps than
with the libc ones.

--Juan


		
__________________________________ 
Do you Yahoo!? 
Check out the new Yahoo! Front Page. 
www.yahoo.com