C++/VB - Dialog receiving WM_COMMAND over and over again.

Asked By JoeO

20-Nov-09 07:16 PM

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

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!

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

iRetCode = CWT_THREAD_TIMEDOUT;
}

21-Nov-09 09:52 PM

See below...

****
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.
****
*****
How are you causing the dialog to recognize a keyboard shortcut?  That would be important
information.
****
****
Why a cast?  Isn't &hThread already an LPHANDLE?  If it is, you do not need the cast; if it
is not, you should not 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

22-Nov-09 05:24 PM

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 am hoping to find the smallest code
change possible right now.

23-Nov-09 09:25 AM

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



Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

23-Nov-09 12:37 PM

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.

23-Nov-09 12:55 PM

See below...

****
OK, that clarifies things a lot.  I will try a couple experiments to see if I can figure out
what is going wrong.  I do not 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.
****
****
Sigh.  Then there is even less excuse for this overly-complex simulation of a modal dialog
box.  I have seen this solution posted, but it always struck me as creating a complex
solution to a trivial problem.
****
****
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.
****
****
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 could not 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
****
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

23-Nov-09 03:07 PM

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

02-Dec-09 08:29 AM

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.

02-Dec-09 10:53 AM

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

Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm