Opened 3 years ago

Closed 8 months ago

#13878 closed defect (fixed)

Menu item loses Its icon if it is removed and re-attached

Reported by: wxLuis Owned by:
Priority: normal Milestone: 3.1.0
Component: wxMSW Version: dev-latest
Keywords: wxMenuItem remove ownerdrawn Cc:
Blocked By: Blocking:
Patch: yes

Description

In Reference to Ticket #4247 (closed defect: worksforme)

It does not work for me. I compiled the menu sample and added the two
lines in the previous ticket

fileMenu->Remove(item);
fileMenu->Append(item);

and the bitmap image was removed from the
menu Item. Win 7 32 bits

See attached image

Attachments (3)

Capture.JPG download (248.3 KB) - added by wxLuis 3 years ago.
Jpg Image
remove-menu-item.patch download (550 bytes) - added by awi 8 months ago.
Set owner-drawn flag on removing item.
menu-menuitem.patch download (10.2 KB) - added by awi 8 months ago.
Patch rearranging wxMenu/wxMenuItem with regards to IsOwnerDrawn() function

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by wxLuis

Jpg Image

comment:1 Changed 3 years ago by vadz

  • Status changed from new to infoneeded_new
  • Summary changed from MenuItem looses Its Icon if it is removed and re-attached to Menu item loses Its icon if it is removed and re-attached

Well, the screenshot doesn't show much... (it's difficult to make a snapshot of opened menu, I can only do this in a VM).

Anyhow, I'm not sure what to do about this, there shouldn't be any difference between our test environments (I use VC9 and not 10 but I don't see how could it matter) and looking at the code Append() should set up the bitmap.

Please try to debug it as I'm unlikely to do anything about this without being able to reproduce the problem. Can you see the difference between the first execution of Append() for this item (when, I assume, the bitmap does appear) and the second one?

TIA!

Changed 8 months ago by awi

Set owner-drawn flag on removing item.

comment:2 Changed 8 months ago by awi

  • Keywords wxMenuItem remove ownerdrawn added
  • Patch set
  • Status changed from infoneeded_new to new
  • Version changed from 2.9.3 to dev-latest

Indeed, if menu item has bitmap(s) and it is removed from the menu and then appended again then the bitmap is no longer displayed.
This happens because if the item is removed from the menu its owner-drawn state is not reset correctly.
Item with bitmaps which is not attached to the menu should be marked as owner-drawn (this state is verified/reset when the item is later appended to the menu).
Patch to fix the issue attached. (attachment:ticket:13878:remove-menu-item.patch)

Test case:

  • samples/menu/menu.cpp

    old new  
    534534    item->SetBitmap(copy_xpm); 
    535535    fileMenu->Append(item); 
    536536    fileMenu->AppendSeparator(); 
     537    fileMenu->Remove(item);  
     538    fileMenu->Append(item);  
    537539#endif // USE_LOG_WINDOW 
    538540 
    539541    fileMenu->Append(Menu_File_ShowDialog, wxT("Show &Dialog\tCtrl-D"), 

comment:3 Changed 8 months ago by vadz

  • Milestone set to 3.1.0
  • Status changed from new to confirmed

Thanks, this definitely fixes the problem and I'll apply it if it can't be solved in any other way but I wonder if we shouldn't rethink our approach to handling owner drawn status completely, because it's rather unclear now and results in many bugs (including this one).

I think that it might be better to let wxMenuItem::IsOwnerDrawn() always return true as long as any bitmaps or colours/fonts are set and use MustUseOwnerDrawn() (which should probably be renamed to something like MSWMustUseOwnerDrawn() to better show that it's MSW-specific) when we need to know if it's owner-drawn at MSW level. This would allow us to avoid all the tricky business of calling SetOwnerDrawn(false) just at the right moment.

What do you think?

comment:4 Changed 8 months ago by awi

Right, current way of dealing with owner-drawn state is rather unintuitive and misleading.
I think it makes sense to reshape it a bit. I will prepare something.

Changed 8 months ago by awi

Patch rearranging wxMenu/wxMenuItem with regards to IsOwnerDrawn() function

comment:5 Changed 8 months ago by awi

Attached patch introduces following changes/fixes to the current wxMenu/wxMenuItem behaviour:

  1. wxMenuItem::IsOwnerDrawn() indicates only and only whether the item has any nonstandard settings regarding bitmaps, fonts, colours which cannot be represented in a native manner while drawing.
  2. wxMenu::DoInsertOrAppend() is rearranged and it no longer relies on the previous wxMenuItem::IsOwnerDrawn() implementation.
  3. Bitmaps can be now used both in owner-drawn and native modes so wxMenuItem::SetBitmap() function family is now exposed not only in owner-drawn mode.

This patch seems to fix both the issue with removing/re-attaching the item. It also seems that there is no regression with regards to the issue with setting the bitmap before/after attaching the item itself to the menu.
It was tested with library compiled with wxUSE_OWNER_DRAWN set to 0 and also to 1. It was also tested in real owner-drawn mode (this happened by chance under XP).

One minor design question left: One of the crucial here Win API function SetMenuItemInfo() is not supported under Win CE and also seems that Digital Mars compiler has/had(?) some problems with it. In one place it was used originally a conditional compilation statement to bypass this issue but it was not implemented for all occurrences of this API.
I don't know what is the policy regarding Win CE / Digital Mars compatibility so it is untouched.

comment:6 Changed 8 months ago by vadz

Thanks a lot, this goes a long way towards making this code more readable!

I don't think anybody cares about Windows CE by now (I'm pretty sure the build for it is broken anyhow) and I'm pretty sure nobody cares about DMC any more (which is no longer developed since a few years and which definitely can't compile current wx code). So I'd be fine with removing all this cruft, I'd just prefer to do it all at once.

comment:7 Changed 8 months ago by VZ

(In [76201]) Rename wxMenuItem::MustUseOwnerDrawn() to MSWMustUseOwnerDrawn().

No real changes, just make it clear that this method is MSW-specific and is
about using owner drawn items at MSW level and not wx one.

See #13878.

comment:8 Changed 8 months ago by VZ

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

(In [76202]) Make wxMSW owner drawn menu item code more clear and correct.

The user-visible effect of this change is that removing the item from the menu
and adding it back doesn't lose its icon any more.

At the code level, wxMenuItem::SetBitmaps() and related methods are
implemented outside of "#if wxUSE_OWNER_DRAWN" which allows to use them even
in minimalistic library builds. And IsOwnerDrawn() is not used any more to
determine whether the item has bitmaps, just only if it's really owner drawn,
making the code more clear and fixing at least the bug above and possible more.

Closes #13878.

Note: See TracTickets for help on using tickets.