From 907e4ea8621d8c68138f880928a2bc032232c574 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jun 2022 00:16:58 +0100 Subject: [PATCH 1/3] Add wxBitmapBundle::GetConsensusSizeFor(double) and test it Add an overload of the existing function which can be easily tested in the unit tests and add a trivial new test for it. --- include/wx/bmpbndl.h | 6 +++++- src/common/bmpbndl.cpp | 10 +++++++++- tests/graphics/bmpbundle.cpp | 31 +++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/include/wx/bmpbndl.h b/include/wx/bmpbndl.h index 1b137e588a..ddcba2930d 100644 --- a/include/wx/bmpbndl.h +++ b/include/wx/bmpbndl.h @@ -152,7 +152,11 @@ public: // Implementation only from now on. // Get the bitmap size preferred by the majority of the elements of the - // bundles at the scale appropriate for the given scale. + // bundles at the given scale or the scale appropriate for the given window. + static wxSize + GetConsensusSizeFor(double scale, + const wxVector& bundles, + const wxSize& sizeDefault); static wxSize GetConsensusSizeFor(wxWindow* win, const wxVector& bundles, diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 3316c0b14e..fb02842521 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -596,7 +596,15 @@ wxBitmapBundle::GetConsensusSizeFor(wxWindow* win, const wxVector& bundles, const wxSize& sizeDefault) { - const double scale = win->GetDPIScaleFactor(); + return GetConsensusSizeFor(win->GetDPIScaleFactor(), bundles, sizeDefault); +} + +/* static */ +wxSize +wxBitmapBundle::GetConsensusSizeFor(double scale, + const wxVector& bundles, + const wxSize& sizeDefault) +{ const wxSize sizeIdeal = sizeDefault*scale; // We want to use preferred bitmap size, but the preferred sizes can be diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index c90e022e6d..dbb2e9a50e 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -460,3 +460,34 @@ TEST_CASE("BitmapBundle::Scale", "[bmpbundle][scale]") } #endif // ports with scaled bitmaps support + +TEST_CASE("BitmapBundle::GetConsensusSize", "[bmpbundle]") +{ + // Just a trivial helper to make writing the tests below simpler. + struct Bundles + { + wxVector vec; + + void Add(int size) + { + vec.push_back(wxBitmapBundle::FromBitmap(wxSize(size, size))); + } + + int GetConsensusSize(double scale) const + { + return wxBitmapBundle::GetConsensusSizeFor(scale, vec, wxSize()).y; + } + } bundles; + + // When there is a tie, a larger size is chosen by default. + bundles.Add(16); + bundles.Add(24); + CHECK( bundles.GetConsensusSize(2) == 48 ); + + // Breaking the tie results in the smaller size winning now. + bundles.Add(16); + CHECK( bundles.GetConsensusSize(2) == 32 ); + + // Integer scaling factors should be preferred. + CHECK( bundles.GetConsensusSize(1.5) == 16 ); +} From e13b4f88335664ea3d0a7a25efbe05c7820c171e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jun 2022 01:52:53 +0100 Subject: [PATCH 2/3] Don't force fractional scale when toolbar bitmap size is given The old code in wxToolBarBase::AdjustToolBitmapSize() forced the use of the exact value of the requested bitmap size multiplied by the current scale factor, which resulted in ugly bitmaps whenever fractional scaling factor was used. It also used not immediately clear IncTo() call. Simplify and improve it by handling the cases when we have a requested bitmap size and we don't have it differently: if we do have it, just use it directly, but only with an integer scale factor. And if we don't, then simply use the bitmap size suitable for the current scale factor. This seems to result in the most expected behaviour and, notably, doesn't break the toolbar sample where the bitmap size can still be toggled between small and big bitmaps on both normal and high DPI monitors. Also update the documentation: still recommend not to use SetToolBitmapSize() at all, but don't claim that it forces fractional scaling, as this is not the case any longer. --- docs/doxygen/overviews/high_dpi.md | 3 ++- interface/wx/toolbar.h | 13 +++++------ src/common/tbarbase.cpp | 35 ++++++++++++++++++++---------- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/docs/doxygen/overviews/high_dpi.md b/docs/doxygen/overviews/high_dpi.md index ffc6e8fed0..58960a1a09 100644 --- a/docs/doxygen/overviews/high_dpi.md +++ b/docs/doxygen/overviews/high_dpi.md @@ -299,7 +299,8 @@ programs involves doing at least the following: wxArtProvider::CreateBitmapBundle() (and, of course, return multiple bitmaps from it) instead of wxArtProvider::CreateBitmap(). 4. Removing any calls to wxToolBar::SetToolBitmapSize() or, equivalently, - `` attributes from the XRC, as this forces unwanted scaling. + `` attributes from the XRC, as this overrides the best size + determination and may result in suboptimal appearance. diff --git a/interface/wx/toolbar.h b/interface/wx/toolbar.h index c021c1ec56..d67c92b178 100644 --- a/interface/wx/toolbar.h +++ b/interface/wx/toolbar.h @@ -882,15 +882,15 @@ public: It is usually unnecessary to call this function, as the tools will always be made big enough to fit the size of the bitmaps used in them. - Moreover, calling it may force wxToolBar to scale its images, even - using non-integer scaling factor, which will usually look bad, instead - of adapting the image size to the current DPI scaling in order to avoid - doing this. + Moreover, calling it forces wxToolBar to scale its images in high DPI + using the provided size, instead of letting wxBitmapBundle used for the + tool bitmaps determine the best suitable bitmap size, which may result + in suboptimal appearance. If you do call it, it must be done before toolbar is Realize()'d. Example of using this function to force the bitmaps to be at least - 32 pixels wide and tall: + 32 pixels wide and tall (at normal DPI): @code toolbar->SetToolBitmapSize(FromDIP(wxSize(32, 32))); toolbar->AddTool(wxID_NEW, "New", wxBitmapBundle::FromXXX(...)); @@ -898,9 +898,6 @@ public: toolbar->Realize(); @endcode - Note that this example would scale bitmaps to 48 pixels when using 150% - DPI scaling, which wouldn't happen without calling SetToolBitmapSize(). - @param size The size of the bitmaps in the toolbar in logical pixels. diff --git a/src/common/tbarbase.cpp b/src/common/tbarbase.cpp index 1aaf478555..673022012a 100644 --- a/src/common/tbarbase.cpp +++ b/src/common/tbarbase.cpp @@ -475,7 +475,23 @@ void wxToolBarBase::AdjustToolBitmapSize() bundles.push_back(bmp); } - if ( !bundles.empty() ) + if ( bundles.empty() ) + return; + + wxSize sizeNeeded; + + if ( m_requestedBitmapSize != wxSize(0, 0) ) + { + // If we have a fixed requested bitmap size, use it, but scale it by + // integer factor only, as otherwise we'd force fractional (and hence + // ugly looking) scaling here whenever fractional DPI scaling is used. + + // We want to round 1.5 down to 1, but 1.75 up to 2. + int scaleFactorRoundedDown = + static_cast(ceil(2*GetDPIScaleFactor())) / 2; + sizeNeeded = m_requestedBitmapSize*scaleFactorRoundedDown; + } + else // Determine the best size to use from the bitmaps we have. { wxSize sizePreferred = wxBitmapBundle::GetConsensusSizeFor ( @@ -492,22 +508,17 @@ void wxToolBarBase::AdjustToolBitmapSize() // scale factor may be different from 1) use this size at all // currently, so it shouldn't matter. But if/when they are modified to // use the size computed here, this would need to be revisited. - sizePreferred = FromPhys(sizePreferred); - - // Don't decrease the bitmap below the size requested by the application - // as using larger bitmaps shouldn't shrink them to the small default - // size. - sizePreferred.IncTo(FromDIP(m_requestedBitmapSize)); - - // No need to change the bitmaps size if it doesn't really change. - if ( sizePreferred == sizeOrig ) - return; + sizeNeeded = FromPhys(sizePreferred); + } + // No need to change the bitmaps size if it doesn't really change. + if ( sizeNeeded != sizeOrig ) + { // Call DoSetToolBitmapSize() and not SetToolBitmapSize() to avoid // changing the requested bitmap size: if we set our own adjusted size // as the preferred one, we wouldn't decrease it later even if we ought // to, as when moving from a monitor with higher DPI to a lower-DPI one. - DoSetToolBitmapSize(sizePreferred); + DoSetToolBitmapSize(sizeNeeded); } } From 72f851f6f46b10dbd1ee0da3e48d39689135d2da Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jun 2022 02:02:56 +0100 Subject: [PATCH 3/3] Remove the size parameter of wxBitmapBundle::GetConsensusSizeFor() It doesn't seem to be useful and wasn't really specified in 2 out of 3 existing calls to this function and was probably wrongly specified in the remaining one, so just remove it for now, it can always be added later if we decide what exactly should it do. --- include/wx/bmpbndl.h | 8 ++----- src/common/bmpbndl.cpp | 43 +++++++++--------------------------- src/common/bmpcboxcmn.cpp | 3 +-- src/common/tbarbase.cpp | 8 ++----- tests/graphics/bmpbundle.cpp | 2 +- 5 files changed, 17 insertions(+), 47 deletions(-) diff --git a/include/wx/bmpbndl.h b/include/wx/bmpbndl.h index ddcba2930d..dabbfa8d2d 100644 --- a/include/wx/bmpbndl.h +++ b/include/wx/bmpbndl.h @@ -154,13 +154,9 @@ public: // Get the bitmap size preferred by the majority of the elements of the // bundles at the given scale or the scale appropriate for the given window. static wxSize - GetConsensusSizeFor(double scale, - const wxVector& bundles, - const wxSize& sizeDefault); + GetConsensusSizeFor(double scale, const wxVector& bundles); static wxSize - GetConsensusSizeFor(wxWindow* win, - const wxVector& bundles, - const wxSize& sizeDefault); + GetConsensusSizeFor(wxWindow* win, const wxVector& bundles); // Create wxImageList and fill it with the images from the given bundles in // the sizes appropriate for the DPI scaling used for the specified window. diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index fb02842521..a15b004b65 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -593,20 +593,16 @@ void RecordSizePref(SizePrefs& prefs, const wxSize& size) /* static */ wxSize wxBitmapBundle::GetConsensusSizeFor(wxWindow* win, - const wxVector& bundles, - const wxSize& sizeDefault) + const wxVector& bundles) { - return GetConsensusSizeFor(win->GetDPIScaleFactor(), bundles, sizeDefault); + return GetConsensusSizeFor(win->GetDPIScaleFactor(), bundles); } /* static */ wxSize wxBitmapBundle::GetConsensusSizeFor(double scale, - const wxVector& bundles, - const wxSize& sizeDefault) + const wxVector& bundles) { - const wxSize sizeIdeal = sizeDefault*scale; - // We want to use preferred bitmap size, but the preferred sizes can be // different for different bitmap bundles, so record all their preferences // first. @@ -631,25 +627,13 @@ wxBitmapBundle::GetConsensusSizeFor(double scale, } else if ( countThis == countMax ) { - // We have a tie between different sizes, choose the one - // corresponding to the current scale factor, if possible, as this - // is the ideal bitmap size that should be consistent with all the - // other bitmaps. - if ( sizePreferred != sizeIdeal ) - { - if ( sizeThis == sizeIdeal ) - { - sizePreferred = sizeThis; - } - else // Neither of the sizes is the ideal one. - { - // Choose the larger one as like this some bitmaps will be - // downscaled, which should look better than upscaling some - // (other) ones. - if ( sizeThis.y > sizePreferred.y ) - sizePreferred = sizeThis; - } - } + // We have a tie between different sizes. + + // Choose the larger one as like this some bitmaps will be + // downscaled, which should look better than upscaling some + // (other) ones. + if ( sizeThis.y > sizePreferred.y ) + sizePreferred = sizeThis; } } @@ -664,12 +648,7 @@ wxBitmapBundle::CreateImageList(wxWindow* win, wxCHECK_MSG( win, NULL, "must have a valid window" ); wxCHECK_MSG( !bundles.empty(), NULL, "should have some images" ); - // We arbitrarily choose the default size of the first bundle as the - // default size for the image list too, as it's not clear what else could - // we do here. Note that this size is only used to break the tie in case - // the same number of bundles prefer two different sizes, so it's not going - // to matter at all in most cases. - wxSize size = GetConsensusSizeFor(win, bundles, bundles[0].GetDefaultSize()); + wxSize size = GetConsensusSizeFor(win, bundles); // wxImageList wants the logical size for the platforms where logical and // physical pixels are different. diff --git a/src/common/bmpcboxcmn.cpp b/src/common/bmpcboxcmn.cpp index db4e608e7d..73023ddd79 100644 --- a/src/common/bmpcboxcmn.cpp +++ b/src/common/bmpcboxcmn.cpp @@ -72,8 +72,7 @@ void wxBitmapComboBoxBase::UpdateInternals() m_usedImgSize = wxBitmapBundle::GetConsensusSizeFor ( GetControl(), - m_bitmapbundles, - wxSize(0, 0) + m_bitmapbundles ); } } diff --git a/src/common/tbarbase.cpp b/src/common/tbarbase.cpp index 673022012a..72b022812c 100644 --- a/src/common/tbarbase.cpp +++ b/src/common/tbarbase.cpp @@ -493,12 +493,8 @@ void wxToolBarBase::AdjustToolBitmapSize() } else // Determine the best size to use from the bitmaps we have. { - wxSize sizePreferred = wxBitmapBundle::GetConsensusSizeFor - ( - this, - bundles, - ToDIP(sizeOrig) - ); + const wxSize + sizePreferred = wxBitmapBundle::GetConsensusSizeFor(this, bundles); // GetConsensusSizeFor() returns physical size, but we want to operate // with logical pixels as everything else is expressed in them. diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index dbb2e9a50e..810ed9f61d 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -475,7 +475,7 @@ TEST_CASE("BitmapBundle::GetConsensusSize", "[bmpbundle]") int GetConsensusSize(double scale) const { - return wxBitmapBundle::GetConsensusSizeFor(scale, vec, wxSize()).y; + return wxBitmapBundle::GetConsensusSizeFor(scale, vec).y; } } bundles;