The fact that you need to intorduce the txtWasAllocated flag suggests that something is wrong. Also MF_OWNERDRAW certainly does not qualify as a
MF_STRING
alias. I'll try to play with your test case and see if I can find a better
fix.
The particular problem in the bug report was that the ownerdraw can indeed have an associated string value (which is converted to Unicode if necessary) which is kept across the menu item being converted to a String or Ownerdraw type, and our code wouldn't allow returning the string value nor did it save a copy of it in the ownerdraw case.
It also highlighted problems with the code where we wouldn't free up the allocated storage in one of the cases even for the string type, and once you added the ownerdraw type into the decision making about freeing then it was a lot simpler and cleaner to save whether the memory was allocated or not. I don't doubt you could do it without the flag, but it would also be more complex checks in each case.
You are correct. The real problem is that the text field should not be used anymore for bitmaps handles after the hbmpItem field was added just for that. This is proven by the fact that strings cannot only be combined with owner draw menu, but also with bitmaps and even separators.
Hmmm... As was found from the tests once I checked the rc, you cant convert a string type to a bitmap type directly, although I didn't test adding a valid bitmap handle. My tests haven't proved (or attempted to prove) whether you can have a string attached to a bitmap type, but reading the MSDN it states that a menuitem which is a bitmap has the low-order word of the dwTypeData member as the bitmap handle, and hence implies you couldn't have an associated text (as that is normally pointed to via the same field). However, since Windows also copies the value, they too may have the equivalent of the internal 'text' field which may or may not get cleared, so perhaps this is wrong. It was outside the scope of what I was trying to fix.
I am too busy celebrating vacation atm, but I hope to send in a patch with extensive tests somewhere tomorrow or so.
Sure, thanks - I'll watch out for it. Can you make sure you incorporate the specific tests for the bug report in it too please.
Jason
PS While I think about it, I've also fixed another menu issue as well (bug 3095), and have attached a sample patch to the bug report which includes this and the changes to fix and test that one, and if you have an interest in the menu code you might want to check that out as well. I was going to submit it once this one got into cvs.