Opened 8 years ago

Closed 9 months ago

#9388 closed defect (fixed)

Allow adding menu bitmaps after append [wxMSW]

Reported by: troelsk Owned by:
Priority: normal Milestone:
Component: wxMSW Version: dev-latest
Keywords: wxMenuBar wxMenu wxBitmap Cc: troelsk
Blocked By: Blocking: #3206
Patch: yes

Description

Adding bitmap to menus (wxMenuItem.SetBitmap) after the menu item is appended have no effect.
This makes it impossible to use bitmaps from wxArtProvider (or other such sources) in xrc menus.

How to reproduce:

1.
samples/xrc/myframe.cpp:

#include "../../art/quit.xpm"
Load the menubar from XRC and...
wxMenuBar* menubar = wxXmlResource::Get()->LoadMenuBar(wxT("main_menu"));
wxMenuItem* item = menubar->FindItem(XRCID("unload_resource_menuitem"));
item->SetBitmap(wxBitmap(quit_xpm));
menubar->UpdateMenus();
SetMenuBar(menubar); no bitmap at all

2.
samples/menu/menu.cpp:

[Move the SetBitmap line down below Append]
fileMenu->Append(item);
item->SetBitmap(copy_xpm); no bitmap at all

'Conclusion':

The incantations in src/msw/menu.cpp/wxMenu::DoInsertOrAppend are never executed. I see no easy fix for this :(

Regards

Attachments (3)

menu-ownerdraw.patch download (822 bytes) - added by troelsk 10 months ago.
Sample
menu-refactoring.patch download (9.3 KB) - added by awi 9 months ago.
Some wxMenu, wxMenuItem refactoring.
menu-set-bitmap.patch download (3.4 KB) - added by awi 9 months ago.
Changing bitmap for menu item.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by troelsk

Bug #1699175 is somewhat related.

comment:2 Changed 7 years ago by wojdyr

  • Component set to GUI-all
  • Keywords wxMenuBar added

ftr, sf bug 1699175 is now #4020

comment:3 Changed 3 years ago by vadz

  • Component changed from GUI-all to wxMSW
  • Keywords medium added
  • Status changed from new to confirmed

This still doesn't work with 2.9.3, there is no bitmap if the menu sample is modified like this:

  • samples/menu/menu.cpp

    a b MyFrame::MyFrame() 
    466466#if USE_LOG_WINDOW 
    467467    wxMenuItem *item = new wxMenuItem(fileMenu, Menu_File_ClearLog, 
    468468                                      wxT("Clear &log\tCtrl-L")); 
     469    fileMenu->Append(item); 
    469470#if wxUSE_OWNER_DRAWN || defined(__WXGTK__) 
    470471    item->SetBitmap(copy_xpm); 
    471472#endif 
    472     fileMenu->Append(item); 
    473473    fileMenu->AppendSeparator(); 
    474474#endif // USE_LOG_WINDOW 
    475475 

We need to indeed either refactor DoInsertOrAppend() to be able to reuse the part of it dealing with the bitmaps from elsewhere. Or we could just remove the item from the menu and reinsert it back with the bitmap.

Doing the latter should be simple enough if anybody is motivated...

comment:4 Changed 3 years ago by oneeyeman

Vadim,
It might be simpler but is it a right thing to do?

comment:5 Changed 23 months ago by troelsk

  • Keywords wxMenu wxBitmap added
  • Type changed from enhancement to defect
  • Version set to 2.9-svn

comment:6 Changed 14 months ago by vadz

Interestingly, as #15574 shows, the effects can be even stranger when non-default DPI is used: under XP we get a totally corrupted menu bitmap in this case, while under 7 we don't get anything at all.

In any case, this is clearly something we ought to fix, but setting the bitmap before appending remains a relatively simple workaround.

comment:7 Changed 11 months ago by awi

  • Blocking 3206 added

#3206 seems to describe the same issue.

comment:8 Changed 11 months ago by vadz

  • Blocking

(In #3206) In fact it's exactly the same as the other bug, let's keep the newer one opened as it contains slightly more information.

Changed 10 months ago by troelsk

Sample

comment:9 Changed 10 months ago by troelsk

  • Version changed from stable-latest to dev-latest

This is still broken. When adding a bitmap to a menu item then SetItemLabel() no longer works.
Demonstration:

Index: samples/docview/docview.cpp
===================================================================
--- samples/docview/docview.cpp (revision 76023)
+++ samples/docview/docview.cpp (working copy)
@@ -217,6 +217,13 @@

menuFile->AppendSeparator();
menuFile->Append(wxID_EXIT);

+
+ wxMenuItem* item;
+ item = menuFile->FindItem(wxID_EXIT);
+ item->SetBitmap(wxBitmap(notepad_xpm)); -> SetOwnerDrawn() -> problem
+ item->SetOwnerDrawn(); -> problem
+ item->SetItemLabel(item->GetItemLabel() + "123" );
problem: no effect in ownerdraw mode
+ item->SetItemLabel(item->GetItemLabel() + "\tCtrl+Q"); problem: accelerator in effect - but not visible in menu

comment:10 Changed 10 months ago by vadz

That's because there is an assumption in the code that owner drawn menu items don't need their text set at all at Windows level, but this is not true if the item is not really owner drawn but the flag was just set by SetBitmap().

The real solution is to implement wxMenuItem::SetBitmap() correctly and not call SetOwnerDrawn(true) from it but just update the MIIM_BITMAP if the system supports it as it would also fix the original bug. But this would require refactoring the code in wxMenu::DoInsertOrAppend() which nobody has been brave enough to do so far...

So for now I'll at least add a hack to fix setting of the label. But the setting of the bitmap itself after appending the item remains broken.

comment:11 Changed 10 months ago by VZ

(In [76042]) Fix setting the label for already existing menu items with bitmaps in wxMSW.

Do update the label at Windows level if we don't use MF_OWNERDRAW style,
checking for IsOwnerDrawn() is wrong because the flag it tests may be set even
if the item is not really owner drawn from Windows point of view.

This is a mess and setting the bitmap for the existing items is still broken,
but at least setting the label works now.

See #9388.

comment:12 Changed 10 months ago by VZ

(In [76044]) Fix setting the label for already existing menu items with bitmaps in wxMSW.

Do update the label at Windows level if we don't use MF_OWNERDRAW style,
checking for IsOwnerDrawn() is wrong because the flag it tests may be set even
if the item is not really owner drawn from Windows point of view.

This is a mess and setting the bitmap for the existing items is still broken,
but at least setting the label works now.

See #9388.

Changed 9 months ago by awi

Some wxMenu, wxMenuItem refactoring.

Changed 9 months ago by awi

Changing bitmap for menu item.

comment:13 Changed 9 months ago by awi

  • Blocking
  • Keywords medium removed
  • Patch set

Attached patches allow changing bitmaps in the menu at any moment. It is even possible to remove bitmap from menu by calling SetBitmap(wxNullBitmap).

  1. Patch with some wxMenu, wxMenuItem refactoring. It moves some helper functions related to wxMenuItem only from wxMenu to wxMenuItem. In order to get access to some of them (MustUseOwnerDrawn(), GetHBitmapForMenu()) wxMenu is declared as a friend of wxMenuItem class.

(If friend classes are not allowed these functions can be exposed as public ones.)
attachment:ticket:9388:menu-refactoring.patch

  1. Patch allowing changing bitmaps in the menu item.

attachment:ticket:9388:menu-set-bitmap.patch

And some test sample:

  • samples/menu/menu.cpp

    old new  
    436436// ---------------------------------------------------------------------------- 
    437437// MyFrame 
    438438// ---------------------------------------------------------------------------- 
     439#include "wx/artprov.h" 
    439440 
    440441// Define my frame constructor 
    441442MyFrame::MyFrame() 
     
    533534                                      wxT("Clear &log\tCtrl-L")); 
    534535    item->SetBitmap(copy_xpm); 
    535536    fileMenu->Append(item); 
     537    item->SetBitmap(wxArtProvider::GetBitmap("wxART_DELETE")); 
     538//    item->SetBitmap(wxNullBitmap); 
    536539    fileMenu->AppendSeparator(); 
    537540#endif // USE_LOG_WINDOW 
    538541 

comment:14 Changed 9 months ago by vadz

Thanks a lot for cleaning this up!

I have a doubt about calling SetOwnerDrawn(true) in the beginning of wxMenuItem::DoSetBitmap() however: do we really want to do this if the item is not attached to the menu yet? AFAICS the setting of "owner drawn" flag should be postponed until the item is appended to the menu, shouldn't it?

comment:15 Changed 9 months ago by vadz

After thinking more about this, I've managed to convince myself that the code is correct, so I'm going to commit it.

However I am not sure if this can be checked in to 3.0 branch: as the inline functions are changed by this patch, bad things risk happening if an application compiled using the old headers is linked with the new DLL, as "wrong" code will be executed. I'm not actually sure if it's going to be really bad or just not better than before (which would be acceptable), but when in doubt, I'm not going to put this in 3.0...

Thanks again for fixing this!

comment:16 Changed 9 months ago by VZ

(In [76190]) Refactor wxMSW: move some code from wxMenu to wxMenuItem.

This will allow reusing it for other wxMenuItem bitmap-related operations.

See #9388.

comment:17 Changed 9 months ago by VZ

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

(In [76191]) Fix setting menu item bitmaps after appending them in wxMSW.

Update the bitmap used by Windows when using non-owner-drawn items with
bitmaps.

Closes #9388.

Note: See TracTickets for help on using tickets.