"Mike Hearn" m.hearn@signal.qinetiq.com wrote:
Fail SetMenuItemInfo if both MFT_BITMAP and MFT_SEPARATOR are specified (an api violation).
I thought that Alexandre will not commit this patch, since it's obviously wrong.
- if (lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) {
- WARN("fType contains MFT_BITMAP and MFT_SEPARATOR, API violation\n");
- return FALSE;
- }
You are testing here whether either MFT_BITMAP or MFT_SEPARATOR is set, which is perfectly valid to specify one of them. If you want to test whether both flags are set simultaneously then use something like this:
if ((lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) == (MFT_BITMAP | MFT_SEPARATOR))
A conformance test to show the real behaviour is urgently required.
You are testing here whether either MFT_BITMAP or MFT_SEPARATOR is set, which is perfectly valid to specify one of them. If you want to test whether both flags are set simultaneously then use something like this:
if ((lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) == (MFT_BITMAP | MFT_SEPARATOR))
Ah, thanks :) I'll get there eventually..... yes, I see why now.
A conformance test to show the real behaviour is urgently required.
If you mean one that tries every combination of flags to see what Windows returns then that's beyond my capabilities and current toolset (no real windows development tools). I ran QuickTime 6 in Logview, and it shows up as "bad parameter" and the calls return false, and as the problem seemed to be having BITMAP and SEPARATOR set at the same time I thought for now it'd be good enough to just check for that, as this makes IE6 and QuickTime work properly.
Should I resubmit the patch with my mistake corrected? thanks -mike
"Mike Hearn" m.hearn@signal.qinetiq.com wrote:
A conformance test to show the real behaviour is urgently required.
If you mean one that tries every combination of flags to see what Windows returns then that's beyond my capabilities and current toolset (no real windows development tools).
That's too bad.
I ran QuickTime 6 in Logview, and it shows up as "bad parameter" and the calls return false, and as the problem seemed to be having BITMAP and SEPARATOR set at the same time I thought for now it'd be good enough to just check for that, as this makes IE6 and QuickTime work properly.
Should I resubmit the patch with my mistake corrected?
I would suggest to revert your patch until we will find what exactly causes IE6 and QuickTime to fail.
I would suggest to revert your patch until we will find what exactly causes IE6 and QuickTime to fail.
Well, in the case of IE6 it's a crash caused by it attempting to destroy the wrong menu (it does stuff with cloning menus passed in by the html renderer it seems). That's a secondary reaction to this function failing, seemingly because the first check returns false when actually it shouldn't. Unfortunately Marcus (who put the check in) can't remember why he chose that test, it was back in 2001, so I decided to leave the warning in as it might prove useful in future. Having the function succeed here doesn't stop IE from working properly, everything seems to be fine.
So the second bit of code (that I got wrong) was the check to stop QuickTime corrupting its menus by passing in bad data (otherwise the menus are set to garbage characters). This check only returns false for QuickTime, not IE.
if (((lpmii->fMask & MIIM_FTYPE) == MIIM_FTYPE) && ((lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) == (MFT_BITMAP | MFT_SEPARATOR))) { WARN("fType contains MFT_BITMAP and MFT_SEPARATOR, API violation\n"); return FALSE; }
Does that look any better? It checks that if the type is being set, those two flags aren't both present, which appears to fix IE and QuickTime, and is a check that should probably be in there anyway.
Well, you know a lot more about this than me, so if you think the check shouldn't be added then that's fine.
"Mike Hearn" m.hearn@signal.qinetiq.com wrote:
Well, you know a lot more about this than me, so if you think the check shouldn't be added then that's fine.
I'm not opposed to your fix, not at all. I just would like to understand better what is the real problem with our code. If you, or somebody else, could confirm (by a test case) that exactly (MFT_BITMAP | MFT_SEPARATOR) combination should be rejected then I'll vote for your patch.
I'm not opposed to your fix, not at all. I just would like to understand better what is the real problem with our code. If you, or somebody else, could confirm (by a test case) that exactly (MFT_BITMAP | MFT_SEPARATOR) combination should be rejected then I'll vote for your patch.
Reading about the MENUITEMINFO structure. Take from MSDN The MFT_BITMAP, MFT_SEPARATOR, and MFT_STRING values cannot be combined with one another.
Hope this helps Alistair
"Alistair Leslie" aleslie@neumaflo.com.au wrote:
Reading about the MENUITEMINFO structure. Take from MSDN The MFT_BITMAP, MFT_SEPARATOR, and MFT_STRING values cannot be combined with one another.
In that case the code which verifies MENUITEMINFO fields could be generalized. All existing checks in SetMenuItemInfoA should be removed and something like this should be added to SetMenuItemInfo_common:
if (lpmii->fMask & MIIM_TYPE ) { if (lpmii->fType & MFT_BITMAP) { if (lpmii->fType & (MFT_SEPARATOR | MFT_STRING)) return FALSE; } else if (lpmii->fType & MFT_SEPARATOR) { if (lpmii->fType & (MFT_BITMAP | MFT_STRING)) return FALSE; } else if (lpmii->fType & MFT_STRING) { if (lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) return FALSE; } }
Mike, could you please try this approach and report whether it helps?
Unfortunately it seems I was wrong about it stopping the QuickTime menu corruption... it merely reduces it and changes where it appears. The changes you gave below don't seem to change that, as far as I can see, so somehow we're missing a check that Windows does somewhere.
In particular the rather confusing nature of the calls Quicktime makes doesn't help:
SetMenuItemInfoA( 0x0006025F 0 TRUE [0x0013FD04] -> 44 , MIIM_CHECKMARKS | MIIM_BITMAP | MIIM_FTYPE , MFT_BITMAP | MFT_MENUBARBREAK | MFT_OWNERDRAW | MFT_SEPARATOR , 0x00000020 , 1310100 , 0x3C0052C4 , 0x00000311 , 0x00000202 , 0x00000000 , "x H" , 4 , 0x0013FD94 ) -> FALSE
fMask -> MIIM_CHECKMARKS | MIIM_BITMAP | MIIM_FTYPE fType -> MFT_BITMAP | MFT_MENUBARBREAK | MFT_OWNERDRAW | MFT_SEPARATOR
Which other than being API violations don't make sense anyway, why would any menu in quicktime need to have a vertical break?
I think you're right in that a full test case is needed, but I can't do that (at least, not in C, if Object Pascal is acceptable I might be able to do so).
On Thu, 2003-01-16 at 01:30, Dmitry Timoshkov wrote:
"Alistair Leslie" aleslie@neumaflo.com.au wrote:
Reading about the MENUITEMINFO structure. Take from MSDN The MFT_BITMAP, MFT_SEPARATOR, and MFT_STRING values cannot be combined with one another.
In that case the code which verifies MENUITEMINFO fields could be generalized. All existing checks in SetMenuItemInfoA should be removed and something like this should be added to SetMenuItemInfo_common:
if (lpmii->fMask & MIIM_TYPE ) { if (lpmii->fType & MFT_BITMAP) { if (lpmii->fType & (MFT_SEPARATOR | MFT_STRING)) return FALSE; } else if (lpmii->fType & MFT_SEPARATOR) { if (lpmii->fType & (MFT_BITMAP | MFT_STRING)) return FALSE; } else if (lpmii->fType & MFT_STRING) { if (lpmii->fType & (MFT_BITMAP | MFT_SEPARATOR)) return FALSE; } }
Mike, could you please try this approach and report whether it helps?
On Thursday 16 January 2003 11:44 am, Mike Hearn wrote:
I think you're right in that a full test case is needed, but I can't do that (at least, not in C, if Object Pascal is acceptable I might be able to do so).
Why can't it be done in C, out of curiosity? Please forgive me if paying attention to the rest of this thread would have answered this question :(
Also: I spent the past 5 years programming Object Pascal. It would be great (for me, at least) if we could make tests in Object Pascal, but it's hard for me to imagine how that could ever be a wine-compatible approach? Is there even an OSS compiler for Object Pascal? Even if there is such a thing, I don't think it would be of much utility without the wine headers being also ported to Object Pascal, a huge task in itself.
Just wondering, no flame intended :)
Well, a conformance test would be most useful for being run on Windows, we already know what Wine does. So the fact that Delphi is faster to write stuff in than raw C is what motivated me to ask. Yes, there is a free object pascal compiler, i can't remember what it's called though
On Thu, 2003-01-16 at 18:10, Greg Turner wrote:
On Thursday 16 January 2003 11:44 am, Mike Hearn wrote:
I think you're right in that a full test case is needed, but I can't do that (at least, not in C, if Object Pascal is acceptable I might be able to do so).
Why can't it be done in C, out of curiosity? Please forgive me if paying attention to the rest of this thread would have answered this question :(
Also: I spent the past 5 years programming Object Pascal. It would be great (for me, at least) if we could make tests in Object Pascal, but it's hard for me to imagine how that could ever be a wine-compatible approach? Is there even an OSS compiler for Object Pascal? Even if there is such a thing, I don't think it would be of much utility without the wine headers being also ported to Object Pascal, a huge task in itself.
Just wondering, no flame intended :)
"Dmitry Timoshkov" dmitry@baikal.ru writes:
I'm not opposed to your fix, not at all. I just would like to understand better what is the real problem with our code. If you, or somebody else, could confirm (by a test case) that exactly (MFT_BITMAP | MFT_SEPARATOR) combination should be rejected then I'll vote for your patch.
At least on NT4 it's not rejected. In fact NT seems to accept any combination of flags. I have removed the check for now.
-
Alexandre Julliard -
Alistair Leslie -
Dmitry Timoshkov -
Greg Turner -
Mike Hearn