Bug 53052 - KIconTheme icon theme matching doesn't use icons that need to be scaled
Summary: KIconTheme icon theme matching doesn't use icons that need to be scaled
Status: RESOLVED FIXED
Alias: None
Product: kdelibs
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: SVN
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: James Richard Tyrer
URL:
Keywords:
: 68625 (view as bug list)
Depends on:
Blocks:
 
Reported: 2003-01-16 09:49 UTC by Alexander Larsson
Modified: 2007-06-01 06:20 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Don't scale icons up unless you have to, but do scale icons. (1.35 KB, patch)
2003-01-16 09:51 UTC, Alexander Larsson
Details
Minor change to fix the problem (1.57 KB, patch)
2007-05-31 22:36 UTC, James Richard Tyrer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Larsson 2003-01-16 09:49:27 UTC
Version:            (using KDE KDE 3.0.99)
Installed from:    Compiled From Sources

If for instance a theme has a 48x48 version of an icon only, and you request a 32x32 version you get the "unknown" icon back instead of a scaled version of the 48x48 icon.

The problem happens when doing the second level matching (KIcon::MatchBest) in KIconTheme::iconPath().

It iterates over all directories doing:

  if (match == KIcon::MatchExact)
  {
     if ((dir->type() == KIcon::Fixed) && (dir->size() != size))
       continue;
     if ((dir->type() == KIcon::Scalable) &&
        ((size < dir->minSize()) || (size > dir->maxSize())))
       continue;
     if ((dir->type() == KIcon::Threshold) &&
       (abs(dir->size()-size) > dir->threshold()))
       continue;
   } else
   {
     dw = dir->size() - size;
     if (dir->type() != KIcon::Threshold &&
        ((dw > 7) || (abs(dw) >= abs(delta))))
       continue;
   }

So, if dir->size() is 48 and size is 32, dw will be 16 and a scaled icon will not be used.
I think this is wrong. A scaled icon is prefered over the unknown icon. I don't know where the magic number 7 comes from, but i suspect it was added to avoid scaling icons up, as that can result in some very ugly icons.

I do think that a better solution to that is to always pick the closest larger icon and scale it down, and only scale icons up if there is no larger one. I'm attaching a patch that implements this.
Comment 1 Alexander Larsson 2003-01-16 09:51:26 UTC
Created attachment 757 [details]
Don't scale icons up unless you have to, but do scale icons.
Comment 2 Zack Rusin 2003-09-14 07:00:48 UTC
Subject: kdelibs/kdecore

CVS commit by zrusin: 

Patch by Alexander Larsson, fixes 53052 "KIconTheme icon matching doesn't find 
icons that need to be scaled". Thanks Alex!

CCMAIL: 53052-done@bugs.kde.org


  M +33 -9     kicontheme.cpp   1.55


--- kdelibs/kdecore/kicontheme.cpp  #1.54:1.55
@@ -360,7 +360,31 @@ KIcon KIconTheme::iconPath(const QString
         } else
         {
+          // dw < 0 means need to scale up to get an icon of the requested size
+          if (dir->type() == KIcon::Fixed)
+          {
             dw = dir->size() - size;
-            if (dir->type() != KIcon::Threshold &&
-               ((dw > 7) || (abs(dw) >= abs(delta))))
+          } else if (dir->type() == KIcon::Scalable)
+          {
+            if (size < dir->minSize())
+              dw = dir->minSize() - size;
+            else if (size > dir->maxSize())
+              dw = dir->maxSize() - size;
+            else
+              dw = 0;
+          } else if (dir->type() == KIcon::Threshold)
+          {
+            if (size < dir->size() - dir->threshold())
+              dw = dir->size() - dir->threshold() - size;
+            else if (size > dir->size() + dir->threshold())
+              dw = dir->size() + dir->threshold() - size;
+            else
+              dw = 0;
+          }
+          /* Skip this if we've found a closer one, unless
+             it's a downscale, and we only had upscales befores.
+             This is to avoid scaling up unless we have to,
+             since that looks very ugly */
+          if ((abs(dw) >= abs(delta)) &&
+              !(delta > 0 && dw < 0))
                 continue;
         }


Comment 3 James Richard Tyrer 2003-12-29 08:02:17 UTC
Please see new BUG #68625

This is not a duplicate but a very similar problem.

--
JRT
Comment 4 James Richard Tyrer 2004-06-27 05:34:48 UTC
Why was this closed???

It has NOT been fixed!!!

UPDATE:

I still have this problem with KDE-3.2.3.

This bug has been partially fixed in 3.3 ALPHA 1, but there are still problems.

Test case:

Select Icon Theme of KDE Classic.

In Konqueror, show the BookMark ToolBar with at least one folder on it.

Select Icon Size of Medium (22x22) for the BookMark ToolBar.

Then the icons shown is Crystal SVG rather than KDE Classic.

This is just an instance of what is a general design problem.

--
JRT
Comment 5 James Richard Tyrer 2005-10-26 21:16:04 UTC
To provide additional information:

http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html
<<
The lookup is done first in the current theme, and then recursively in each of the current theme's parents, and finally in the default theme called "hicolor" (implementations may add more default themes before "hicolor", but "hicolor" must be last). As soon as there is an icon of any size that matches in a theme, the search is stopped. Even if there may be an icon with a size closer to the correct one in an inherited theme, we don't want to use it. Doing so may generate an inconsistant change in an icon when you change icon sizes (e.g. zoom in).
>>

This seems very clear to me.  To make this very clear, KDE does not do this -- it does not follow the spec.  If you are using KDEClassic and a size is missing, you get a CrystalSVG icon not a scaled KDEClassic icon.

The first fix for this bug caused a new bug which now needs to be fixed.
Comment 6 James Richard Tyrer 2005-10-27 03:53:44 UTC
*** Bug 68625 has been marked as a duplicate of this bug. ***
Comment 7 James Richard Tyrer 2007-05-31 22:36:08 UTC
Created attachment 20748 [details]
Minor change to fix the problem

This appears to fix the problem.
Comment 8 James Richard Tyrer 2007-06-01 06:20:19 UTC
SVN commit 670335 by tyrerj:

BUG:53052

 M  +5 -12     kiconloader.cpp  


--- branches/KDE/3.5/kdelibs/kdecore/kiconloader.cpp #670334:670335
@@ -460,19 +460,17 @@
     static const QString &xpm_ext = KGlobal::staticQString(".xpm");
     ext[count++]=&xpm_ext;
 
-    /* antlarr: Multiple inheritance is a broken concept on icon themes, so
-       the next code doesn't support it on purpose because in fact, it was
-       never supported at all. This makes the order in which we look for an
-       icon as:
+    /* JRT: To follow the XDG spec, the order in which we look for an
+       icon 1s:
 
        png, svgz, svg, xpm exact match
+       png, svgz, svg, xpm best match
        next theme in inheritance tree : png, svgz, svg, xpm exact match
+                                        png, svgz, svg, xpm best match
        next theme in inheritance tree : png, svgz, svg, xpm exact match
+                                        png, svgz, svg, xpm best match
        and so on
 
-       And if the icon couldn't be found then it tries best match in the same
-       order.
-
        */
     for ( KIconThemeNode *themeNode = d->links.first() ; themeNode ;
 	themeNode = d->links.next() )
@@ -484,11 +482,6 @@
 		return icon;
 	}
 
-    }
-
-    for ( KIconThemeNode *themeNode = d->links.first() ; themeNode ;
-	themeNode = d->links.next() )
-    {
 	for (int i = 0 ; i < count ; i++)
 	{
 	    icon = themeNode->theme->iconPath(name + *ext[i], size, KIcon::MatchBest);