Re: Waiting for Dialog to update in a Nnew Thread

See below...
On Sat, 05 Nov 2005 10:18:07 GMT, "Alan Hadley" <alan_hadley@xxxxxxxxxxxxxxxx> wrote:

>I have a main thread that controls all of the UI and calculations. I want
>another thread to maintain a progress/log, and also to show a Cancel button
>to interrupt long calculations in the main thread, the main window is hidden
>during these calculations but the progress box is vizible.
****
Wrong approach. Put the calculations in a thread, and all the UI features in the main GUI
thread.
****
>
>I want a list control in the progress box to say what stage the calculations
>have reached.
****
See above
****
>
>I have got the thing working by running the processing functions in new
>threads but the code is messy, and hard to maintain. So I want to do
>something like this:-
****
Define "messy" and "hard to maintain". Mostly, working cross-thread is quite
straightforward, and easy to maintain, if you take the right approach. Multithreaded GUI
interfaces are nearly impossible to get right, and although you have have the temporary
illusion you have solved the proble, you almost certainly have not, and in fact created
something far worse
****
>
>// this is a function from my dialog class
>void CProgress::AddString(char *s)
>{
> m_list.AddString(s);
> m_list.SetTopIndex(m_list.GetCount()-1);
> UpdateWindow();
****
UpdateWIndow should not be necessary. SetTopIndex will cause a redraw.
****
>}
>
>// This function is in the same class as the instance of the dialog above,
>the thread class
> //{{AFX_MSG_MAP(CProgressThread)
> ON_THREAD_MESSAGE(WM_PROGRESS_ADD,Add)
****
Put this dialog in the main GUI thread, do the computations in a secondary thread, and do
not use thread messages to the main UI thread. In fact, do not ever use thread messages
to any thread that has user-visible UI components
****
>void CProgressThread::Add(WPARAM w,LPARAM l)
>{
> HANDLE event;
> event=OpenEvent(EVENT_ALL_ACCESS,true,"AddEvent");
****
Already you are in trouble. What possible value could an event have here? If you need
one, your basic approach is wrong. Rewrite it
****
> progress.AddString((char*)l); // ***
> SetEvent(event);
> CloseHandle(event);
****
None of the event-related code makes sense. Lose all of it
****
>}
>
>// This functuion is in the main processing class, I want it to wait until
>the text appears in the dialog
>char* t[128];
****
Why do you think char * makes sense? On the whole, you should forget that char exists
except in very, very limited conditions, and this is not one of them. And why is the
"magic number" 127 hardwired into this? Use #define. Use LPTSTR, although it is not
clear why you need an array of 128 string pointers; if you did, you would have chosen
something like CStringArray instead of char *.
****
>void CCombineDoc::AddString(CString s)
>{
> if(s.GetLength()>127) s=s.Left(127);
> strcpy(t,s);
****
So you are copying into an undefined and uninitialized pointer to a string? Why are you
using strcpy? Why isn't 't' a CString? Why is it needed at all? strcpy is considered a
prime example of unsafe, unmaintainable, and error-prone code. There is a simple rule: if
you write strcpy, you have made a design error. There *might* be some very exotic
conditions under which strcpy might be used, but this is not among them. The whole use of
char * and strcpy is a mistake. Not to mention the wrong data type, the fixed-size
buffer, and a few other messy and unmaintainable misfeatures. If you're going to talk
about "messy" and "hard to maintain", the first thing you have to do is write code that is
in and of itself non-messy and maintainable. This code does not meet that criterion.
****
> HANDLE event;
> event=CreateEvent(0,true,false,"AddEvent");
*****
This code makes no sense. THe presence of an event is completely inappropriate here. Lose
it and all code related to it. You said the alternatives were "messy" and "hard to
maintain". THIS code is messy. And impossible to maintain. If you need an event, you've
got the design wrong..
****
> progress->PostThreadMessage(WM_PROGRESS_ADD,0,(long)t);
****
NEVER use WM_ for a user-defined message; this is reserved for Microsoft. If you want to
talk about "maintainability", then you must not confuse future maintainers by making them
look through the MSDN for non-existent messages. Use a clearly-user-defined prefix. A
lot of us use UWM_ or WMU_, but you are free to use any prefix not already specified by
Microsoft. WM_ is reserved and should not be used for user-defined messages
****
> WaitForSingleObject(event,INFINITE);
****
The correct approach to cross-thread messaging is NOT to copy into a static variable,
which is invariably a mistake, but to simply allocate a heap object, e.g.,

CString * t = new CString(s);
progress->PostMessage(UWM_PROGRESS_ADD, 0, (LPARAM)t);

and let the receiver delete it when done.

Note that the (long) cast is erroneous; what in the world let you think that an LPARAM is
permitted to be cast as a (long). It must be cast as an (LPARAM). Note that in Win64
this code would lose the high-order 32 bits of the address, but the (LPARAM) cast would be
correct.

Read my essay on worker threads on my MVP Tips site. The approach of using a global and
an event is wrong, messy, and IMPOSSIBLE to maintain. It is just about the worst possible
implementation you could choose.

****
> CloseHandle(event);
>}
>
>But it locks up. If I comment out the line marked // *** the Wait works
>properly, so the problem is something to do with manipulating the dialog
>controls.
****
Locks up? I'm amazed it works at all! Perhaps you should do something that is less
messy, and in fact correct. Go read my essay. And learn about data types; WPARAM is
WPARAM, *not* UINT; LPARAM is LPARAM, *not* long (just because you once read one header
file for one platform and discovered these are the implementations on that one platform
does not make it correct to violate the specification by using the wrong types).
****
>
>Any suggestions?
>
>Alan
>
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.