Fix Retina display issues with ribbon (PR #26409)
Blake-Madden
In macOS, there were two issues:
- The area drawn around regular-sized buttons was twice the size it should be.
This PR fixes the size calculation of the button area, bitmap scaling, and implements some dead code with small button sizing.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26409
Commit Summary
File Changes
(4 files)
- M src/ribbon/art_aui.cpp (3)
- M src/ribbon/art_msw.cpp (17)
- M src/ribbon/art_msw_flat.cpp (3)
- M src/ribbon/buttonbar.cpp (33)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26409.patch
- https://github.com/wxWidgets/wxWidgets/pull/26409.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Sorry, I think changes here are not right. There should be really no need for any preprocessor checks, wx API provides all the necessary functions for translating between different kinds of pixels and wxBitmapBundle should already take care of the bitmap scale (if it really doesn't, it's a bad bug which needs to be fixed in it).
> +#ifdef __WXOSX__ + const double scale = GetContentScaleFactor(); +#else
This shouldn't be necessary at all, GetDPIScaleFactor() returns the same thing as GetContentScaleFactor() in wxOSX.
> +#ifdef __WXOSX__ + if ( bitmap.IsOk() ) + bitmap.SetScaleFactor(scale); +#endif
This is very suspicious. First of all because if we need to check for anything, it should be wxHAS_DPI_INDEPENDENT_PIXELS and not __WXOSX__ as wxGTK must be affected by the same problem. Second, and more importantly, because the bitmap returned from wxBitmapBundle must already have the correct scale factor.
> +#ifdef __WXOSX__ + // macOS DC is in logical units (points); use base sizes directly + const wxSize& bmpSizeLarge = m_bitmap_size_large; + const wxSize& bmpSizeSmall = m_bitmap_size_small; +#else
This is not necessary, it's enough to use GetContentScaleFactor() (which will return 1 under Windows) instead of GetDPIScaleFactor() below.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> +#ifdef __WXOSX__ + const double scale = GetContentScaleFactor(); +#else
const double scale = GetDPIScaleFactor() / GetContentScaleFactor(); worked for me on both MSW and macOS
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
> +#ifdef __WXOSX__ + if ( bitmap.IsOk() ) + bitmap.SetScaleFactor(scale); +#endif
I remove the guards, but I still need to call SetScaleFactor or the icons don't go down to 16x16 when the ribbon is small on macOS.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 482f88b Remove preprocessor guards
—
View it on GitHub or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26409)circlecli error is from an unrelated Lexilla warning
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Sorry for the delay, but after looking at the latest version I am still almost sure it is wrong, I hope you can find the correct solution by just setting the sizes correctly (which means expressing them in either physical or DPI-independent pixels, as necessary).
> + const double scale = GetDPIScaleFactor() / GetContentScaleFactor(); + const wxSize bmpSizeLarge = m_bitmap_size_large * scale; + const wxSize bmpSizeSmall = m_bitmap_size_small * scale;
I think this is identical to much more comprehensible
⬇️ Suggested change- const double scale = GetDPIScaleFactor() / GetContentScaleFactor(); - const wxSize bmpSizeLarge = m_bitmap_size_large * scale; - const wxSize bmpSizeSmall = m_bitmap_size_small * scale; + const wxSize bmpSizeLarge = FromDIP(m_bitmap_size_large); + const wxSize bmpSizeSmall = FromDIP(m_bitmap_size_small);
This should work as long as m_bitmap_size_xxx are indeed expressed in DIPs. Are they?
> @@ -969,6 +969,8 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
const wxBitmapBundle& bundle = disabled ?
m_bundlesLargeDisabled[idx] : m_bundlesLarge[idx];
bitmap = bundle.GetBitmap(scaledLarge);
+ if ( bitmap.IsOk() )
+ bitmap.SetScaleFactor(contentScale);
Sorry but this just can't be correct. wxBitmapBundle returns the bitmap with the scale factor consistent with the requested size, changing it later can only break things.
If you pass the correct size to GetBitmap() there should be no need for this.
> const wxSize scaledLarge = m_bitmap_size_large * scale;
const wxSize scaledSmall = m_bitmap_size_small * scale;
This is weird because it's not the same as FromDIP(). But I think this just means this is wrong and this is why you have to work around this bug with SetScaleFactor() call below.
> @@ -980,6 +982,8 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
const wxBitmapBundle& bundle = disabled ?
m_bundlesSmallDisabled[idx] : m_bundlesSmall[idx];
bitmap_small = bundle.GetBitmap(scaledSmall);
+ if ( bitmap_small.IsOk() )
+ bitmap_small.SetScaleFactor(contentScale);
Same comment as above.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -980,6 +982,8 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
const wxBitmapBundle& bundle = disabled ?
m_bundlesSmallDisabled[idx] : m_bundlesSmall[idx];
bitmap_small = bundle.GetBitmap(scaledSmall);
+ if ( bitmap_small.IsOk() )
+ bitmap_small.SetScaleFactor(contentScale);
Should I try a different approach? Sorry, I'm just struggling with this one. This small icon issue was introduced when the wxBitmapBundle support was added, but I can't trace it to what specifically is causing the issue. This was the cleanest fix I could come up with that worked on all platforms, but I'm having difficulty following the interactions going on in here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -980,6 +982,8 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
const wxBitmapBundle& bundle = disabled ?
m_bundlesSmallDisabled[idx] : m_bundlesSmall[idx];
bitmap_small = bundle.GetBitmap(scaledSmall);
+ if ( bitmap_small.IsOk() )
+ bitmap_small.SetScaleFactor(contentScale);
I am not sure what the issue is, so I can't say anything useful but the basic principles are:
- It should be never necessary to use neither
GetDPIScaleFactor()norGetContentScaleFactor()explicitly, using{To,From}DIP()should be always sufficient. wxBitmapBundle::GetBitmap(size)works, i.e. it returns the bitmap of size and scale factor compatible with physical size (so when you ask for 32x32 bitmap you can get back a 32x32 bitmap with scale factor 1 or 64x64 bitmap with scale factor 2), and so it is wrong to override the scale factor of the returned bitmap.
Giving this, I strongly suspect that the sizes of icons are not computed correctly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 85630bf Use FromDIP()/ToPhys() and fix small button sizing
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -980,6 +982,8 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
const wxBitmapBundle& bundle = disabled ?
m_bundlesSmallDisabled[idx] : m_bundlesSmall[idx];
bitmap_small = bundle.GetBitmap(scaledSmall);
+ if ( bitmap_small.IsOk() )
+ bitmap_small.SetScaleFactor(contentScale);
OK, I reworked it, still looks correct on Retina and HiDPI Windows.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks, this looks good to me now, but I'd rather not apply the change that I commented below. If you don't see any problems with this, I can merge it.
> @@ -944,19 +945,18 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
wxRibbonButtonBarLayout* layout = m_layouts.Item(m_current_layout);
- // Calculate DPI-scaled bitmap sizes (must match layout calculation)
- const double scale = GetDPIScaleFactor();
- const wxSize scaledLarge = m_bitmap_size_large * scale;
- const wxSize scaledSmall = m_bitmap_size_small * scale;
+ // wxBitmapBundle::GetBitmap() expects a size in physical pixels, so convert
+ // the stored DIP sizes all the way to physical pixels (ToPhys() is a no-op
+ // under MSW, while FromDIP() is a no-op under the ports using DPI-independent
+ // pixels, so this gives the right result everywhere).
+ const wxSize scaledLarge = ToPhys(FromDIP(m_bitmap_size_large));
+ const wxSize scaledSmall = ToPhys(FromDIP(m_bitmap_size_small));
for ( auto& button : layout->buttons )
{
wxRibbonButtonBarButtonBase* base = button.base;
As you can see from the diagram here, the new code is exactly equivalent to the old one, so it shouldn't be necessary to change this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -944,19 +945,18 @@ void wxRibbonButtonBar::OnPaint(wxPaintEvent& WXUNUSED(evt))
wxRibbonButtonBarLayout* layout = m_layouts.Item(m_current_layout);
- // Calculate DPI-scaled bitmap sizes (must match layout calculation)
- const double scale = GetDPIScaleFactor();
- const wxSize scaledLarge = m_bitmap_size_large * scale;
- const wxSize scaledSmall = m_bitmap_size_small * scale;
+ // wxBitmapBundle::GetBitmap() expects a size in physical pixels, so convert
+ // the stored DIP sizes all the way to physical pixels (ToPhys() is a no-op
+ // under MSW, while FromDIP() is a no-op under the ports using DPI-independent
+ // pixels, so this gives the right result everywhere).
+ const wxSize scaledLarge = ToPhys(FromDIP(m_bitmap_size_large));
+ const wxSize scaledSmall = ToPhys(FromDIP(m_bitmap_size_small));
for ( auto& button : layout->buttons )
{
wxRibbonButtonBarButtonBase* base = button.base;
sure, sounds good. thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.