Compartilhar via

What’s wrong with this code, part 24 – the answer

  • Artigo

In my last post, I included a snippet from an MSDN article written by Kenny Kerr. 

The snippet was pretty straightforward, but had a subtle bug in it:

 CRect rectangle;
VERIFY(m_splitButton.GetWindowRect(
    &rectangle));

TPMPARAMS params = { sizeof(TPMPARAMS) };
params.rcExclude = rectangle;

CMenuHandle menu = m_menu.GetSubMenu(0);

VERIFY(menu.TrackPopupMenuEx(TPM_LEFTBUTTON,
    rectangle.left, rectangle.bottom,
    m_hWnd, &params));

 

The problem was that on Bidi localized systems the popup menu is located in the wrong location.  The TrackPopupMenuEx API takes screen coordinates for the popup menu and on a LTR system creates the popup window with the top left corner of the window at that screen coordinate.  The problem here is that on an RTL system the top right of the menu is located at the screen coordinates.  The good news is that the fix is relatively simple:

 CRect rectangle;
VERIFY(m_splitButton.GetWindowRect(
    &rectangle));

TPMPARAMS params = { sizeof(TPMPARAMS) };
params.rcExclude = rectangle;

CMenuHandle menu = m_menu.GetSubMenu(0);

VERIFY(menu.TrackPopupMenuEx(TPM_LEFTBUTTON,
    ((GetWindowLong(m_hWnd, GWL_EXSTYLE) & WS_EX_LAYOUTRTL) != 0 ? rectangle.right :  rectangle.left), rectangle.bottom,
    m_hWnd, &params));

Before I posted this blog post, I asked the author (who also commented on the previous post) about it.  His response was:

As I tried to trim the sample to the bare minimum I didn’t bother with this. You can use GetSystemMetrics to get the proper alignment, but I’m not sure this constitutes a “nasty bug” so you may be after something else.

IMHO Kenny’s right – code samples are intended to be the bare minimums, there are tons of differences between what you do in a code sample and what you do in production code – support for RTL languages is one of those taxes that get left behind when writing samples.

The reason I wrote the post wasn’t to pick on Kenny or MSDN magazine.  Instead it was to point out the fact that popup menus tied to UI elements don’t work the way you expect on mirrored builds, leading to subtle display issues.

 

 

Kudos and mea culpas:

First off, my phrasing was incorrect – the bug wasn’t “nasty” it was subtle and not at all obvious.

Kenny’s comment was the first to mention the RTL issue, but to be fair, he and I had exchanged emails last week about this issue so I don’t feel good in giving him the kudos.  Instead I’m going to give it to Ryan who correctly pointed out that the snippet assumes LTR order.

 

There are some other interesting potential issues like Maciej Rutkowski pointing out that the UI might misbehave at the bottom of the screen (a good point).

Comments

  • Anonymous
    October 07, 2008
    The comment has been removed

  • Anonymous
    October 07, 2008
    Out of curiosity, would the Tablet PC "handedness" (left-handed/right-handed) option affect this too (this option affects to which side the menus open)?

  • Anonymous
    October 08, 2008
    I don't agree with the statement that code samples shouldn't contain "real-world" code like this. How else is a new Win32 programmer supposed to know this? For experienced programmers, it might be obvious, but it's very likely that not everyone will immediately realize the RLT implications (or even know that RTL systems exist). I'm quite sure that lots of code samples end up being copy/pasted into production code. Yes, perhaps the samples would be a little longer and more difficult to read, but at least you could rely on them to show the "correct" way to use an API in your real-world application.

  • Anonymous
    October 08, 2008
    I work for a UI control vendor and I can't state enough how important it is that examples DO correctly cater for RTL scenarios. All production software should handle RTL layou, if it doesn't, it's undeployable (at worst) or an eyesore (at best) in those locates where RTL is the norm.

  • Anonymous
    October 09, 2008
    If I might opine (grumpily) for a moment: there are two giant bugs presented here. Firstly, there's no such thing as a 'CMenuHandle'.  I mean, there obviously is, or the code won't even compile.  But MSDN certainly knows nothing about it.  You can't google for it.  You can't tell how to handle any resource issues.  You can't tell what member function it has.  Presumably it's some kind of wrapper about a CMenu -- but how is anyone supposed to know that? Secondly, 'TrackPopupMenuEx' takes in the wrong set of parameters.  The number one use case is, "someone clicked on a menu-thing and wants a good pop-up" -- which means we know a rectangle (the original menu) such that we want the sub-menu to be positioned "correctly".  What "correctly" means is defined by MSFT and should be handled by standard MSFT code.

  • Anonymous
    October 09, 2008
    why do you use params.rcExclude ?  I'm not familiar with its purpose