Add support for high DPI state images `wxTreeCtrl` (PR #24000)
VZ
This adds wxTreeCtrl::SetStateImages() which allows to pass a vector of bitmap bundles, possibly containing high DPI images, to the control and thus addressing #23993.
It's difficult (well, impossible) to see any changes in the sample, unfortunately, as we still don't have any high DPI icons there. I'd like to add some, but my artistic talents are non-existing or worse, so I decided not to subject anybody to their fruits. If anybody could contribute any kind of checked/unchecked and colour blob icons in 16x16 and 32x32 sizes, possibly by just finding them somewhere on the web under permissible licence, I could switch to using them in the sample and then the difference should be actually visible.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24000
Commit Summary
- 11c722c Remove confusing boolean parameter from treectrl sample
- 7af6014 Extract custom wxBitmapBundleImpl in treectrl sample
- 19158fc Fix clearing images used with wxWithImages
- b705df5 Fix not updating wxGenericTreeCtrl when clearing images
- 1190057 Fix initial size of treectrl sample window in high DPI
- db78dab Add wxTreeCtrl::SetStateImages()
File Changes
(9 files)
- M include/wx/generic/treectlg.h (1)
- M include/wx/msw/treectrl.h (2)
- M include/wx/treectrl.h (15)
- M include/wx/withimages.h (9)
- M interface/wx/treectrl.h (25)
- M samples/treectrl/treetest.cpp (135)
- M samples/treectrl/treetest.h (4)
- M src/generic/treectlg.cpp (19)
- M src/msw/treectrl.cpp (6)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24000.patch
- https://github.com/wxWidgets/wxWidgets/pull/24000.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- 5cae29e Fix initial size of treectrl sample window in high DPI
- d42aefc Add wxTreeCtrl::SetStateImages()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- ed228c9 Add high DPI versions of state icons to the treectrl sample
- bc35b31 Add wxTreeCtrl::GetStateImageCount() and HasStateImages()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
Tested backporting this to 3.2 as I don't have a working test setup for the main branch at the moment, and it looks good and seems to work! Thank you!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- d8d9d86 Don't use GetStateImageList() in wxGenericTreeCtrl implementation
- d05f454 Remove GetUpdatedImageListFor() from wxGenericTreeCtrl code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
In my testing, the latest commits here result in no item images being shown at all for the "only 24x24" bundles in my testcase. I'll try to figure out why...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
In my testing, the latest commits here result in no item images being shown at all for the "only 24x24" bundles in my testcase. I'll try to figure out why...
Oops, thanks for testing. The image list must somehow be still used somewhere, but I don't see where immediately.
Please let me know if you can find something quickly, but if not, I'll debug it myself, as it's my bug, after all, so I should find and fix it too...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
Never mind, this is a problem on the KiCad side, as we were relying on the old image list API.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
OK, after fixing that, I'm back to where I was with manually patching out the assert: There is no longer an assertion, but the 24x24 images are drawn at 24x24 logical size rather than at 16x16
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Just to be sure: the old wxImageList-based API is still supposed to work, same as before, if you use it only. But if you use SetImages(), GetImageList() indeed doesn't work any more in the sense that it just returns nullptr.
Thinking more about this, it's not a good change to make in 3.2 branch as there definitely could be some code doing just this by now. And I'm not even really sure if it's a good thing to change in master because the native MSW wxTreeCtrl still uses wxImageList internally and so its GetImageList() still returns something valid... I'm afraid I'm going to have to revert the last commit here, finally, sorry for the confusion/waste of time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
But if you use SetImages(), GetImageList() indeed doesn't work any more in the sense that it just returns nullptr.
Yes, this was the issue, KiCad was using GetImageList and then canceling the assignment of images when it returned nullptr
Thinking more about this, it's not a good change to make in 3.2 branch as there definitely could be some code doing just this by now.
OK, I guess I will stand by to see what you come up with
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
Can you maintain the m_imageList around as a "shadow copy" to avoid breaking code that depends on it, but do the actual image size and painting code with the bitmap bundles if they are present?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Can you maintain the m_imageList around as a "shadow copy" to avoid breaking code that depends on it, but do the actual image size and painting code with the bitmap bundles if they are present?
Yes, this is exactly the plan (and how it already works for the normal images), but we'd better avoid the asserts when creating this (unused) image list in this case :-/
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think merging both this one and #24033 should result in correct behaviour for the state images too, but I'd appreciate if you could please confirm this. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
With them both cherry-picked:
- There are no assertions
- State image behavior seems correct
- In the case with the "bad" set of images for the regular (non-state) images, where I have a mix of 16x16 and 24x24, the rendering is still not correct: the 24x24 images are rendered larger, not scaled down to 16x16 like they should be.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks again for testing!
I can reproduce the problem by doing the following in MyTreeCtrl::CreateImages() of the treetest sample:
auto fileIcon = wxArtProvider::GetIcon(wxART_NORMAL_FILE, wxART_LIST, wxSize(16, 16)); auto folderIcon = wxArtProvider::GetIcon(wxART_FOLDER, wxART_LIST, wxSize(24, 24)); wxVector<wxBitmapBundle> images; images.push_back(fileIcon); images.push_back(fileIcon); images.push_back(folderIcon); images.push_back(folderIcon); images.push_back(folderIcon); SetImages(images);
but I'm not really sure what to do about it: should we really shrink 24px images to 16px? This will look horrible... Scaling 16px ones up to 24px won't be much better, but still preferable, probably.
What I really don't understand is how did it work before, it looks like we always assumed that all images in wxGenericTreeCtrl were of the same size. What am I missing here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think I'm going to merge this for now, especially if we want to backport it to 3.2, as this really needs to be done a.s.a.p. Handling of heterogenous bitmaps can be improved later, but in the meanwhile it will be already nice to at least have support for the high DPI state bitmaps of the same size.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
but I'm not really sure what to do about it: should we really shrink 24px images to 16px? This will look horrible... Scaling 16px ones up to 24px won't be much better, but still preferable, probably.
I don't think this needs to be addressed here and now, but in my opinion this is a confusing point about how wxBitmapBundle works and a downside of its current API. Reading some of the API docs led me to believe that it was possible to get a wxBitmapBundle to produce a scaled bitmap to an arbitrary size not present in the bundle, and the way it would do so is to pick the closest bitmap available (larger than the target size, if possible) and scale that bitmap to fit.
Instead it seems like currently it is not really possible to create a wxBitmapBundle containing only a 24x24 image and get out a 16x16 (physical or logical) image, except for the case where the display's content scale factor is 0.75 or 1.33.
While I can fix my current use case by just generating the 16x16 and 32x32 bitmaps to add to this bundle, it seems like this still limits the usefulness of wxBitmapBundle: there are cases where it would be nice to pack in an arbitrary set of bitmaps, and be able to retrieve out an arbitrary size bitmap, with the size based on what the calling function wants, not (only) what the display scaling factor is.
What I really don't understand is how did it work before, it looks like we always assumed that all images in wxGenericTreeCtrl were of the same size. What am I missing here?
Sorry if I was not clear: this has never worked with bitmap bundles. Your MRs don't introduce this behavior, they just don't "fix" it either.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I don't think this needs to be addressed here and now, but in my opinion this is a confusing point about how wxBitmapBundle works and a downside of its current API. Reading some of the API docs led me to believe that it was possible to get a wxBitmapBundle to produce a scaled bitmap to an arbitrary size not present in the bundle,
Yes, it should be possible to do this. I suspect that there might be a bug with the setting of the scale factor in the bitmaps returned by it, e.g. with (something similar to) your test case I do get a bitmap with physical size 24x24 but its scale factor is set to ⅔ which makes it actually appear as 16x16 on the screen. I need some more time to look into this and make sure that this is indeed what happens, but if I'm right, it's the code which is wrong and not the documentation.
and the way it would do so is to pick the closest bitmap available (larger than the target size, if possible) and scale that bitmap to fit.
Yes, again, it's supposed to work like this. It's just that the results are so bad for anything but x2 scaling that it really shouldn't be used.
While I can fix my current use case by just generating the 16x16 and 32x32 bitmaps to add to this bundle, it seems like this still limits the usefulness of wxBitmapBundle: there are cases where it would be nice to pack in an arbitrary set of bitmaps, and be able to retrieve out an arbitrary size bitmap, with the size based on what the calling function wants, not (only) what the display scaling factor is.
I don't think it's going to be that useful considering the poor visual results, but, yes, it should definitely be possible.
What I really don't understand is how did it work before, it looks like we always assumed that all images in wxGenericTreeCtrl were of the same size. What am I missing here?
Sorry if I was not clear: this has never worked with bitmap bundles. Your MRs don't introduce this behavior, they just don't "fix" it either.
OK, this is a relief, I was wondering what I could have missed here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
I don't think it's going to be that useful considering the poor visual results, but, yes, it should definitely be possible.
I agree the results will not look great, but from my point of view the critical thing is that it is rendered in the same logical space as desired, because right now they are rendering at a larger logical size than they should be, which impacts the rest of the layout of nearby widgets.
If the only problem is scaling artifacts, what we plan to do is pre-render bitmaps of the most common sizes, but it still will be the case that on some platforms, some users will set scaling factors that are not common, and scaling will be necessary. This tradeoff is OK from my point of view, as long as the GUI layout remains consistent.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
OK, so after spending a lot of time on this I think that I am finally sure about what's going on here and what needs to be done. So:
wxBitmapBundlebehaves correctly in the vacuum (this is a relief), i.e. the bitmap returned from itsGetBitmap()has the correct scale factor when it is used on its own.- However, this is not the right thing to do when using multiple images as we need to use the best size for all of them and not each one of them (which may differ), so
wxWithImagesneeds to override this logic, which is done in the updated #24033. - But for this to work we also need to ensure that we're actually using the correct DPI scaling in the first place, which wasn't the case in wxGTK until #24040.
So, applying both the PRs mentioned above should make it work as good as possible for the normal images. I'll rebase this one once the other 2 are merged in order to check that state images work too, but I don't see why they shouldn't.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@craftyjon Do you plan to test this in the near future or should I just merge it and fix it later if necessary?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
Sorry I lost track of this and didn't realize you were waiting for me, I thought you already planned to just merge something. I tested this through manual cherry-picking before, and it seemed fine but I thought you may be making further changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
No, I don't have anything else planned for now, so I'll just merge everything.
The changes turned out to be so extensive that I'm not sure if it's wise to try to backport them to 3.2, but I could try to, if you think it would be useful -- please let me know. But let's confirm that everything is working as expected in master first.
Thanks again for your testing!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #24000.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
Thank you. I'm going to attempt to backport them to KiCad's 3.2 branch for testing, since that's what we're building against on macOS right now. It would be OK if we just carried these in our branch if they are too much to make it to the "official" 3.2 branch; we just will have to wait for a newer wx to support it on other platforms besides macOS in that case.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.