Improve image size checks in wxImageList::Add()

[wxtrac]

Issue migrated from trac ticket # 18188

component: GUI-generic | priority: normal | resolution: fixed | keywords: HiDPI

2018-08-11 19:55:12: @dkulp created the issue

---

Please remove the wxASSERT's in wxImageList::Add(wxIcon) on OSX. I'm trying to add retina icons for wxTreeListCtrl on my OSX builds, but the asserts in there means my debug builds cause a bunch of asserts whenever I start up my app. I ended up adding a "non-retina" icon and then calling Replace to replace it with the retina version as Replace doesn't invoke the asserts.

If I try to add it as a wxBitmap, that doesn't work as wxIcon wxImageList::GetIcon fails with "cannot convert from bitmap to icon".

[wxtrac]

2018-08-13 22:53:10: @vadz changed component from wxOSX to GUI-generic

2018-08-13 22:53:10: @vadz changed title from Remove wxASSERT's in wxImageList::Add to Improve image size checks in wxImageList::Add()

2018-08-13 22:53:10: @vadz commented

Do you mean the assert checking the bitmap size is correct? If so, I don't think it should be removed. However it should check the size correctly, i.e. accounting for the scale. IOW GetScaled{Width,Height}() should be used instead of just Get{Width,Height}().

Could you please check if making this change fixes the problem?

[wxtrac]

2018-08-14 01:05:54: @dkulp commented

---

wxIcon doesn't have a GetScaled set of methods. Thus, that isn't doable. On OSX, an Icon can be a multiple of the "set" size and it works.

[wxtrac]

2018-08-14 15:18:38: @vadz commented

---

wxIcon doesn't have a GetScaled set of methods.

It arguably should, at least I don't see why should it be different from wxBitmap.

On OSX, an Icon can be a multiple of the "set" size and it works.

Yes, as the assert explains, it works in the generic wxImageList implementation used under macOS. But it does not work under MSW. And we really want things not to only work in the same way under all platforms, but also to not work consistently everywhere, which is why this assert is useful.

[wxtrac]

2018-08-15 21:55:21: @dkulp commented

---

Let me change the issue to a question then: how can I assign Retina icons for use in a wxTreeViewCtrl on OSX in a supported way that doesn't generate a bunch of asserts when running in debug mode?

[wxtrac]

2018-08-16 13:19:58: @vadz changed status from new to confirmed

2018-08-16 13:19:58: @vadz commented

Sorry, I don't think we have a way to do this currently. AFAICS we need to add wxIcon::GetScaled{Width,Height}() and change the assert to use these functions to actually fix this.

[wxtrac]

2018-08-29 10:21:55: ufospoke commented

---

If I try to add it as a wxBitmap, that doesn't work as wxIcon wxImageList::GetIcon fails with "cannot convert from bitmap to icon"

Please see the attached patch to the treelist sample that shows this.

[wxtrac]

2018-08-29 10:22:55: ufospoke uploaded file treelist.patch (2.0 KiB)

patch to the treelist sample that triggers "failed in GetIcon(): cannot convert from bitmap to icon"

[wxtrac]

2018-10-30 01:45:30: @vadz commented

---

Just a note: the assert about converting bitmap to icon doesn't happen any longer in the latest master, presumably after the recent Stefan's changes.

I'll update this ticket when the fix for the primary problem is ready.

[wxtrac]

2018-10-30 02:00:27: @vadz commented

---

Actually I think fixing this is as trivial as just removing some code, see PR 997.

I also think that we need to remove much more code and get rid of Mac-specific wxImageList entirely as it seems almost exactly the same as wxGenericImageList and the parts which are not the same should be merged into the latter, so I'll do this next.

[wxtrac]

2018-10-30 09:31:30: @csomor changed status from confirmed to closed

2018-10-30 09:31:30: @csomor set resolution to fixed

2018-10-30 09:31:30: @csomor commented

In d118ca6c5689f8ba4fac2e96b9354e80d9a42627: Remove wxImageList::Add() overload taking wxIcon from wxOSX (#997)

This is not necessary as wxIcon is implicitly convertible to wxBitmap anyhow, so calling Add(icon) works using the existing Add(wxBitmap) overload, and is actually harmful because of the wrong icon size check in this function, which used physical bitmap size instead of the logical (scaled) size.

Closes #18188.