"Joseph M. Newcomer" <newcomer@flounder.com> wrote in message
Don't pop up dialog boxes of any form in a thread. There is no reason to
do this in a
thread; that's what your mainGUI thread is for.
See below...
On Thu, 26 Apr 2007 12:44:05 -0700, Punit Kaur
<PunitKaur@discussions.microsoft.com>
wrote:
Hi,
I have a multi-threaded application. In the application there is a class
CDiskSpaceWarn corresponding to a modeless dialog box that I need to
create
in DiskSpaceCheck thread which is spawned in the Application main dialog's
Oninitdialog method.
Here is the definition of the thread:
UINT ThreadDiskSpaceWarn(LPVOID pvoid)
{
extern CDiskSpaceWarn *pdlg;
****
A locally declared extern for a global variable? Is this code for real?
(a) a global
variable is not needed (b) local extern declarations are poor style
****
long unsigned int timer=0;
****
That's UINT
****
ULARGE_INTEGER ll,pp,oo;
pdlg = new CDiskSpaceWarn;
****
Get rid of the above line.
****
GetDiskFreeSpaceEx(NULL,&ll,&pp,&oo);
float fDiskNum= (static_cast<float>(static_cast<__int64>(oo.QuadPart
/(1024
* 1024))))/1000;
for(int i=0;1==1;++i)
****
Have you ever heard of "while(TRUE)"? This is the silliest infinite loop
test I've ever
seen. You declare a variable, don't use it, use a silly test for
continuation, and
increment the counter that is never used. Some people will write
for(;;)
which will work quite nicely; I'm suggesting
BOOL running = TRUE;
while(running)
*****
{
if(fDiskNum<20.0)
{
****
SomeWindowInMainThread->PostMessage(UWM_REQUEST_DISK_SPACE_WARNING);
****
pdlg->Create(IDD_DISKSPACEWARN,AfxGetApp()->m_pMainWnd);
pdlg->ShowWindow(SW_SHOW);
pdlg->CenterWindow(NULL);
pdlg->RunModalLoop();
}
Sleep(3600000);
****
Note that you will not be able to shut this thread down for 3600 seconds,
or 6 hours if I
did the arithmetic right. It should be handled as
switch(::WaitForSingleObject(shutdown, 3600000))
{ /* wait */
case WAIT_TIMEOUT:
break;
case WAIT_OBJECT_0:
running = FALSE;
continue;
default:
... deal with
errors
} /* wait */
*****
if(!threadDiskSpaceWarn) //Closing?
break;
****
Wrong approach here; this is not tested until after you come out from the
sleep, and in
any case that is the wrong time to test it; you want to break out of the
sleep early if
you are shutting down. Create an event:
HANDLE shutdown = ::CreateEvent(NULL, TRUE, FALSE, NULL);
and when your app is shutting down, you will do
::SetEvent(shutdown);
****
}
return(0);
****
I didn't know return was a function that takes arguments; I was under the
impression the
syntax did not require parentheses (this is a common affectation, to
parenthesize the
return arguments, although it makes no sense unless there is a complex
expression that
actually requires parentheses)
return 0;
****
}
The EndDialog menthod for this modeless dialog is overridden as follows:
void CDiskSpaceWarn::OnOK()
{
// TODO: Add extra validation here
pdlg->EndModalLoop(100);
****
Get rid of the above line. You don't need it because this dialog does not
belong in a
thread. In any case, there are deeper problems here.
*****
pdlg->DestroyWindow();
CDialog::OnOK();
*****
This will probably cause some interesting disasters because CDialog::OnOK
must not be
called for modeless dialogs. Lose the above line
*****
}
The thread is doing nothing but checking the hard diskps space every one
hr
and brings up a modeless dialogbox incase the diskspace is less than 20GB.
It
works well in the Release mode. But it throws an assertion error in Debug
mode. I read somewhere that it is not a right way to use Main windows
handle
in worker threads. Could you please tell me how to go about it.?
****
No, this is a common misunderstanding of what is going on. What you are
actually saying is
"my program is defective, but if I run it in the release mode the
fundamental defect in my
program appears to be harmless because it is not checked for and
reported". It is
meaningless to EVER build a release version if the debug version is giving
assertion
failures or any other error. The key here is you make the program work,
and only then do
you entertain the possibility of building a release version.
What is wrong here is that you are creating a modeless dialog in a worker
thread. Besides
the fact that you are creating a dialog in a thread, which is already bad
practice, you
are not using a thread with a message pump, so the design is seriously
broken in two
critical places.
Get rid of the dialog in the thread. Put it in the main GUI thread.
Since it is
modeless, this does not change the user's apparent behavior (of course,
there is never a
lengthy period of time in the main thread when it is not processing
messages, right? There
are no long loops massive I/O operations, or blocking operations in the
main GUI
thread...? If there are, get rid of them.)
See my essays on messaging, worker threads, and dialog boxes on my MVP
Tips site.
joe
*****
I need to fix this asap.
Thanks
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm