Re: Bug in win xp: Auto-detaching modeless dialog.

From: Joseph M. Newcomer (newcomer_at_flounder.com)
Date: 10/22/04 

Date: Fri, 22 Oct 2004 12:28:21 -0400

Many things wrong with the code here. Like most of it.

It is usually a good idea to use classname::IDD for the ID of the dialog box, rather than
the explicit ID. While they are the same, the classname::IDD (for classname the name of
your class) is easier to write and read.

You should not make the variable a CDialog*, but the appropriate type for your dialog!
This failure can lead to all sorts of catastrophes as you overwrite memory.

        CMyDialog * prog = new CMyDialog;
        BOOL ret = prog->Create(CMyDialog::IDD);

would be the tasteful way to create the box.

Avoid silly beginner mistakes like using a symbol like "I" (or is it "l"?) to name an
identifier. That is, is it lower-case L or capital I? HN is bad enough without things like
this contributing to its unreadability.

if(!ret) AfxMessageBox(...);

works but what happens next? If it failed, why aren't you returning? Instead, you go and a
process as if it had worked anyway! And I note that you do not actually delete the CDialog
object you created; this leads to a storage leak.

What are you doing declaring a char buffer and doing itoa? This is silly C programming.
Use a CString and CString::Format. Writing a char buffer is usually a suspect programming
practice. For one thing, it should have been TCHAR and you would have used the
Unicode-aware functions to manage it, but the whole approach is just wrong.

If you believe itoa is "more efficient" than using CString::Format, you need to rethink
your approach to programming. You are saving a few hundred instructions by uisng itoa and
then executing a few hundred thousand to perhaps a few million with that SetWindowText
call, so what, exactly is "efficient" about saving 0.0000001% of the total execution of
your program by using antiquated and error-prone methodologies?

The whole loop with a sleep in it is a colossal blunder. You have tied up the message pump
so the dialog is dead, dead, dead; the GUI is blocked, etc.

I don't even know what "detatch" means, since there is no technical description of the
word "detach" in the Windows API with respect to windows. But with a deeply flawed
structure like you show here, any malfunction dealing with the GUI is not even worthy of
discussion. You have such a fundamental design flaw that nothing else matters. Fix the
problem of sleeping in a loop that blocks the GUI first, and then you have something worth
talking about.

For example, the correct approach would be to put the modeless dialog variable as a
member variable of the CDialogTestView class, and create the dialog, and go away, having
set a timer:

void CDialgoTestView::OnTest()
   {
     if(prog != NULL)
            return; // we should not be here if a dialog is already up
     prog = new CWhateverDialog;
     if(!prog->Create(CWhateverDialog::IDD))
        {
          delete prog;
          prog = NULL;
          return;
         }
     StepsCount = MAX_STEPS;
     SetTimer(IDT_MY_TIMER, 100);
   }

void CDialogTestView::OnTimer(UINT nID, ...)
   {
     CString s;
     switch(nID)
           { /* nID */
             case IDG_MY_TIMER:
        s.Format(_T("%d"), StepsCount);
                  if(prog != NULL)
                         prog->SetWindowText(s);
                  StepsCount--;
                  if(StepsCount == 0)
                      { /* done */
                        KillTimer(IDT_MY_TIMER);
                        prog->DestroyWindow();
                        prog = NULL;
                      } /* done */
                 break;
              ... other timer events here
            } /* nID */
       return CDialog::OnTimer(...);
     }

Note that you need to make sure your modeless dialog has a PostNcDestroy handler that is
like this:

void CMyDialog::PostNcDestroy()
    {
      delete this;
    }

which is one of the many reasons that creating a variable of type CDialog makes no sense.

You have to stop thinking like a console app programmer and start thinking like a Windows
programmer. Be very, very suspicious every time you think about writing a Sleep(). 90% of
the time I see a Sleep() it is simply a fundamental design flaw. Long loops that block
the message pump but interact with the user are also suspect. Either make them threads, or
drive them from events that don't block the message pump. Asynchronous, event-driven
programming is the paradigm of choice. You are using synchronous, procedural programming,
and it just doesn't fit the Windows model.
                                        joe

                 

On 22 Oct 2004 02:12:37 -0700, ril@op.pl (Ril) wrote:

>Hi.
>If I create a modeless dialog box in win xp and in the same method I
>do some calculations before deleting this dialog then after about 5
>seconds this dialog becomes an independent window and doesn't receive
>messages from the application. This bug happens only when I start the
>project outside the developer environment. I also have to either click
>on the dialog box or cover part of my application window by another
>window. This bug occured on all computers with win xp I had access to.
>Does anybody know how I can fix it?
>
>Here are the steps to reproduce the error. You can download the
>project from http://republika.pl/pointer/dialogtest.zip
>
>1. Create an MFC application by MFC application.
>2. Create a method and fill it with the following code:
>
>void CDialogTestView::OnTest()
>{
> const int c_nStepsCount = 150;
>
> CDialog* l_Prog = new CDialog();
>
> //BOOL ret = l_Prog->Create(IDD_ABOUTBOX, NULL); // does not care if
>there is NULL or this
> BOOL ret = l_Prog->Create(IDD_ABOUTBOX, this);
> if(!ret) AfxMessageBox("Error creating Dialog");
>
> l_Prog->ShowWindow( SW_SHOW );
>
> for (int i=0; i<c_nStepsCount; i++) {
> Sleep(100); // way of sleeping is unimportant
> //ActiveSleep(100); // way of sleeping is unimportant
> char buffer[100];
> itoa(i, buffer, 10);
> l_Prog->SetWindowText(buffer); // The dialog detaches also when this
>line is removed
>
> }
> l_Prog->DestroyWindow();
>}
>
>Active sleep procedure code is the following. It doesn't care if you
>sleep actively or not:
>
>void ActiveSleep(int milisec) {
> _timeb now, stop;
>
> _ftime( &stop );
> stop.millitm += milisec;
> stop.time += stop.millitm/1000;
> stop.millitm = stop.millitm % 1000;
>
> do {
> _ftime( &now);
> } while (now.time < stop.time || (now.time == stop.time &&
>now.millitm < stop.millitm));
>}
>
>Now execute the program _outside_ the environment (either by Ctrl-F5
>or executing the binary). Wait until the number on the dialog box is
>>55 and click it. The dialog will detach, the numbers will stop
>changing and you will be able to move this dialog box.
>
>Thanks in advance
>Ril
>ril@op.pl

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