I made the changes you suggested and the problem went away. I'm
On Mon, 03 Mar 2008 13:16:18 -0500, Joseph M. Newcomer
See below...
On Mon, 03 Mar 2008 17:18:58 GMT, mcmillan@viselect.com (Brad McMillan) wrote:
Hi:
I have a dialog-based C++ .NET application that has a class based on
CTreeCtrl. The "OnRButtonDown()" method of this class has code that
brings up a popup menu that allows the user to select any of 5 choices
(when they right click a tree item whose ItemData value is 29 in this
example) with the following code:
hTItem = HitTest( point, &nFlags );
DWORD iData = GetItemData( hTItem );
HMENU hMenu=::CreatePopupMenu();
****
Why not use CMenu?
CMenu menu;
if(!menu.CreatePopupMenu())
return; // or other error handler
Better still, create the menu as a resource! There is no reason to create the menu at all
here
CMenu menu;
menu.LoadMenu(IDR_TREEVIEW_POPUP);
CMenu * popup;
popup = menu.GetSubmenu(0);
Note that you will have problems doing localization if you use literal strings if those
literal strings are language-specific; you should not use literal strings but load the
strings from a STRINGTABLE. If you load the menu resource instead, you don't need to
worry about this at all.
***
if (NULL!=hMenu)
{
if ( iData==29 )
****
WHy 29 instead of some meaningful name defined by #define or const int?
****
{
::AppendMenu(hMenu, MF_STRING, 1, "SEL 1");
****
Literal, 8-bit character strings represent obsolete programming methodology. At the VERY
LEAST you should put _T() around every literal string; but see above comments on
localization. The assignment of numerical values like 1, 2, etc. is also poor practice,
see comments below about symbolic values
*****
::AppendMenu(hMenu, MF_STRING, 2, "SEL 2");
::AppendMenu(hMenu, MF_STRING, 3, "SEL 3");
::AppendMenu(hMenu, MF_STRING, 4, "SEL 4");
::AppendMenu(hMenu, MF_STRING, 5, "SEL 5");
::AppendMenu(hMenu, MF_STRING, 6, "Exit");
point.y = 16*(point.y/16);
****
Seems pointless (pun intended). First, why does it need to be 16-pixel aligned? Why do
you think pixels are going to matter anyway? Note that this is going to produce different
effects on any display OTHER than your current machine at the instant you are doing this
test (change any of a dozen characteristics of your machine at 16 loses all meaning). If
you want to truncate to 16, it would be more effective to write
point.y &= ~0xF;
****
ClientToScreen(&point);
int sel=0;
sel=::TrackPopupMenu( hMenu, TPM_CENTERALIGN|TPM_RETURNCMD,
****
Put spaces around operators, so that the code is readable:
TPM_CENTERALIGN | TPM_RETURNCMD
Also, it should read
TPM_CENTERALIGN | TPM_RETURNCMD | TPM_NONOTIFY
because you don't want a message sent to the specified window.
Why use the raw API in an MFC app?
****
point.x, point.y-57,
****
See above. 57 is a completely meaningless number except on your display with your current
settings; it will be nonsensical on nearly any other display. If you make adjustments
like this, you need to use a display-independent value which you compute on the fly.
*****
0, m_hWnd, NULL);
if ( sel<6 )
****
Why are you not using symbolic values? Or better still, an enum type:
enum {SEL1, SEL2, SEL3, SEL4, SEL5, SELEXIT};
and write
if(sel != SELEXIT)
Note also that you will post the message if the user simply clicks outside the menu, or
hits escape, or anything else, and you still post a message. So you might want the test
if(sel != 0 && sel != SELEXIT)
which is a whole lot more intelligible, and doesn't require making unrelated changes if
you add three more selections next week.
****
{
::PostMessage(m_hPWnd, P_MESSAGE, (WPARAM) 29, (LPARAM) sel );
****
What is m_hPWnd? Where is it set? A more common way of writing this would be
::PostMessage(::GetParent(m_hWnd), P_MESSAGE, (WPARAM)iData, (LPARAM)sel);
or, since this is supposedly MFC
GetParent()->PostMessage(P_MESSAGE, (WPARAM)iData, (LPARAM)sel);
I fail to see the fascination with using the raw API here in what is clearly an MFC app.
Note that sending a message to OTHER than the parent is very strange.
****
}
}
}
When any of the selections SEL 1 to SEL 5 are chosen, a message is
sent to the main program where the message handler causes a dialog box
to be generated that depends on the selection that was made with code
that is similar to the following:
if ( wParam==29 )
****
Why is 29 a special value? Why not use a symbolic constant?
****
{
sdDlg.selType = lParam;
sdDlg.DoModal();
}
The problem I'm having is that when "SEL 4" is selected the dialog box
that is brought up, sdDlg, intermittantly freezes the program (about
half the time) and I need to bring up the task manager and choose "End
Task" to stop it.
****
I suspect that the fact you have not used the TPM_NONOTIFY is going to be an issue here.
Fix that first.
Did you use the debugger to Break All and see where it is hung? That would actually be
useful information.
****
This doesn't happen on any of the other selections and I've tried to
bring up different dialog boxes on "SEL 4", but they freeze also.
****
Probably because your app is interpreting WM_COMMAND:SEL4 as something to do, and is
therefore doing something odd. Fix the TPM_NONOTIFY problem before doing anything else.
****
I've run out of ideas on how to troubleshoot this problem.
****
Have you considered using the debugger?
joe
****
Can anyone tell me what to do to figure out how to solve this?
Thanks,
Brad McMillan
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm