Re: [lmi] Showing better icons in high DPI

[Vadim Zeitlin]

On Thu, 17 Feb 2022 00:42:13 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> Okay, so that would constitute useful incremental progress: the text
GC> is much more legible. Text rendering is important; icons, not so much.
GC> The second screenshot, modified to magnify the icons (however crudely),
GC> is optimal: you've improved the text clarity, and costly further
GC> changes to retouch the icons don't seem worth the effort.

 I've implemented this behaviour now, please see the attached screenshots.
It required only trivial changes to icon_monger, in fact I spent most of
the time updating the comment explaining this change rather than on the
code itself. Here is the full patch (not to be applied yet, as it requires
the latest wx, just FYI):

---------------------------------- >8 --------------------------------------
diff --git a/icon_monger.cpp b/icon_monger.cpp
index 5eb70fe89..eb382e8ab 100644
--- a/icon_monger.cpp
+++ b/icon_monger.cpp
@@ -126,9 +126,30 @@
 }
 } // Unnamed namespace.
 
-/// Provide the most suitable icon in the given context.
+/// Create a bitmap bundle, capable of representing the same bitmap at multiple
+/// resolutions from a wxArtID value. In our case we only use a single bitmap
+/// resolution currently, but it's still preferable to override this function
+/// rather than CreateBitmap() for the following reasons:
+///
+///  - We can return the bitmap of any size, without bothering to rescale it
+///    ourselves and wxBitmapBundle will take care of actually resizing it if
+///    necessary.
+///  - We can keep the existing check for the bitmap having the expected size
+///    as simple as it was, whereas with CreateBitmap() we would have to relax
+///    it to avoid warnings when a scaled version of the bitmap is requested,
+///    which is difficult to do and so this check would probably have to be
+///    simply sacrificed entirely.
+///  - Overriding CreateBitmapBundle() is marginally more efficient, as it is
+///    called first, with CreateBitmap() only used as a fallback.
+///  - It will be easier to extend this version in the future by returning
+///    multiple bitmaps, e.g. creating a bundle from multiple PNGs or an SVG.
+///
+/// But currently we use only a single file, by finding the file corresponding
+/// to the given icon name.
+///
+/// Note that builtin wxArtID values are converted to the corresponding names,
+/// i.e.
 ///
-/// Convert builtin wxArtID values to fitting filenames, e.g.:
 ///   wxART_FOO_BAR --> foo-bar.png    [default size]
 ///   wxART_FOO_BAR --> foo-bar-16.png [16 pixels square]
 ///
@@ -140,7 +161,7 @@
 /// Diagnosed failures are presented merely as warnings because they
 /// do not make the system impossible to use.
 
-wxBitmap icon_monger::CreateBitmap
+wxBitmapBundle icon_monger::CreateBitmapBundle
     (wxArtID const&     id
     ,wxArtClient const& client
     ,wxSize const&      size
@@ -155,7 +176,7 @@
             {
             // This is not an error as not all wxART id's have lmi overrides,
             // so just let the next art provider deal with it.
-            return wxNullBitmap;
+            return wxBitmapBundle{};
             }
         icon_name = i->second;
         }
@@ -207,7 +228,7 @@
             }
         }
 
-    wxImage image(icon_path.string().c_str(), wxBITMAP_TYPE_PNG);
+    wxBitmap image(icon_path.string().c_str(), wxBITMAP_TYPE_PNG);
     if(!image.IsOk())
         {
         warning()
@@ -219,12 +240,6 @@
         return wxNullBitmap;
         }
 
-    // WX !! Remove this little block when wx does this automatically.
-    if(image.HasMask())
-        {
-        image.InitAlpha();
-        }
-
     if(desired_size != wxSize(image.GetWidth(), image.GetHeight()))
         {
         warning()
@@ -234,21 +249,18 @@
             << image.GetWidth()
             << " by "
             << image.GetHeight()
-            << " has been scaled because no bitmap of requested size "
+            << " may be scaled because no bitmap of requested size "
             << desired_size.GetWidth()
             << " by "
             << desired_size.GetHeight()
             << " was found."
             << LMI_FLUSH
             ;
-        image.Rescale
-            (desired_size.GetWidth()
-            ,desired_size.GetHeight()
-            ,wxIMAGE_QUALITY_HIGH
-            );
+        // Note that there is no need to actually rescale the image, this will
+        // be done by wxBitmapBundle itself automatically if needed.
         }
 
-    return wxBitmap(image);
+    return wxBitmapBundle(image);
 }
 
 /// Load the image from the given file.
diff --git a/icon_monger.hpp b/icon_monger.hpp
index 7675bf21d..66f91be05 100644
--- a/icon_monger.hpp
+++ b/icon_monger.hpp
@@ -44,8 +44,8 @@ class icon_monger
     icon_monger(icon_monger const&) = delete;
     icon_monger& operator=(icon_monger const&) = delete;
 
-    // wxArtProvider required implementation.
-    wxBitmap CreateBitmap
+    // Implement wxArtProvider function.
+    wxBitmapBundle CreateBitmapBundle
         (wxArtID const&
         ,wxArtClient const&
         ,wxSize const&
---------------------------------- >8 --------------------------------------

As I tried to explain, we could also keep overriding GetBitmap(), but IMO
this would result in more complex code, at least if we wanted to keep the
check for scaling, so I've decided to do it like this instead, even if it
might seem to not make much sense to use a wxBitmapBundle for just a single
bitmap.

 Also, I've replaced the use of wxImage with wxBitmap just because we don't
need wxImage any more. We could keep using it, but this would just do extra
conversions behind the scene, so I've decided to get rid of them.


GC> > Using SVGs also has the extra advantage of allowing to adapt the icon
GC> > sizes exactly to the expected size, even when using 150% or 250% or
GC> > other weird DPI scaling
[...]
GC> That's a valid point, but it doesn't change the cost-benefit tradeoff.

 Just to confirm, with my changes above the icons are shown at the same
(double) size at 150% DPI, i.e. bigger than would be expected and also at
the same (normal) size at 125% DPI, i.e. smaller than expected. I don't
really know how bothersome it is going to be, or not be, in practice. I
suspect it shouldn't be too bad because lmi toolbar bitmaps are 24x24 and
nobody has ever really objected to using 32x32 bitmaps in the toolbar at
standard DPI, so using 48x48 bitmaps rather than 36x36 ones with 150%
scaling shouldn't hopefully be a big problem neither.

 For me, personally, it's the bitmap blurriness which looks ugly. But
unless you dramatically change your mind, we'll just have to live with it
and I won't return to this again.

GC> If I never edit another icon as long as I live, that'll be too soon.

 I do still wonder about whether we should at least allow using new icons
in bigger sizes. I.e. suppose we want to add another toolbar icon --
wouldn't it be nice to be able to use 48x48 size for it?

 Anyhow, please let me know what do you think of the current version.

 Thanks in advance,
VZ

pgpnTGYlVg5on.pgp
Description: PGP signature