Differential D12509

Fix preview overlay for small custom folder icons
AbandonedPublic
Actions

Authored by muhlenpfordt on Apr 25 2018, 7:00 AM.

Details

Reviewers
None
Group Reviewers
Gwenview
Summary

If a custom folder icon is too small (128 or 256 px, depends on Browse
Mode zoom level) the preview is created but immediately repainted with
the base icon. This is caused by KIO::PreviewJob returning only
down- but not upscaled icons.
This patch scales the preview thumbnail to the requested size if it's
too small, analogous to KIconLoader::loadIcon() which loads the
base icon.

CCBUG: 315983
FIXED-IN: 18.04.1
Depends on D12465

Test Plan
  • Set a custom icon (size < 128 px) for a folder containing some images
  • Start Gwenview in Browse Mode in the parent folder
  • Folder should always view with preview overlay
  • Try with different zoom levels
Before:After:

Diff Detail

Repository
R260 Gwenview
Branch
small-folder-icon (branched from Applications/18.04)
Lint
Lint OK
Unit
No Unit Test Coverage
muhlenpfordt requested review of this revision.Apr 25 2018, 7:00 AM
muhlenpfordt created this revision.
rkflx added a subscriber: rkflx.Apr 25 2018, 10:59 AM

Doesn't your approach mean that now the thumbnail quality will be worse for absolute paths?

Left: pattern-gnome

Right: /usr/share/icons/oxygen/64x64/apps/pattern-gnome.png (which is the maximum size, i.e. what Gwenview would pick when only the name is specified and a large zoom level is chosen)

muhlenpfordt added a comment.Apr 25 2018, 11:45 AM
In D12509#253527, @rkflx wrote:

Doesn't your approach mean that now the thumbnail quality will be worse for absolute paths?

Hm, not sure what you expect if an absolute path is given?
If an explicit file is referenced as folder icon, exactly this one is loaded.
Using only a name without path KIO::PreviewJob selects the best for the requested size.

rkflx added a comment.Apr 25 2018, 11:57 AM
In D12509#253548, @muhlenpfordt wrote:
In D12509#253527, @rkflx wrote:

Doesn't your approach mean that now the thumbnail quality will be worse for absolute paths?

Hm, not sure what you expect if an absolute path is given?
If an explicit file is referenced as folder icon, exactly this one is loaded.
Using only a name without path KIO::PreviewJob selects the best for the requested size.

Ah, maybe I used unclear terminology for "thumbnail". My point was that for both the relative and the absolute patch the same icon is chosen (i.e. the GNOME foot) and rendered in the same quality, but in one case the preview thumbnail of the image painted over the icon is pixelated, while it is fine for the other case.

The ingredients are the same: Same image to show a thumbnail for, same folder icon. Only in one case the thumbnail is not rendered optimally.

muhlenpfordt added a comment.Apr 25 2018, 12:19 PM
In D12509#253558, @rkflx wrote:

Ah, maybe I used unclear terminology for "thumbnail". My point was that for both the relative and the absolute patch the same icon is chosen (i.e. the GNOME foot) and rendered in the same quality, but in one case the preview thumbnail of the image painted over the icon is pixelated, while it is fine for the other case.

The ingredients are the same: Same image to show a thumbnail for, same folder icon. Only in one case the thumbnail is not rendered optimally.

Ok, I think I got it now. ;)

The base icon is not the same for both thumbnails. PreviewJob loads the 128 or 256 pixel version for the left thumbnail and as given the 64 pixel version for the right one. Each of these icons are overlayed with the mini thumbnails. Since PreviewJob does not scale the base icon by itself to the requested size the mini thumbnails are also smaller.

rkflx added a comment.EditedApr 25 2018, 12:24 PM
In D12509#253567, @muhlenpfordt wrote:

The base icon is not the same for both thumbnails. PreviewJob loads the 128 or 256 pixel version for the left thumbnail and as given the 64 pixel version for the right one.

The maximum size available on all of / for that icon is 64px. Despite that, Gwenview is able to show a good looking thumbnail in one case, but not the other.

Each of these icons are overlayed with the mini thumbnails. Since PreviewJob does not scale the base icon by itself to the requested size the mini thumbnails are also smaller.

We seem to have two different scaling code paths:

  • Left: Scale 64px foot up, overlay with thumbnail, get sharp result.
  • Right: Overlay 64px foot with thumbnail, scale both up, get pixelated result.

Edit: The first code path was pre-existing, the second code path is the one you added. IOW, my gut feeling is that the bug should be fixed in a different place than you are trying to currently.

muhlenpfordt added a comment.Apr 25 2018, 12:45 PM
In D12509#253568, @rkflx wrote:

The maximum size available on all of / for that icon is 64px. Despite that, Gwenview is able to show a good looking thumbnail in one case, but not the other.

Tested with an icon that exists only at 64 px and got the same result now. ;)

  • Left: Scale 64px foot up, overlay with thumbnail, get sharp result.
  • Right: Overlay 64px foot with thumbnail, scale both up, get pixelated result.

Actually PreviewJob scales the base icon by itself for non absolute paths, overlays only after that and returns a thumbnail of the requested size.
Not scaling for absolute paths icons really sounds like a bug inside the library.

muhlenpfordt added a comment.Apr 27 2018, 6:28 AM

I digged a little deeper in the libraries... ;)
The base icon is loaded in kio-extras function ThumbnailProtocol::thumbForDirectory().
It uses QIcon::fromTheme() to find an icon and converts this to a QPixmap.
The description for the conversion function pixmap() says:

The pixmap might be smaller than requested, but never larger.

I don't know why non absolute path icons are upscaled anyway but the docu states we can't count on the requested size.
IMO thumbForDirectory() should check and scale this base icon if needed. This will result in a upscaled base icon but higher quality overlay items.
Do you think this is the right way? Should I adapt this patch for kio-extras?

rkflx added a comment.May 5 2018, 1:43 PM

Spent quite some time on this, and it seems fixing it in kio-extras is the best (if not our only) option.

The following patch fixes it in Gwenview as well as in Dolphin for me:

P204 Improve preview overlay for low-res folder icons
1diff --git a/thumbnail/thumbnail.cpp b/thumbnail/thumbnail.cpp
2index 502b2d17..bc3f6aa3 100644
3--- a/thumbnail/thumbnail.cpp
4+++ b/thumbnail/thumbnail.cpp
5@@ -467,11 +467,17 @@ QImage ThumbnailProtocol::thumbForDirectory(const QUrl& directory)
6 QString localFile = directory.path();
7
8 KFileItem item(QUrl::fromLocalFile(localFile));
9- const QPixmap folder = QIcon::fromTheme(item.iconName()).pixmap(qMin(m_width, m_height));
10+ const int extent = qMin(m_width, m_height);
11+ QPixmap folder = QIcon::fromTheme(item.iconName()).pixmap(extent);
12+
13+ // Scale up base image to ensure overlays are rendered with
14+ // the best quality possible even for low-res custom folder icons
15+ if (folder.width() < extent || folder.height() < extent) {
16+ folder = folder.scaled(extent, extent, Qt::KeepAspectRatio, Qt::SmoothTransformation);
17+ }
18
19 const int folderWidth = folder.width();
20 const int folderHeight = folder.height();
21-
22 const int topMargin = folderHeight * 30 / 100;
23 const int bottomMargin = folderHeight / 6;
24 const int leftMargin = folderWidth / 13;

Before:

After:

TL;DR: Yes, I think your idea is the right way.

Revision Contents
Changeset List

Diff 33037

View Options

lib/thumbnailprovider/thumbnailprovider.cpp

Loading...