Allow using wxBitmapBundle with wxDataViewBitmapRenderer #22411
Conversation
This is just a convenient helper for resetting the bundle contents which seems more readable than assigning an empty bundle to it.
Instead of using either a wxBitmap or a wxIcon, always use the same wxBitmapBundle object to store whatever we are rendering. This slightly simplifies the code and prepares for further changes, but nothing real changes yet.
Instead of checking for the exact variant type match, call the new IsCompatibleVariantType() virtual function, which still does the same check by default, but can be overridden to allow other types as well if they're accepted by the renderer. This will be soon used to allow accepting both wxBitmap (which must still be accepted for compatibility) and wxBitmapBundle (which is the simplest way to support high DPI) in wxDataViewBitmapRenderer at once.
Allow using either wxBitmap or wxIcon in wxDataViewBitmapRenderer independently of the type the renderer was created with because this is convenient and there is no real danger in allowing to mix and match bitmaps and icons together. Override the just added IsCompatibleVariantType() to implement this.
Define wxBitmapBundleVariantData without using the standard macros that only work for wxObject-derived classes, but using more or less what they expand into. This will allow using wxBitmapBundle with wxDataViewCustomRenderer subclasses in the upcoming commit.
This allows returning the entire bundle from the model GetValue() function to let the renderer itself to select the best matching bitmap to use.
The default type doesn't really matter after the previous commits, as the renderer accepts both wxBitmap and wxBitmapBundle (and also wxIcon) in any case anyhow, but it seems better to use the preferred type as the default one. Also make the documentation more useful, although an example is still lacking.
vadz
force-pushed
the
dvc-bitmap-renderer-bundle
branch
from
34fd6d4
to
1b82a9f
Compare
|
Apparently nobody feels brave enough to have a look at this, so I'm going to use the only reliably working method for getting comments on my changes: merge it and see if it breaks anything. |
Sorry I wasn't able to accommodate you in the past 3 business days :/ It looks fine to me. I wanted to actually test it, as you asked, which means updating Poedit's build to it first on macOS and Windows, and I just wasn't able to do it yet. |
Sorry, my comment was intended to be a light-hearted one, nothing more. But, also, IME any comments about the PRs tend to come either relatively quickly (1-2 days) or not at all, so I thought it was safe to assume that none were upcoming here. I'm glad to be proven wrong, thanks for looking at this!
Please let me know if you'd prefer me to wait until you can test it before I merge it. If you can, it would, of course, be better to wait until you can check that there are no unexpected problems with this. But I'd definitely like to merge this one before 3.1.7 (supposed to be released on 2022-06-06). Thanks again! |
Explain what it is in the ctor documentation and link to the new IsCompatibleVariantType() in GetVariantType() docs.
| bool | ||
| wxDataViewBitmapRenderer::IsCompatibleVariantType(const wxString& variantType) const | ||
| { | ||
| return variantType == wxS("wxBitmap") |
There was a problem hiding this comment.
Does it needs wxBitmapBundle like the other platforms?
There was a problem hiding this comment.
I guess... But then this seems to be completely unimplemented in wxQt anyhow, and wxBitmapBundle is not implemented for it neither, so it's a bit of a moot point anyhow.
|
@vslavik Do you still plan to test this relatively soon? |
|
@vadz I was slowed down by Testing of this so far: macOS is fine. wxGTK scales bitmaps in the list differently for HiDPI (NN instead of bilinear - unclear which is more correct, I'm fine with this). Didn't test Windows yet. |
Oops, this was definitely unexpected. But now I'm even more glad that you could test it!
Looking forward to them, I'd like to understand how did I break it this time...
As per 84cb293 (Make wxBitmap::Rescale() less horrible for commonly used icons, 2022-02-23), I think NN is better (well, less worse). If macOS does bilinear, we're probably not going to (be able?) change it, however. |
|
I confirm that I couldn't find anything wrong with this patchset in Poedit on either of the 3 platforms. I created #22449 for some other issues with
I have somewhat different opinion: while NN can "preserve sharp edges", it can also eradicate said edges altogether if rounding goes the wrong way when scaling down. However, I was commenting on 2x upscaling, where its reasonable (and I frankly don't know what macOS does in absence of Retina versions or what GTK+ native does). |
|
Thanks again for testing this, I'm going to merge this now as this shouldn't be affected by the art provider-related changes that still remain to be done. |
While looking at #22359, I've realized that we couldn't use
wxBitmapBundleto show images in wxDVC, so I've fixed this and now the following diffcompiles and works, i.e. shows crisp and not scaled bitmaps in high DPI.
@vslavik If you could please test this with Poedit, it would be great.
@MaartenBent @csomor Your review would be welcome as always, TIA!