What’s wrong with this code, part 25

Wow, 25 already.

This one’s pretty straightforward.  Once again, it’s a UI issue, since I’ve been spending most of my time doing UI lately.

In this particular case, the code comes from the constructor for an auto-layout class that is used internally in one of our tools.  It saves away window handles for a control and the dialog which holds the control, then saves the size of the dialog and relative location of the control within that dialog.  There’s other code that handles resizing and adjusting the layout of the control when the dialog is resized. 

 CControlLayout::CControlLayout(const HWND hWndControl, const HWND hWndDlg)
    : m_hWnd(hWndControl)
    , m_hWndDlg(hWndDlg)
{
    // Get the parent (dialog) rect, and the control rect
    ::GetClientRect(m_hWndDlg, &m_rcRefDlg);
    ::GetWindowRect(m_hWnd, &m_rcRef);
    ScreenToClientRect(hWndDlg, m_rcRef);
}

 void ScreenToClientRect(/* [in] */ const HWND hWndClient, 
                        /* [in/out] */ RECT &rcInOut)
{
 CPoint ptTopLeft(rcInOut.left, rcInOut.top);
 CPoint ptBottomRight(rcInOut.right, rcInOut.bottom);

  ::ScreenToClient(hWndClient, &ptTopLeft);
 ::ScreenToClient(hWndClient, &ptBottomRight);

  rcInOut.left = ptTopLeft.x;
 rcInOut.top = ptTopLeft.y;
 rcInOut.right = ptBottomRight.x;
 rcInOut.bottom = ptBottomRight.y;
}

m_rcRefDlg holds the reference rect for the dialog and m_rcRef holds the reference rect for the control relative to the dialog.

This code has been in the UI for quite a while and recently one of our testers discovered a nasty bug while running a test pass.

The question is: What’s wrong with this code.  I believe we shipped Windows Vista with this bug (I’m not 100% sure, since I don’t have a Windows Vista machine to test it), so it’s pretty subtle.

 

Edit: OOPS - I forgot to include ScreenToClientRect.

Comments

• Anonymous
  November 17, 2008
  The only thing that jumps out at me as odd here is that the ScreenToClientRect function uses hWndDlg (a const parameter) as its parameter instead of m_hWndDlg (a non-const member variable). Not knowing how the ScreenToClientRect function works, this may or may not be pertinent, but under the assumption that it's just a RECT version of ScreenToClient, I'm not coming up with any scenario in which this should matter. I'll be very curious to know what people wiser than me can come up with.

• Anonymous
  November 17, 2008
  The only thing I can think of is that if it is possible for the parent window to have a right-to-left layout (WS_EX_LAYOUTRTL), then the Windows API documentation says you should use MapWindowPoints.  I don't have any personal experience with this, though.

• Anonymous
  November 17, 2008
  I guess this fails if the control has a different LTR/RTL layout than its window. Well, guess: <http://msdn.microsoft.com/en-us/library/ms632599(VS.85).aspx#layout_mirroring> is fairly clear about this: "// DO NOT USE THIS IF APPLICATION MIRRORS THE WINDOW // get coordinates of the window in screen coordinates GetWindowRect(hControl, (LPRECT) &rControlRect);   // map screen coordinates to client coordinates in dialog ScreenToClient(hDialog, (LPPOINT) &rControlRect.left); ScreenToClient(hDialog, (LPPOINT) &rControlRect.right);"

• Anonymous
  November 17, 2008
  Don't know what ScreenToClientRect does for certain from the code that you have posted, but if it (as the name implies) just calls ScreenToClient twice instead of using MapWindowPoints, that will break in RTL layout mode.  I have to assume that <starwars>this isn't the bug we're looking for</starwars> given that the crucial code for such a bug is omitted, though.

• Anonymous
  November 17, 2008
  DINGDING*. I guess it was too easy.  That's exactly the problem - ScreenToClient returns the flipped screen points, so ScreenToClientRect returns the bottom and right coordinates of the window incorrectly. The right code is (as Reinder mentioned) to call MapWindowRect using NULL as the "From" window.

• Anonymous
  November 17, 2008
  May I ask why do you pass 'const HWND's in the constructor instead of just 'HWND's ? These parameters are passed by value, so I don't understand the use of 'const'... I would just write code like this: CControlLayout(HWND hWndControl, HWND hWndDlg) What am I missing here? Thanks

• Anonymous
  November 17, 2008
  "const" in this case just helps to safeguard the value of the handle inside the method. Compiler should be wise enough to pass the handle as a value and give you compile time error in case you occasionally try to modify it. So there should be no performance penalty (at least similar behavior was in Borland compilers).

• Anonymous
  November 18, 2008
  Question... Is ScreenToClientRect yours and, if so, why bother with the use of CPoint?  Not a biggy I know, but have you ever stepped through the assembly code generated for this versus using a simple struct?   I ask because I was tasked with speeding up the painting of some custom controls a few years ago.  I did everything I could to speed up the calculations and drawing code but got only a slight improvement because there was minimal work involved in the routines.  After stepping through the (fully optimized) code generated by Visual Studio and seeing how much overhead was caused by using the CRects and CPoints instead of simple structs I swapped them out.  It gave me a ten fold increase in painting speed, cut the object code size in half, and I lost minimal readability in the source. Yes, I profiled it extensively and yes, it was very noticable to the end users as they (unfortunately) tended to place the controls on dialogs in considerable volume.  Ever since I've been careful to only use such "helper" classes where there is a real benefit to using them and their member functions.

• Anonymous
  November 18, 2008
  The comment has been removed

• Anonymous
  November 18, 2008
  The comment has been removed

• Anonymous
  January 21, 2009
  Sorry Larry, I'm not understanding this.  In what circumstances is this wrong? I've set my frame window to WS_EX_LAYOUTRTL but I find that ScreenToClient and MapWindowPoints give exactly the same results (commenting out the relevant lines) m_OrigRects = new CRect[count]; m_count = count; for (int x = 0; x < count; x++) {  m_pWnd->GetDlgItem(ids[x])->GetWindowRect(m_OrigRects + x);  m_pWnd->ScreenToClient(m_OrigRects + x); // ::MapWindowPoints(NULL, m_pWnd->m_hWnd, (LPPOINT) (m_OrigRects+x),2); } I must be missing something.

• Anonymous
  January 21, 2009
  Ah, I see my error now. The MFC ScreenToClient does some extra work: void CWnd::ScreenToClient(LPRECT lpRect) const { ASSERT(::IsWindow(m_hWnd)); ::ScreenToClient(m_hWnd, (LPPOINT)lpRect); ::ScreenToClient(m_hWnd, ((LPPOINT)lpRect)+1); if (GetExStyle() & WS_EX_LAYOUTRTL) CRect::SwapLeftRight(lpRect); }