Allow using wxBitmapBundle with wxDataViewBitmapRenderer (PR #22411)
VZ
While looking at #22359, I've realized that we couldn't use wxBitmapBundle to show images in wxDVC, so I've fixed this and now the following diff
diff --git a/samples/dataview/dataview.cpp b/samples/dataview/dataview.cpp index 4b2f1d933e..4057a43959 100644 --- a/samples/dataview/dataview.cpp +++ b/samples/dataview/dataview.cpp @@ -904,7 +904,7 @@ void MyFrame::BuildDataViewCtrl(wxPanel* parent, unsigned int nPanel, wxDataViewColumn* const colCheckIconText = new wxDataViewColumn ( L"\u2714 + icon + text", - new wxDataViewCheckIconTextRenderer(), + new wxDataViewBitmapRenderer(), // also works with explicit "wxBitmap" MyListModel::Col_ToggleIconText, wxCOL_WIDTH_AUTOSIZE ); diff --git a/samples/dataview/mymodels.cpp b/samples/dataview/mymodels.cpp index d6d76c2414..5aa6f20388 100644 --- a/samples/dataview/mymodels.cpp +++ b/samples/dataview/mymodels.cpp @@ -472,6 +472,11 @@ void MyListModel::GetValueByRow( wxVariant &variant, break; case Col_ToggleIconText: + if ( row % 2 ) + variant << m_icon[row % 2]; // .GetBitmap(wxSize(16, 16)); -- not needed any longer, but still works too + else + variant = wxNullVariant; + break; { wxString text; wxCheckBoxState state;
compiles 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!
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22411
Commit Summary
- dc90e92 Add wxBitmapBundle::Clear()
- eb361a1 Use a single wxBitmapBundle in wxDataViewBitmapRenderer
- 23e815b Allow relaxing the check for variant type in wxDataViewRenderer
- d8abdfb Accept either bitmaps or icons in wxDataViewBitmapRenderer
- 5556ea8 Add support for storing wxBitmapBundle in a wxVariant
- 5017e54 Accept wxBitmapBundle in wxDataViewBitmapRenderer too
- 34fd6d4 Use wxBitmapBundle as default type in wxDataViewBitmapRenderer
File Changes
(14 files)
- M include/wx/bmpbndl.h (18)
- M include/wx/dvrenderers.h (14)
- M include/wx/generic/dvrenderers.h (9)
- M include/wx/gtk/dvrenderers.h (5)
- M include/wx/osx/dvrenderers.h (5)
- M include/wx/qt/dvrenderers.h (4)
- M interface/wx/bmpbndl.h (12)
- M interface/wx/dataview.h (25)
- M src/common/bmpbndl.cpp (65)
- M src/common/datavcmn.cpp (2)
- M src/generic/datavgen.cpp (42)
- M src/gtk/dataview.cpp (29)
- M src/osx/cocoa/dataview.mm (19)
- M src/qt/dvrenderers.cpp (7)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22411.patch
- https://github.com/wxWidgets/wxWidgets/pull/22411.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.
- 8d3e7fd Accept wxBitmapBundle in wxDataViewBitmapRenderer too
- 1b82a9f Use wxBitmapBundle as default type in wxDataViewBitmapRenderer
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
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.
—
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
Apparently nobody feels brave enough to have a look at this,
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.
—
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
@vslavik commented on this pull request.
> @@ -155,6 +155,15 @@ class WXDLLIMPEXP_CORE wxDataViewRendererBase: public wxObject
wxString GetVariantType() const { return m_variantType; }
+ // Check if the given variant type is compatible with the type expected by
+ // this renderer: by default, just compare it with GetVariantType(), but
+ // can be overridden to accept other types that can be converted to the
+ // type needed by the renderer.
+ virtual bool IsCompatibleVariantType(const wxString& variantType) const
It's a bit weird API now: on one hand, this function, on the other, explicitly passing single variant type to the ctor and having GetVariantType(). The latter two heavily imply there's one type per renderer.
IsCompatibleVariantType() makes more sense than a single type, of course, so I don't even know if we can do anything here. Maybe document GetVariantType() as returning the primary type, and referencing the other function from its docs?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -155,6 +155,15 @@ class WXDLLIMPEXP_CORE wxDataViewRendererBase: public wxObject
wxString GetVariantType() const { return m_variantType; }
+ // Check if the given variant type is compatible with the type expected by
+ // this renderer: by default, just compare it with GetVariantType(), but
+ // can be overridden to accept other types that can be converted to the
+ // type needed by the renderer.
+ virtual bool IsCompatibleVariantType(const wxString& variantType) const
Yes, there is some intrinsic incompatibility between allowing wxDataViewBitmapRenderer to support multiple variant types and the idea of having a single variant type for the renderer. But I think we really want to allow the former and the only alternative I can see would be to allow a wxVariant of type "bitmap" to contain either a single bitmap or a bitmap bundle, but this would be even weirder, IMO.
So I guess I'll just improve the documentation as you suggest for now, because I still don't see any really natural way of dealing with it, especially a backwards-compatible one (e.g. it could be more natural to have GetVariantTypes() -> vector<string> instead of the current function, but this would require tons of changes and ugly hacks to preserve compatibility that don't seem to be worth it).
—
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
@vslavik commented on this pull request.
> @@ -155,6 +155,15 @@ class WXDLLIMPEXP_CORE wxDataViewRendererBase: public wxObject
wxString GetVariantType() const { return m_variantType; }
+ // Check if the given variant type is compatible with the type expected by
+ // this renderer: by default, just compare it with GetVariantType(), but
+ // can be overridden to accept other types that can be converted to the
+ // type needed by the renderer.
+ virtual bool IsCompatibleVariantType(const wxString& variantType) const
But I think we really want to allow the former
Yes, definitely.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Apparently nobody feels brave enough to have a look at this,
Sorry I wasn't able to accommodate you in the past 3 business days :/
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!
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.
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, 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!
—
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 1 commit.
- 140b88a Improve wxDataViewRenderer variant type documentation
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
Tested provided example on Windows and it seems to work good.
In include/wx/qt/dvrenderers.h:
>
wxDataViewBitmapRenderer( const wxString &varianttype = GetDefaultType(),
wxDataViewCellMode mode = wxDATAVIEW_CELL_INERT,
int align = wxDVR_DEFAULT_ALIGNMENT );
bool SetValue( const wxVariant &value );
bool GetValue( wxVariant &value ) const;
+
+ bool IsCompatibleVariantType(const wxString& variantType) const;
wxOVERRIDE like the other platforms?
> @@ -54,6 +54,13 @@ bool wxDataViewBitmapRenderer::GetValue( wxVariant &value ) const
return false;
}
+bool
+wxDataViewBitmapRenderer::IsCompatibleVariantType(const wxString& variantType) const
+{
+ return variantType == wxS("wxBitmap")
Does it needs wxBitmapBundle like the other platforms?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -54,6 +54,13 @@ bool wxDataViewBitmapRenderer::GetValue( wxVariant &value ) const
return false;
}
+bool
+wxDataViewBitmapRenderer::IsCompatibleVariantType(const wxString& variantType) const
+{
+ return variantType == wxS("wxBitmap")
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In include/wx/qt/dvrenderers.h:
>
wxDataViewBitmapRenderer( const wxString &varianttype = GetDefaultType(),
wxDataViewCellMode mode = wxDATAVIEW_CELL_INERT,
int align = wxDVR_DEFAULT_ALIGNMENT );
bool SetValue( const wxVariant &value );
bool GetValue( wxVariant &value ) const;
+
+ bool IsCompatibleVariantType(const wxString& variantType) const;
This would result in "inconsistent use of override" warnings as it's not used for the other methods of the same class, so omitting it looks like a lesser evil (of course, ideal would be to add it everywhere but considering this is nothing more than a stub anyhow, I didn't feel like doing it would be useful).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vslavik Do you still plan to test this relatively soon?
—
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
@vadz I was slowed down by wxBitmapBundle changes breaking, of all things, the one platform that did natively handle bundles in wxBitmap just fine previously, and none of bitmaps, including in wxDVC, work in Poedit on macOS. Figuring that out seemed rather more pressing (patches incoming now that I did).
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz I was slowed down by
wxBitmapBundlechanges breaking, of all things, the one platform that did natively handle bundles inwxBitmapjust fine previously, and none of bitmaps, including in wxDVC, work in Poedit on macOS.
Oops, this was definitely unexpected. But now I'm even more glad that you could test it!
Figuring that out seemed rather more pressing (patches incoming now that I did).
Looking forward to them, I'd like to understand how did I break it this time...
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.
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.
—
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
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 wxBitmapBundle transition.
As per 84cb293 ... I think NN is better (well, less worse).
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).
—
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 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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #22411 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.