macOS: Fixes to incorrect NSImage creation from wxBitmapBundle and missing menu bitmaps (PR #25744)

Václav Slavík

unread,

Aug 30, 2025, 10:18:14 AM (8 days ago) Aug 30

to wx-...@googlegroups.com, Subscribed

This PR contains two commits that appear unrelated at first, but the underlying problem in both of them is the same thing: misunderstanding of what `NSImage` is (a direct equivalent of `wxBitmapBundle`) and creating it through a lossy conversion to `wxBitmap` (which unlike on other platforms *doesn't* have a native equivalent).

### wxMenu bitmaps

The primary commit throws out all of the `SetupBitmaps()` scaffolding from wxOSX and reverts most of 337940f, 616e7c8, plus bits of c0dbe80; 338cd95 is also related). It then replaces all that code with a straightforward implementation that just directly sets the bundle/NSImage in `wxMenuItemImpl`.

This fixes multiple bugs, introduced by 337940f, in wxOSX implementation of menu icons:

1. Bitmaps set on items in the global Mac menu were ignored.
2. So were the ones set in the (also Mac-specific) app menu.
3. Loosing compatibility with `NSImage`-backed `wxBitmap` and rendering them without appearance awareness and in low resolution in the global menu

Václav Slavík

unread,

Aug 30, 2025, 10:22:26 AM (8 days ago) Aug 30

to wx-...@googlegroups.com, Push

@vslavik pushed 2 commits.

cd85a0f Fix broken wxMenuItem bitmap setting on macOS
9a4d91f Fix multiple cases of wrong NSImage use in wxDVC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Václav Slavík

unread,

Aug 30, 2025, 10:24:31 AM (8 days ago) Aug 30

to wx-...@googlegroups.com, Subscribed

vslavik left a comment (wxWidgets/wxWidgets#25744)

One more thing: this restores the logic to pre- 337940f and does the same as the other wxMenuItem::Set/Update methods do:

void wxMenuItem::SetBitmap(const wxBitmapBundle& bitmap)
{
    wxMenuItemBase::SetBitmap(bitmap);
    UpdateItemBitmap();
}

void wxMenuItem::UpdateItemBitmap()
{
    if ( !m_parentMenu )  // ??????????
        return;

    if ( m_bitmap.IsOk() )
    {
        GetPeer()->SetBitmap(m_bitmap);
    }
}

However, @csomor, I don't understand why the !m_parentMenu check is there? I don't see any corresponding code to create native representations in DoInsertOrAppend (they weren't there before the reverted changes either)? It seems to be that this never worked correctly for detached menu items and is unnecessary: the peer object is already there and has a perfectly good NSMenuItem, so this check is unnecessary in all the UpdateItemXXX methods, no? Am I missing something here?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

Aug 30, 2025, 10:49:11 AM (8 days ago) Aug 30

to wx-...@googlegroups.com, Subscribed

@vadz approved this pull request.

Thanks, the changes indeed make sense to me and it's, of course, nice to simplify the code while fixing bugs.

Concerning NSImage in the public API: it looked wrong to me to provide access to impl-specific details in wxBitmapBundler itself. If we need a way to do it from outside wx, perhaps we should add void* wxBitmapBundleImpl::GetNativeHandle() const { return nullptr; } and override it in wxOSXImageBundleImpl to return the NSImage? We could probably also get rid of gs_nativeImages then...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Václav Slavík

unread,

Aug 30, 2025, 11:28:04 AM (8 days ago) Aug 30

to wx-...@googlegroups.com, Subscribed

vslavik left a comment (wxWidgets/wxWidgets#25744)

If we need a way to do it from outside wx, perhaps we should add void* wxBitmapBundleImpl::GetNativeHandle() const { return nullptr; }

Honestly, why reinvent the wheel? Anything untyped or different from wxBitmap is not worth the hassle. That is:

#ifdef __WXOSX__
WX_NSImage GetNSImage() const  { return wxOSXGetImageFromBundle(*this); }
static wxBitmapBundle FromNSImage(WX_NSImage img) { return wxOSXMakeBundleFromImage(img); }
#endif

This allows you to use NSImage easily :SetBitmap([NSImage imageName:@"..."]);

wxOSX is a special case here. It is the only platform where native handle makes sense, so why over-generalize it?

and override it in wxOSXImageBundleImpl to return the NSImage?

Note the that the global mapping - as far as I could understand - is not limited to wxOSXImageBundleImpl and the design of these classes necessitates hooking OSX-specific code into other implementations (wxBitmapBundleImplSet here):
https://github.com/wxWidgets/wxWidgets/blob/fd0c23ee24c37035a4f87c963e4844b1e0bdb231/src/common/bmpbndl.cpp#L236-L238

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Aug 30, 2025, 1:16:28 PM (8 days ago) Aug 30

to wx-...@googlegroups.com, Subscribed

vadz left a comment (wxWidgets/wxWidgets#25744)

Honestly, why reinvent the wheel?

Having the accessor function in wxBitmapBundle itself means that we'll have to always use gs_nativeImages which doesn't seem great (adding a static FromNSImage() is fine though).

Note the that the global mapping - as far as I could understand - is not limited to wxOSXImageBundleImpl and the design of these classes necessitates hooking OSX-specific code into other implementations (wxBitmapBundleImplSet here):

I would like to change this too, of course, but I'm too afraid of touching this code...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Stefan Csomor

unread,

Aug 31, 2025, 12:26:04 PM (7 days ago) Aug 31

to wx-...@googlegroups.com, Subscribed

csomor left a comment (wxWidgets/wxWidgets#25744)

However, @csomor, I don't understand why the !m_parentMenu check is there? I don't see any corresponding code to create native representations in DoInsertOrAppend (they weren't there before the reverted changes either)? It seems to be that this never worked correctly for detached menu items and is unnecessary: the peer object is already there and has a perfectly good NSMenuItem, so this check is unnecessary in all the UpdateItemXXX methods, no? Am I missing something here?

No, you are not :-) we are missing the Carbon implementation where there was no native menu item and everything had to be done via the parent menu. So these checks are not needed neither for Cocoa nor UIKit.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Stefan Csomor

unread,

Aug 31, 2025, 12:31:51 PM (7 days ago) Aug 31

to wx-...@googlegroups.com, Subscribed

@csomor approved this pull request.

Thank you, looks fine to me, I have not yet discovered where we really do lose the native NSImage that was stored in the wxBitmap, but I think having the bundle in the API makes things more safe in regard to keeping multi-res native. So +1 from me

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Aug 31, 2025, 5:58:53 PM (7 days ago) Aug 31

to wx-...@googlegroups.com, Subscribed

vadz left a comment (wxWidgets/wxWidgets#25744)

Vaclav, do you want to remove the parent menu check as part of this PR or should I merge/push it as is? There is no hurry either way, I just don't want to want this prematurely nor to leave it unmerged for too long unnecessarily.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Václav Slavík

unread,

Sep 3, 2025, 11:14:27 AM (4 days ago) Sep 3

to wx-...@googlegroups.com, Push

@vslavik pushed 1 commit.

4437050 Remove unnecessary m_parentMenu checks in wxOSX

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Václav Slavík

unread,

Sep 3, 2025, 11:38:06 AM (4 days ago) Sep 3

to wx-...@googlegroups.com, Subscribed

vslavik left a comment (wxWidgets/wxWidgets#25744)

Vaclav, do you want to remove the parent menu check as part of this PR or should I merge/push it as is?

Sorry, I was AFK. I added the removal in 4437050 now.

I have not yet discovered where we really do lose the native NSImage that was stored in the wxBitmap

This was because the SetupBitmaps code called wxMenuItemBase::GetBitmap(), which under the hood uses GetBitmapFromBundle(), which eventually calls this code where forcing scale factor does the destruction inwxBitmapBundle::GetBitmap(size):
https://github.com/wxWidgets/wxWidgets/blob/170c38631458dcef648c31b9d98dcc20a8c57f9c/src/common/bmpbndl.cpp#L587-L590

But I didn't verify this hypothesis once I realized the fix was to sidestep all this...

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Václav Slavík

unread,

Sep 3, 2025, 11:48:09 AM (4 days ago) Sep 3

to wx-...@googlegroups.com, Subscribed

vslavik left a comment (wxWidgets/wxWidgets#25744)

Having the accessor function in wxBitmapBundle itself means that we'll have to always use gs_nativeImages which doesn't seem great (adding a static FromNSImage() is fine though).

I don't see how the global map is related to having wxOSXGetImageFromBundle exposed publicly? Note that the map and all return values are implicitly ref-counted by the Objective-C++ compiler and any user of the API will necessarily invoke it from Obj-C++, so while it looks like passing raw pointers, it's best thought of as shared_ptr, i.e. it would be possible to reimplement NSView* GetNSImage() w/o the global map.

Note the that the global mapping - as far as I could understand - is not limited to wxOSXImageBundleImpl and the design of these classes necessitates hooking OSX-specific code into other implementations (wxBitmapBundleImplSet here):

I would like to change this too, of course, but I'm too afraid of touching this code...

I'm not a fan of neither the global map nor this, but AFAIK both are consequences of

the design choice to allow multiple implementations, instead of having one per platform and
the fact that you will always need efficient way to convert any wxBitmapBundle instance into NSImage on macOS

"Efficient" implies you can't just do it every time a conversion is done. And point 1 forces the implementation to be off-band and not part of wxOSXImageBundleImpl ...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Sep 6, 2025, 2:48:22 PM (22 hours ago) Sep 6

to wx-...@googlegroups.com, Subscribed

vadz left a comment (wxWidgets/wxWidgets#25744)

the design choice to allow multiple implementations, instead of having one per platform and

This was my decision and I still think it makes sense, at least for the other platforms that don't provide any standard/native way of doing what this class does. But it would almost certainly be awkward to use a different approach in wxOSX.

the fact that you will always need efficient way to convert any wxBitmapBundle instance into NSImage on macOS

We could add an extra virtual member function returning NSImage to wxBitmapBundleImpl under Mac to make it possible to handle this efficiently in wxOSXImageBundleImpl or am I missing something?

Anyhow, for now I'll merge this already, thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Sep 6, 2025, 3:21:32 PM (22 hours ago) Sep 6

to wx-...@googlegroups.com, Subscribed

Merged #25744 into master.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward