Dialog receiving WM_COMMAND over and over again.

[JoeO]

I've got a dialog that under certain circumstances (when a radio
button is pressed), needs to create another thread and wait for it to
finish. While it is waiting, it processes the windows message loop
(PeekMessage with PM_NOREMOVE) and sends all messages down through the
app via AfxGetApp()->PumpMessage().

Let's call the radio button ID_DO_WORK.

If the user clicks ID_DO WORK with the mouse to start this chain of
events, every thing works fine. However if the user uses keyboard
shortcuts to "press" the radio button, I get into this hideous
nightmare. I keep getting the same WM_COMMAND with ID_DO_WORK over
and over again -- the same one I am already processing.

I cannot figure out why a keyboard activation causes this while a
mouse click does not. Should I be ignoring certain messages in my
loop? Should I be calling something besides PumpMessage().

Here is the relevant code:

// NOW WAIT ON A SINGLE MESSAGE
dwExitCode = MsgWaitForMultipleObjects (1,
(LPHANDLE)
&hThread,
false,

dwRemainingTimeOutMs,
QS_ALLEVENTS |
QS_ALLINPUT);

// NOW TEST THE EXIT CODE
if (dwExitCode == WAIT_OBJECT_0)
{
// THREAD HAS SUCCESSFULLY COMPLETED
bExitWaitLoop = true;

// THREAD GOT SIGNALLED (EXITED) OK
::GetExitCodeThread (hThread, &dwExitCode);

iRetCode = (int)dwExitCode;
}
else if (dwExitCode == WAIT_OBJECT_0 + 1)
{
// AT LEAST ONE WINDOWS MESSAGE IS WAITING TO BE
PROCESSED

while (::PeekMessage (&rxMsg, NULL, 0, 0,
PM_NOREMOVE))
{
TRACE(_T("Work thread got message 0x%X\n"),
rxMsg.message);

// SEND THE MESSAGE BACK TO THE MAIN APP
if (!AfxGetApp ()->PumpMessage ())
{
// GUARD AGAINST IT NOT BEING A QUIT MESSAGE!
::PostQuitMessage (0);

// FORCE PROGRAM TO QUIT
bExitWaitLoop = true;
}
}
}
else
{
// ASSUME A TIMEOUT HERE
bExitWaitLoop = true;

iRetCode = CWT_THREAD_TIMEDOUT;
}

[Joseph M. Newcomer]

See below...

On Fri, 20 Nov 2009 16:07:00 -0800 (PST), JoeO <joseph....@gmail.com> wrote:

>I've got a dialog that under certain circumstances (when a radio
>button is pressed), needs to create another thread and wait for it to
>finish. While it is waiting, it processes the windows message loop
>(PeekMessage with PM_NOREMOVE) and sends all messages down through the
>app via AfxGetApp()->PumpMessage().

****
None of this makes sense. If you have a dialog that needs to wait for a thread, you just
launch the thread. No special work is required to use PeekMessage or call
AfxGetApp()->PumpMessage; in fact, this is a seriously defective design. Don't do it.
****

>
>Let's call the radio button ID_DO_WORK.
>
>If the user clicks ID_DO WORK with the mouse to start this chain of
>events, every thing works fine. However if the user uses keyboard
>shortcuts to "press" the radio button, I get into this hideous
>nightmare. I keep getting the same WM_COMMAND with ID_DO_WORK over
>and over again -- the same one I am already processing.

*****
How are you causing the dialog to recognize a keyboard shortcut? That would be important
information.
****

>
>I cannot figure out why a keyboard activation causes this while a
>mouse click does not. Should I be ignoring certain messages in my
>loop? Should I be calling something besides PumpMessage().
>
>Here is the relevant code:
>
>
> // NOW WAIT ON A SINGLE MESSAGE
> dwExitCode = MsgWaitForMultipleObjects (1,
> (LPHANDLE)

****
Why a cast? Isn't &hThread already an LPHANDLE? If it is, you don't need the cast; if it
isn't, you shouldn't be casting it!
****

****
Throw away every single line of the above code, no exceptions. Get rid of it.

If I were doing this, I would do

void CMyDialog::OnStartThread()
{
ThreadRunning = TRUE;
CWinThread * thread = AfxBeginThread(function, whatever);
if(thread == NULL)
{
ThreadRunning = FALSE;
...perhaps report error here
return;
}
updateControls();
}

And that is all I would do. Then I would have my thread do

/* static */ UINT CMyDialog::function(LPVOID p)
{
...I would extract the CMyDialog* object from p, either because
...I had done 'this' for the second parameter of AfxBeginThread
...or because one of the members of the structure I passed in
...contained my CMyDialog* object

... thread stuff here

dlg->PostMessage(UWM_THREAD_IS_FINISHED);
return 0;
}

Note that there is NO need to do the incredibly elaborate and complex (and error-prone)
code that you are doing. Then I would have a handler

LRESULT CMyDialog::OnThreadIsFinished(WPARAM, LPARAM)
{
ThreadRunning = FALSE;
updateControls();
return 0;
}

which would be handled by having an entry in the message map

ON_REGISTERED_MESSAGE(UWM_THREAD_IS_FINISHED, OnThreadIsFinished)

where I indicate that this is a Registered Window Message.

Now in the updateControls handler, you would simply disable any controls that were
inappropriate if the thread was running. If you have menu items, and you have implemented
OnUpdate handles for them, the handlers would disable the menu items if the thread is
running.

I wouldn't even attempt to try to understand that mess you have. I wouldn't waste time
trying to debug such a bizarre piece of code. Simplify it. If the problem still exists,
THEN there is something worth asking about.

If this piece of code appeared on my desk as something to fix, the first thing I would do
would be to rip it out completely. I have no idea why you need such a complex solution to
what is a fundamentally trivial problem!

Read my essay on dialog control management and my essay on worker threads on my MVP Tips
site. Also my essay on message management.
joe
****
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

[JoeO]

First of all, I agree that this is hideous. I did not write this
code and I would not do it that way either but there is too much other
legacy code depends upon it. Not in a clean, black-box sort of way
but in a very ugly, procedural, sort of way. In short: I cannot
simply rip it out any multithreading and throw it away and start over
at this point. Not given the time constraints I have. We are near a
release. There are simply too many use-cases to test.

I already have a request to redo this mess for the next release but
for now I need to find something that makes as few waves as possible.

However I think I might have made the intent of the code unclear.
This operation is intended to be modal. The user is not allowed to do
anything while the thread is doing its work except to cancel the
operation. The parent dialog hands the task off to somebody's custom
thread class. The custom thread class creates a small, intermediate
modeless dialog with a cancel button and a progress control. It
creates the dialog, starts the thread and then enters a manual message
loop. The progress dialog has a little timer that checks the
progress, updates the progress control, and waits for a CANCEL..

I suppose I could try to make the progress dialog modal and allow its
message loop to handle things but I'm hoping to find the smallest code
change possible right now.

[Joseph M. Newcomer]

Using a thread in a modal fashion is like implementing a complicated and expensive
subroutine call. Note that you can do this just as readily by launching a modal dialog
with a "Cancel" button and doing what I showed. Therefore, the modal dialog already has
all the logic for (a) preventing anything else from happening and (b) has an
MFC-compatible message pump, for which you are just duplicating all the work.

Someone totally clueless about a lot of things wrote this code. Creating a *modeless*
dialog box and then simulating modality with this mess of code is really bizarre!

What you didn't explain was how you are getting the accelerator keys to work; I suspect
problems in that area. Note that by default, accelerator keys do not work in dialog-based
apps, so you have to be doing something special. Given the total cluelessness this code
demonstrates, I would be *very* suspect about how the accelerator keys are being handled
(probably some other home-spun grotesquerie, instead of the obvious and documented
solution for getting accelerator keys to work in dialogs).
joe

[JoeO]

These "accelerators" are normal dialog keyboard shortcuts. The
buttons in question are 3 radio buttons. The user may activate them
through normal keyboard navigation in a dialog. He may use the tab
key to get to the group of radio buttons, then use the arrow keys. Or
he may simply use the keyboard shortcut that one gets for free when
one uses an ampersand in the text of a control. Either way the the
dialog receives a BN_CLICKED notification for the activated radio
button, just as if the user had clicked the dialog button with the
mouse.

The problem I am encountering occurs only when the user does things
this way, with the keyboard rather than with the mouse.

Note also, this is not a dialog-based application. It is a massive
MDI application that presents a modal dialog. The user clicks the
modal dialog (or uses the keyboard) and it starts this whole ugly
process.

I believe the reason they are simulating modality here is that this
code is used so often for so many different tasks, that some of the
tasks want modality and some do not.

You have no need to convince me of the ugliness of this design. You
are preaching to the choir.

[Joseph M. Newcomer]

See below...

On Mon, 23 Nov 2009 09:12:43 -0800 (PST), JoeO <joseph....@gmail.com> wrote:

>These "accelerators" are normal dialog keyboard shortcuts. The
>buttons in question are 3 radio buttons. The user may activate them
>through normal keyboard navigation in a dialog. He may use the tab
>key to get to the group of radio buttons, then use the arrow keys. Or
>he may simply use the keyboard shortcut that one gets for free when
>one uses an ampersand in the text of a control. Either way the the
>dialog receives a BN_CLICKED notification for the activated radio
>button, just as if the user had clicked the dialog button with the
>mouse.

****
OK, that clarifies things a lot. I'll try a couple experiments to see if I can figure out
what is going wrong. I don't see any reason the even should recur.

Unless the handler somehow is forcing a SetFocus that is sending another BN_CLICKED
message for some unknown reason. Look for SetFocus calls.
****

>
>The problem I am encountering occurs only when the user does things
>this way, with the keyboard rather than with the mouse.
>
>Note also, this is not a dialog-based application. It is a massive
>MDI application that presents a modal dialog. The user clicks the
>modal dialog (or uses the keyboard) and it starts this whole ugly
>process.

****
Sigh. Then there is even less excuse for this overly-complex simulation of a modal dialog
box. I've seen this solution posted, but it always struck me as creating a complex
solution to a trivial problem.
****

>
>I believe the reason they are simulating modality here is that this
>code is used so often for so many different tasks, that some of the
>tasks want modality and some do not.

****
The modality would be in how it is called, and the solution shown here is equally
unsuitable in either case; the simpler solution always works correctly in both cases.
****

>
>You have no need to convince me of the ugliness of this design. You
>are preaching to the choir.

****
It is sad we end up inheriting these messes. I just had to clean up one that used a
secondary thread to handle synchronous I/O when the obviously correct solution was to have
coded it to use asynchronous I/O with an I/O Completion Port. The architecture was so
screwed up that only a total rewrite could have salvaged it, and they couldn't do that. So
I had to rewrite the threaded code. At least I managed to remove all the locks! The
original programmer would enter a critical section to store to a single BOOL, and enter
the same critical section to read the single BOOL! D'oh!
joe
****

[JoeO]

Given my time constraints, I suspect my best bet is a simple band-aid
in each of the 3 command handlers to detect and prevent re-entrancy.
No doubt it leaves a swiss-cheese network of holes but by plugging
this one, at least I will not create others.

Next release will be a serious rewrite.

Thanks for your time

[catnip]

I tripped over this too. What I do now is to use BM_SETCHECK to check
the state of the radio buttons. I base any action I might take on
whether a radio button has changed state, rather than just relying on
the fact that a WM_COMMAND message has turned up.

HTH - Paul Sanders.

[Joseph M. Newcomer]

I've never relied on the fact that a button message comes in, even for radio buttons. But
the OP's problem was repeated messages.

Note that the BM_SETCHECK message is the actual implementation of
ctrl.SetCheck(BST_CHECKED)
so there would never be a need to explicitly do a SendMessage yourself.
joe