Opened 11 years ago

Closed 3 years ago

#11657 closed defect (fixed)

Wrong margins for owner drawn items bitmaps under XP

Reported by: rk Owned by:
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: regression Cc: malcompl
Blocked By: Blocking:
Patch: no

Description

There is an assertion failing in trunk (after the merge of the owner-drawn menus branch). This can be seen in the menu sample by opening the "File" menu. It seems that the icon to be drawn is to big for the available space. In my own application I use 16x16 icons and looking at the variables in the debugger when the assertion fails shows that there are only 13x13 pixels left for the icon. Seeing that this whole thing worked with 16x16 icons before the merge, either the assert or the computation of the available space seems to be wrong.

I am on WinXP, if that make a difference.

Attachments (1)

menuicons.PNG download (6.3 KB) - added by TNikolay 11 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by vadz

  • Cc malcompl added
  • Milestone set to 2.9.1
  • Status changed from new to confirmed

I can confirm that the assert happens under XP. It does not happen under Win7.

Marcin, any feedback about this would be much appreciated, TIA!

comment:2 Changed 11 years ago by rk

After some very long debugging session I found out some things that might explain why this suddenly happens.

The actual assert fails, because it checks that the item bitmap to be drawn is at most as big as a checkmark bitmap (which apparently is only 13x13 under WinXP). The code before the merge did have a similar assert, but this checked the actual drawing rectangle instead of the checkmark size. So, we could just change the assert back to what it was before. But that would not completely restore the previous behavior. Something also changed in the positioning of the text label relative to the image. Before the merge there was a margin between the image and the text of a couple of pixels. Now the text starts immediately after the image. This looks really ugly. I suspect the problem in wxMenuItem::OnMeasureItem().
One thing that might be of interest is that the items are not really owner-drawn (wxMenuItem::IsOwnerDrawn() returns false). This is because if an item with a bitmap is created its state is 'ower-drawn' at first, but when it is added to a menu the state is reset to 'not-owner-drawn' in wxMenu::DoInsertOrAppend due to the function IsLessThanStdSize() succeeding. This function has a 'FIXME' comment saying it explicitly added 4 pixels to the actual size of a checkmark bitmap size to support 16x16px images. This suggests, that there was some kind of special treatment for those images before that is now gone.

BTW, the items not really being owner-drawn also explains why the accelerators are not right aligned (see #11479). It seems that the native WinXP-menus draw the accelerators left-aligned.

comment:3 Changed 11 years ago by malcompl

IMHO we should remove the assertion check from the code or check it only when item is ownerdrawn. if the item is not ownerdrawn its height is around 18px, so it is enough space for 16px icons.

comment:4 Changed 11 years ago by rk

I tested removing those '+4's in IsLessThanStdSize(). This makes the menu items with 16x16 bitmaps become owner-drawn. After that the assert is no longer triggered. The problem of the bitmap being too close to the label is still there. And the accelerators are now right-aligned (see my last comment in #11479). But other than that it seems to work fine.

comment:5 Changed 11 years ago by TNikolay

If there any chance that this problem will be fixed soon? Due to this bug there is no possibilities to use latest version from trunk after 23 January  for developing and testing... so, I forced to use  old version, but there are a lot of useful commits in last month :(

comment:6 Changed 11 years ago by malcompl

I will try investigate and fix this problem in some next days.

comment:7 Changed 11 years ago by TNikolay

I'm sorry for troubling you, but is there some progress? There a lot of new improvements for last two months that are useful for using, but there is now way to update due to this error...

comment:8 Changed 11 years ago by vadz

  • Milestone 2.9.1 deleted

I'll apply the workaround/fix of comment:4 and remove the "+4" which really doesn't look like useful in this function. I still hope Malcolm can have a look at this code to address the remaining issues. But at least it shouldn't be as critical any more.

comment:9 follow-up: Changed 11 years ago by VZ

(In [64088]) Remove the extra margins when checking owner drawn menu icons size.

The extra +4 in IsLessThanStdSize() functions resulted in assert failures
under Windows XP after the ownerdraw drawing changes so remove it as nobody
knew why was it there anyhow.

Also replace IsLessThanStdSize() with IsGreaterThanStdSize() to allow using it
directly instead of always testing for IsLessThanStdSize().

See #11657.

Changed 11 years ago by TNikolay

comment:10 in reply to: ↑ 9 Changed 11 years ago by TNikolay

Thanks, now it works better but not ideal :( Icons are very closed to the menu items, see attached screenshot, left side is old version it looks fine. The right side contain screenshot of the program complied with current trunk, it seems that the vertical margins were increased, but horizontal are reduced and this is looks bad.

http://trac.wxwidgets.org/attachment/ticket/11657/menuicons.PNG

comment:11 Changed 11 years ago by vadz

  • Summary changed from Assertion fails in wxMenuItem::OnDrawItem() to Wrong margins for owner drawn items

Yes, this is clearly ugly. Unfortunately I don't know the new owner drawn code very well and so I still hope that Malcolm could take a look at this.

In any case, we need some way to reproduce the problem: does it happen in the menu sample too (possibly with a minimal patch)? And which OS/theme (XP/default?) do you use?

Thanks!

comment:12 Changed 11 years ago by rk

This problem can been seen in the menu sample. And it is present with either classic theme or the normal theme.

I think the problem is in MenuDrawData::Init. If no wxUxThemeEngine is found, the various sizes are initialized with values from system metrics and hard-coded values. But some of the margins are just initialized with zeros (ItemMargin, CheckBgMargin, ArrowMargin).
Those margins are then used in wxMenuItem::OnMeasureItem and wxMenuItem::OnDrawItem to compute the sizes and positions of the parts of the menu item. And there seems to be something missing, obviously.

I looked at the old code before the owner-drawn stuff was changed in r63220. In wxOwnerDrawn::OnMeasureItem was the following line:

    ...
    // add a 4-pixel separator, otherwise menus look cluttered
    *pwidth += 4;
    ...

Also, in wxOwnerDrawn::OnDrawItem there was code that computed the x position of the text label like this:

    ...
    // *2, as in wxSYS_EDGE_Y
    int margin = GetMarginWidth() + 2 * wxSystemSettings::GetMetric(wxSYS_EDGE_X);

    // select the font and draw the text
    // ---------------------------------


    // determine where to draw and leave space for a check-mark.
    // + 1 pixel to separate the edge from the highlight rectangle
    int xText = rc.x + margin + 1;
    ...

I guess this all was supposed to work around the fact that under XP the menu item bitmap is just a replacement for the check mark image. The new code does not seem to take into account that this check mark image is supposed to be only 13x13 pixels (that is what MenuDrawData::CheckSize says).

The problem is now to find the right place to put those magic numbers. ;)

comment:13 Changed 11 years ago by vadz

  • Milestone set to 3.0

The new code was supposed to get rid of the magic numbers, but not at the cost of breaking display... Anyhow, this definitely needs to be fixed but I simply won't have time for this before 2.9.1.

comment:14 Changed 11 years ago by malcompl

Anyway, I will try to fix it as soon as possible, I did just a few tests and analysis, I have a bit to improve calculating and drawing icons, but now hard to find time.
I'm sorry for the delay...

comment:15 Changed 11 years ago by vadz

  • Keywords regression added
  • Milestone changed from 3.0 to 2.9.2

This prevents people from upgrading to 2.9.1 so it's pretty critical. It seems hard to consider reverting to the old code (before ownerdraw branch merge) and losing all the improvements but maybe we should think about doing it if there is no other solution. This is clearly a serious regression.

comment:16 Changed 10 years ago by vadz

  • Milestone changed from 2.9.2 to 3.0
  • Summary changed from Wrong margins for owner drawn items to Wrong margins for owner drawn items bitmaps under XP

And I still don't have time to look at this so postponing it once again but the menu bitmaps look really, really ugly under XP.

Marcin, if you could look at this it would still be very welcome.

comment:17 in reply to: ↑ description Changed 9 years ago by yaroslavp

Not sure if relevant to this particular bug, but my application crashed in the field with this assert failing (the OS is Windows 7 (build 7601, Service Pack 1), 64-bit edition, the lib version is 2.9.2). All the icons in the menus are strictly 16x16. Not reproducible on any of my physical or virtual machines, never seen before on 2.8.8, can't access and debug on the failing computer (info came via automated crash report).

IMHO, 16x16 menu icons are pretty much standard and should never cause a catastrophic failure in any environment.

comment:18 Changed 9 years ago by vadz

  • Milestone changed from 3.0 to 2.9.4

I can't reproduce the assert failure neither but I agree that we should probably just remove it.

comment:19 follow-up: Changed 9 years ago by vadz

  • Milestone 2.9.4 deleted

I'll remove the assert but I have no idea by now what remains to be really fixed here. Does anybody have any simple patches to the menu sample showing the problem?

comment:20 Changed 9 years ago by VZ

(In [71386]) Remove assert checking bitmap size in wxMenuItem drawing code in wxMSW.

This assert was fatal, as usual when asserting from a WM_PAINT handler, as the
function was reentered resulting in nested asserts and program abort, so
remove it to at least let the program continue to run even if there is not
enough space for the bitmap in the menu.

There is, of course, still something wrong with the menu geometry calculations
if this happens but I can't even reproduce this any more so not sure what
exactly.

See #11657.

comment:21 in reply to: ↑ 19 Changed 3 years ago by catalin

Replying to vadz:

I have no idea by now what remains to be really fixed here.

Here the only problem still visible in menu sample is the line height of owner-drawn items (Test -> Check items with bitmaps), and that is part of #11645.

Win 10, x64, VS 2017, 10.0.16299.0, wxW 3.1.1rc

comment:22 Changed 3 years ago by vadz

  • Resolution set to fixed
  • Status changed from confirmed to closed

So this one can be closed then, right? Thanks for testing!

Note: See TracTickets for help on using tickets.