The Nightstar Zoo

Nightstar IRC Network - irc.nightstar.net
It is currently Thu Dec 14, 2017 10:15 am

All times are UTC - 6 hours [ DST ]




Post new topic Reply to topic  [ 6 posts ] 
Author Message
PostPosted: Fri Jul 02, 2004 2:03 pm 
Code:
if (szOutputText) new COutputDialogBox(szOutputText);


I created a fire-and-forget nonmodal dialog box at work, and the above is how I'm invoking it. The idea is that, in response to some user action, I need to be able to output a blob of text to the user in a windows app, but because it could take some time to read the text (and the user may want to keep it around for C&P etc) I need it to not block my thread ala modal dialog. The dialog has one editbox for the text, and an 'OK' button. In the ctor, the object creates the dialog and dumps the text into the editbox, then when the user clicks the button, the object closes the dialog box and then deletes 'this'.

MS has a code scanning tool which scans every checkin to the source repository, and checks for a number of common programming mistakes or logic errors. It's a useful tool, and it's caught a number of my stupid mistakes like not checking API return values, and potential buffer overruns. This time, it caught the above line, labeled it a memory leak, and dutifully filed a bug against my code. As of this writing, I have just received word from a PM (managerial-type) that I have to rewrite the code to appease the tool.

Aside from being a little miffed that the PM trusts an automated tool more than they trust their programmers, I got to wondering about that little snippet of code, and the design ideas that went into it. Most people here are pretty aware that I'm a hardcore procedural coder; it's how I think... At the time, I was proud of myself for creating a dialog class with so much OO shininess. Now I'm not sure.

So I guess my question is this: Is the above code too clever for its own good, or just clever?


Top
  
 
 Post subject:
PostPosted: Fri Jul 02, 2004 2:21 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
Moderator Edit: I promoted this post of Pi's to a new thread.

Here's a solution that I think will work. Interestingly, it is not very OO.

Code:
if (szOutputText) DoModelessDialogBox(szOutputText);

...

void DoModelessDialogBox(const char *szMsg)
{
    // spawn a new thread that expires when its main function exits.
    // discard the thread id, we do not need to talk with it.
    // Note that I can't remember the name of the function.  :-)
    AfxMumbleCreateThread(LaunchOutputDialogBox, szMsg);
}


// This function runs in its own thread. 
AfxMumbleThreadReturn LaunchOutputDialogBox(const char *szMsg)
{
    COutputDialogBox(szOutputText);
}


Now, DoModelessDialogBox is, of necessity, "leaky" in terms of spawning a thread and returning no control to it.

You could write COutputDialogBox so that it launched the thread and then abandoned it, and simply drop the new. But there are some problems with that approach:
  1. That code statement implies to the reader that you're constructing a COutputDialogBox from the message, it's constructor does everything you need, and then it goes away. You would need to document every call with "by the way, this creates a persistent modeless dialog that hangs around until dismissed." Code that requires documentation is smelly.
  2. AfxMumbleCreateThread requires a static function to run. That's okay, I guess; you could define a static method in the class for it to call. This feels really kludgy.
  3. The created dialog box has a message pump. If you launch the dialog via the constructor and using a static thread function, the object is then destroyed and there's nothing left to handle the messages.


That last one could in theory lead you to have the constructor create a dialog object, but if you think that through very far you'll either end up with the constructor calling a new without a matched delete, or the constructor ends up being a function call in an empty object, and you can do away with that object and rename the function DoModelessDialogBox.

There might be a good OO way to do this, but I don't know what it is. It probably involves objects inherited from CThread or something that are designed to launch themselves (or something else) in their own thread and return the thread id to the caller.


Top
 Profile  
 
 Post subject:
PostPosted: Fri Jul 02, 2004 2:24 pm 
What was wrong with the solutions suggested in the other thread? :P


Top
  
 
 Post subject:
PostPosted: Fri Jul 02, 2004 2:46 pm 
I've considered Chalain's spawned-thread option. In the context of the app I'm writing, a whole new thread per dialog (there can be several on the screen at once) seems like using a sledge on a pushpin, but it does solve one sticky problem I'm having with the class. It's implemented in ATL (OO window handling without the MFC gorilla on your back). The 'OK' button handler calls DestroyWindow(), which then synchronously sends WM_DESTROY to the window. The WM_DESTROY handler then calls delete this; and returns. After that, the OK handler returns, and ATL sets some data on the class.

A new thread and making the dialog modal would fix this problem.

So would scrapping ATL and rolling my own static PC dialog code.


Top
  
 
 Post subject:
PostPosted: Fri Jul 02, 2004 3:47 pm 
Their tool is too stupid for its own good. If it can't see a delete this; :) At the same time, the thing really has no way of telling that the OK button will ever get called.
Does it require that the object that creates it be the one that destroys it?

Another idea: An object that is a container of dialog boxes. You have it store the dialog boxes and delete them when they're no longer needed.

Probably make it check all dialog boxes for a closed flag when it creates a new one, or on destruction. You can just instantiate one of these objects for each form you have, then or create a dialog box and toss it to the holder object.

Code:
if (szOutputText) dBoxHolder.add(new COutputDialogBox(szOutputText));


DBoxHolder.add would take in a base class for all your different non-modal fire and forget dialog boxes.

To me, having it delete itself would use less memory in the long run, but this should satisfy the code checker.

You could go even more exotic and have DBoxHolder pass a function to the dialog box that it would call when the OK button is closed, so that DBox could instantly destroy it. Eh, that would probably be too clever for its own good though.


Top
  
 
 Post subject:
PostPosted: Fri Jul 02, 2004 6:13 pm 
Pi wrote:
It's implemented in ATL. The 'OK' button handler calls DestroyWindow(), which then synchronously sends WM_DESTROY to the window. The WM_DESTROY handler then calls delete this; and returns. After that, the OK handler returns, and ATL sets some data on the class.

:stfw:


Top
  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 6 posts ] 

All times are UTC - 6 hours [ DST ]


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group