Re: [bug 4004] MenuItemInfo vs GetMenuString
"Ann & Jason Edmeades" <us(a)edmeades.me.uk> wrote:
Copy and store the dwTypeData of a menu item even for a ownerdraw item, and ensure the memory allocations/freeing occurs correctly
+ /* Just change ftype to string and see what text is stored */ + memset(&info, 0x00, sizeof(info)); + info.cbSize= sizeof(MENUITEMINFO); + info.fMask= MIIM_FTYPE; /* Set string type */ + info.fType= MFT_STRING; + info.dwTypeData= (char *)0xdeadbeef; + SetMenuItemInfo( hmenu, 0, TRUE, &info ); + + /* Did we keep the old dwTypeData? */ + ok (GetMenuString( hmenu, 0, strback, 99, MF_BYPOSITION), "GetMenuString on ownerdraw entry failed\n"); + ok (!strcmp( strback, "Dummy string" ), "Menu text from Ansi version incorrect\n"); + + /* Just change ftype to bitmap and back to ensure data isnt freed */ + memset(&info, 0x00, sizeof(info)); + info.cbSize= sizeof(MENUITEMINFO); + info.fMask= MIIM_FTYPE; /* Set as bitmap type */ + info.fType= MFT_BITMAP; + info.dwTypeData= (char *)0xdeadbee2; + SetMenuItemInfo( hmenu, 0, TRUE, &info ); + info.fType= MFT_OWNERDRAW; /* Set as ownerdraw type */ + info.dwTypeData= (char *)0xdeadbee3; + SetMenuItemInfo( hmenu, 0, TRUE, &info );
You don't check SetMenuItemInfo return code after the above calls and I very much suspect that they fail leading to the observed results. -- Dmitry.
You don't check SetMenuItemInfo return code after the above calls and I very much suspect that they fail leading to the observed results.
Well spotted, thanks! In the one case of string -> ownerdraw you are right, and I have now modified the tests to flag a todo where this unexpectedly works on wine but fails on windows. In the case of ownerdraw -> string -> ownerdraw, the calls do indeed work and keep the saved string. Debugging on windows seems to show the string is copied and saved as well, which concurs with the rest of the patch. The fix (rather than the tests) is still accurate for this as far as I can tell. Attached is new patch, which checks the rc of the SetMenuItemInfo (and InsertMenuItem) calls... Changelog Copy and store the dwTypeData of a menu item even for a ownerdraw item, and ensure the memory allocations/freeing occurs correctly Jason
"Ann & Jason Edmeades" <us(a)edmeades.me.uk> wrote:
In the case of ownerdraw -> string -> ownerdraw, the calls do indeed work and keep the saved string. Debugging on windows seems to show the string is copied and saved as well, which concurs with the rest of the patch.
The fix (rather than the tests) is still accurate for this as far as I can tell.
Attached is new patch, which checks the rc of the SetMenuItemInfo (and InsertMenuItem) calls...
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. -- Dmitry.
On Mon, 26 Dec 2005 16:13:10 +0800, you wrote:
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.
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. I am too busy celebrating vacation atm, but I hope to send in a patch with extensive tests somewhere tomorrow or so. Rein.
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.
"Ann & Jason Edmeades" <us(a)edmeades.me.uk> wrote:
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.
The proposed fix doesn't look right. I'd suggest just submit the test case showing the bug with appropriate todo_wine statements to wine-patches, then we could try to make it pass under Wine. -- Dmitry.
On Mon, 26 Dec 2005 10:57:04 -0000, you wrote:
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.
This what I think that is happening, also including a MF_BITMAP flag to the fFlags field if for backward compatibility it is needed.
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.
I attach it here now (bzip2'ed for once because of its size). Before submitting to wine-patches, I need some time to review the code once more for remnants of old way of thinking and to get rid of the IS_xxxx_ITEM macro's ( or rename them to reflect the true intention).
Can you make sure you incorporate the specific tests for the bug report in it too please.
You tests from 4004.patch2 are included. I just removed the todo_wine{} which is a probably a good sign. Rein.
I attach it here now (bzip2'ed for once because of its size). Before submitting to wine-patches, I need some time to review the code once more for remnants of old way of thinking and to get rid of the IS_xxxx_ITEM macro's ( or rename them to reflect the true intention).
Glancing through the patch, I see what you are trying to do, and some comments (and remember this is just from a glance so far) 1. I'm not sure the MENU_SetItemData for the ownerdraw case is correct as it doesn't actually take a copy of the data, it saves the pointer. I'm pretty sure Windows would take a copy (It does when inserting). In fact the ownerdraw code doesn't set text, just dwitemdata in this routine 2. SetMenuItemInfo_common, In the MIIM_TYPE case for ownerdraw - You would lose the dwtypedata value for ownerdraw with text 3. Unrelated but just spotted, GetMenuItemInfo_common doesn't return dwTypeData for ownerdraw what TYPE is used, can we add a test to confirm that?
You tests from 4004.patch2 are included. I just removed the todo_wine{} which is a probably a good sign.
Yeah, the todo was a condition I didn't really care about at the time, but fixing it is good :-) Thanks Jason
I just got back from being on vacation for a week or so, and found I can't compile the current CVS on either of my systems - even after checking out everything from scratch and making clean. Is anyone else having this problem, or is is specific to my machines? ../../../tools/winegcc/winegcc -B../../../tools/winebuild -mconsole atom.o env.o error.o exception.o generated.o info.o large_int.o path.o om.o reg.o rtl.o rtlbitmap.o rtlstr.o string.o time.o testlist.o -o ntdll_test.exe.so -L../../../libs/port -lwine_port -L../../../dlls -L../../../dlls/kernel32 -L../../../libs -lkernel32 exception.o(.text+0x267): In function `test_prot_fault': /home/evil/install/wine/dlls/ntdll/tests/exception.c:202: undefined reference to `NtCurrentTeb' exception.o(.text+0x271):/home/evil/install/wine/dlls/ntdll/tests/exception.c:203: undefined reference to `NtCurrentTeb' exception.o(.text+0x32b):/home/evil/install/wine/dlls/ntdll/tests/exception.c:213: undefined reference to `NtCurrentTeb' collect2: ld returned 1 exit status winegcc: gcc failed. make[3]: *** [ntdll_test.exe.so] Error 2 Thanks in advance, J
I commented out the three references to NtCurrentTeb in dlls/ntdll/tests/exception.c and WINE now compiles, installs, and runs fine. There's no CVS history for that file beyond the (current) 1.1 version, so I'm guessing an include or something has been missing ever since it was first added on the 17th of this month. I'm still confused, though, as to why I'm the only one (?) seeing the problem (on both my Mandriva 2005LE and Mandriva 2006 machines). I don't immediately see how it (as part of thread.c) would be found when compiling tests/exception.c, but I've only scratched the surface of the source; Any guru's care to illuminate me? -Jesse Evil wrote:
I just got back from being on vacation for a week or so, and found I can't compile the current CVS on either of my systems - even after checking out everything from scratch and making clean.
Is anyone else having this problem, or is is specific to my machines?
../../../tools/winegcc/winegcc -B../../../tools/winebuild -mconsole atom.o env.o error.o exception.o generated.o info.o large_int.o path.o om.o reg.o rtl.o rtlbitmap.o rtlstr.o string.o time.o testlist.o -o ntdll_test.exe.so -L../../../libs/port -lwine_port -L../../../dlls -L../../../dlls/kernel32 -L../../../libs -lkernel32 exception.o(.text+0x267): In function `test_prot_fault': /home/evil/install/wine/dlls/ntdll/tests/exception.c:202: undefined reference to `NtCurrentTeb' exception.o(.text+0x271):/home/evil/install/wine/dlls/ntdll/tests/exception.c:203: undefined reference to `NtCurrentTeb' exception.o(.text+0x32b):/home/evil/install/wine/dlls/ntdll/tests/exception.c:213: undefined reference to `NtCurrentTeb' collect2: ld returned 1 exit status winegcc: gcc failed. make[3]: *** [ntdll_test.exe.so] Error 2
Thanks in advance, J
Evil wrote:
I commented out the three references to NtCurrentTeb in dlls/ntdll/tests/exception.c and WINE now compiles, installs, and runs fine.
There's no CVS history for that file beyond the (current) 1.1 version, so I'm guessing an include or something has been missing ever since it was first added on the 17th of this month.
I'm still confused, though, as to why I'm the only one (?) seeing the problem (on both my Mandriva 2005LE and Mandriva 2006 machines). I don't immediately see how it (as part of thread.c) would be found when compiling tests/exception.c, but I've only scratched the surface of the source; Any guru's care to illuminate me?
-Jesse
Evil wrote:
I just got back from being on vacation for a week or so, and found I can't compile the current CVS on either of my systems - even after checking out everything from scratch and making clean.
Is anyone else having this problem, or is is specific to my machines?
../../../tools/winegcc/winegcc -B../../../tools/winebuild -mconsole atom.o env.o error.o exception.o generated.o info.o large_int.o path.o om.o reg.o rtl.o rtlbitmap.o rtlstr.o string.o time.o testlist.o -o ntdll_test.exe.so -L../../../libs/port -lwine_port -L../../../dlls -L../../../dlls/kernel32 -L../../../libs -lkernel32 exception.o(.text+0x267): In function `test_prot_fault': /home/evil/install/wine/dlls/ntdll/tests/exception.c:202: undefined reference to `NtCurrentTeb' exception.o(.text+0x271):/home/evil/install/wine/dlls/ntdll/tests/exception.c:203: undefined reference to `NtCurrentTeb' exception.o(.text+0x32b):/home/evil/install/wine/dlls/ntdll/tests/exception.c:213: undefined reference to `NtCurrentTeb' collect2: ld returned 1 exit status winegcc: gcc failed. make[3]: *** [ntdll_test.exe.so] Error 2
The problem is caused because NtCurrentTeb is defined as "extern inline". Your compiler appears to have decided not to inline the function so it expects to be able to link to the function. However, the tests aren't linked to ntdll as they need to run on win9x machines. The solution that is probably the best is to include an implementation of NtCurrentTeb at the end of the test to be used if there is no inline version. -- Rob Shearman
On Mon, 26 Dec 2005 21:50:54 -0000, you wrote:
Glancing through the patch, I see what you are trying to do, and some comments (and remember this is just from a glance so far)
1. I'm not sure the MENU_SetItemData for the ownerdraw case is correct as it doesn't actually take a copy of the data, it saves the pointer. I'm pretty sure Windows would take a copy (It does when inserting). In fact the ownerdraw code doesn't set text, just dwitemdata in this routine
I do not understand where the problem is. As you say, In the ownerdraw case only the dwItemData field is filled, which is not supposed to be a pointer to something the code should know or care about.
2. SetMenuItemInfo_common, In the MIIM_TYPE case for ownerdraw - You would lose the dwtypedata value for ownerdraw with text
Yup that bit was suspect but my testing framework was not able to test that. Now it is and indeed the text is preserved, unless new text overwrites it.
3. Unrelated but just spotted, GetMenuItemInfo_common doesn't return dwTypeData for ownerdraw what TYPE is used, can we add a test to confirm that?
Tests added, I could not find anything wrong with the behavior. I have sent the updated patch to wine-patches this time. Rein.
"Rein Klazes" <wijn(a)wanadoo.nl> wrote:
I attach it here now (bzip2'ed for once because of its size). Before submitting to wine-patches, I need some time to review the code once more for remnants of old way of thinking and to get rid of the IS_xxxx_ITEM macro's ( or rename them to reflect the true intention).
Well done Rein, your patch looks straightforward and really simple. Thanks. -- Dmitry.
Hiya, I've found my changes for this other bug, which also relates to the menu code are wrong, and am trying to work out how to identify how windows does it. The problem is that there is a menu item with the same id as a popup menu. In all the tests for all the calling functions, windows will return the menu item rather than the popup when the bycommand options are used. I assumed this meant that popups weren't returned, but that is wrong. If there is no conflict then the functions will return the popup itself even if called by id, so I am trying to work out exactly what search algorithm windows uses via tests. Currently my added tests are: 1. Menu with menuitem id A, get info via id A obviously gives the menuitem 2. Menu containing empty submenu with hmenu (and hence id) of B, getinfo via id B gives the menuitem (ie popups can be returned) 3. Menu containing submenu with hmenu (and hence id) of B which in turn contains 2 menu items, both with id of B. Getinfo on id B gives the first of the two items. I wondered if this meant it searched the submenu for items before possibly returning the submenu itself, but... 4. Menu containing submenus with ids of A and B. The second submenu (with id B) contains 2 items with ids of A, Getinfo on id A gives the first of the two items which come from the second submenu. Ie windows has searched the first menu and found no items (even though the first submenu itself is a match), then searched the second submenu and found an item. Are there any other testcases you think I could add which might shed light onto how windows does it? Given my test results, my current thoughts are to change the algorithm to search for an item but remembering the first matching submenu. If we get to the end of the search with no menuitem hits, but did find a submenu on the way, return that. Regards, Jason
"Ann & Jason Edmeades" <us(a)edmeades.me.uk> wrote:
Currently my added tests are: 1. Menu with menuitem id A, get info via id A obviously gives the menuitem 2. Menu containing empty submenu with hmenu (and hence id) of B, getinfo via id B gives the menuitem (ie popups can be returned) 3. Menu containing submenu with hmenu (and hence id) of B which in turn contains 2 menu items, both with id of B. Getinfo on id B gives the first of the two items.
Again, the very first thing I'd suggest to do is to write a comprehensive test case and send it to wine-patches with appropriate todo_wine statements to make it pass. Then we could see exact steps which lead to a failure and could start thinking of a possible fix. -- Dmitry.
Again, the very first thing I'd suggest to do is to write a comprehensive test case and send it to wine-patches with appropriate todo_wine statements to make it pass. Then we could see exact steps which lead to a failure and could start thinking of a possible fix.
Attached is a set of tests which try to exercise locating via the id parameter when there is a collision between a popup (id == hMenu) and a menu item. As far as I cal tell if a non-popup exists anywhere in the menu then it is returned, and only if none are found then the popup itself is returned. Can anyone think of any tests I can use to prove or disprove this beyond what is in the attached patch? My current though on solving this would be to remember any matching popup match but not return it, only using the remembered value if we exit the search having found no matching ids. Anyone object before I submit? Changelog Tests for bug#3095 where the menu is incorrect because of duplication of a Menu id and an hMenu Jason
participants (5)
-
Ann & Jason Edmeades -
Dmitry Timoshkov -
Evil -
Rein Klazes -
Robert Shearman