D15071: Don't draw frames and shadows around images with an alpha channel

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Frameworks, Dolphin, VDG, broulik, cfeck.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

REVISION SUMMARY
This patch improves and simplifies the criteria for whether or not to draw a frame and a shadow around an image's thumbnail. The old size-based detection code was unreliable and gave false positives as well as false negatives. It is replaced with a simple check for whether the image has an alpha channel, which not only automatically matches all icon files, but also non-icon raster images with transparency, which look better without the frame.

BUG: 258514
FIXED-IN: 5.50

TEST PLAN
Verified unchanged:

- SVG icons still have no frame:

- JPEG and PNG images without transparency still have a frame:

Verified changed:

- Non-icon SVG and PNG images with transparency lose their frames. The difference is especially welcome for the cases where the previous set of criteris caused many false positives for folders full of mixed image formats and sizes. For example:

Before:

After:

Before:

After:

Before:

After:

REPOSITORY
R241 KIO

BRANCH
thumbnail-frame-refinement (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D15071

AFFECTED FILES
src/filewidgets/kfilepreviewgenerator.cpp

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

[Stefan Brüns]

bruns added a comment.


I think this is welcome.
Nate, can you prepare a pair of screenshots where you have one icon per type (SVG, PNG, PNG+alpha, JPEG, ...), giving each file a speaking name?

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

[Andres Betts]

abetts added a comment.


I support this 100%

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a subscriber: markg.
ngraham added a comment.


@bruns, done.

@broulik, @markg I too was concerned about the case of images that have an alpha channel but not actual transparency. I did not actually have any images on my computer that had an alpha channel but no transparency, so I made one with GIMP: I took a PNG with an alpha channel and transparency and I cropped it to remove all the transparency, but I left the alpha channel. I then made another version of the same image that has no alpha channel. Both are correctly seen as not having any transparency and given a frame. See the two top-left images:

F6215000: Alpha but no transparency.png <https://phabricator.kde.org/F6215000>

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham retitled this revision from "Don't draw frames and shadows around images with an alpha channel" to "Don't draw frames and shadows around images with transparency".

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Mark Gaiser]

markg added a comment.


(repost from D15069 <https://phabricator.kde.org/D15069>)

Hi Nate,

I'm afraid this will give inconsistent frames, at least that will be the perception of the user.
The hasAlpha function on a QPixmap (probably) boils down to executing this function:

bool QX11PlatformPixmap::hasAlphaChannel() const
{
if (picture && d == 32)
return true;

if (x11_mask && d == 1)
return true;

return false;
}

But there are quite some places in Qt where the alpha channel checks are done differently.The above would be for X11, it's likely different on wayland, etc...

So where is this going to be inconsistent? Well, with images that "have" an alpha channel but don't use it.
This happens when saving an image. It's often a setting to keep transparency or not (it is in photoshop and gimp).
Therefore you can - and will - have people that have images with an alpha channel but don't use it so they don't get a frame. While the very same image when saved differently (and in png as well) will have a frame. That's going to be a bug report from that user i guess ;)

Having said that, i'm much more in favor of an all-or-nothing approach. Not some logic somewhere that dictates when an image has a frame and when it doesn't.
We've had frames for years so perhaps it's time to just flip the switch and see how users like it. So just flip that switch by default to no frames.

- end of previous comment.

As @ngraham figured out, Qt apparently does some more in depth transparency checking. I wonder if it does a pixel-by-pixel test, that would be expensive.
Anyhow, i'd just say - to be consistent - to flip the switch. No frames at all anywhere. It looks just weird to me to have folders where some images have frames and some don't.
For users it will be weird as well, they might even consider it a bug and report it.

It would be a whole different story if the frames were part of a larger area (so including the filename), kinda like this <Loading Image...>. But it isn't. It's merely directly around the image.
It probably (assumption) was a way to imitate Finder (of macOS) and the old "Windows Live Photo Gallery" specially the later one has a nearly 1-on-1 matching with the frame Dolphin draws.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a comment.


@markg I'm in favor of keeping the conditional logic for the frames. If we think of this from an aesthetic point of view, we should draw frames and shadows around images that actually look better as a result. Those would be square and rectangular images with no transparency, which are pretty common for typical users. Images with transparency don't look good with the frames and shadows, so this patch turns them off. I don't think the inconsistency will bother people. On the contrary, the unnecessary *consistency* is what's bothering some people! :)

I suspect you're right that originally, this feature was an attempt to mimic macOS Finder. Finder IMHO does a much worse job than we currently do or that we would do with this patch: it currently puts a frame around every image file unconditionally. It makes no attempt to detect icon files that look better with no frame, and it suffers from the "double frame" issue for window screenshots that include a shadow. I think if we land this patch and D15069 <https://phabricator.kde.org/D15069>, our file dialog and Dolphin will have a better behavior. :)

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Kai Uwe Broulik]

broulik added a comment.


I'm a bit concerned that removing the frame most of the time blurs the lines between "previews of images" and "actual file icons", especially for the SVG case.

Now they all look like proper file icons rather than previews of the file's contents. I can see that an EXE "thumbnail" is a proper file icon but in many other cases it's misleading.

Why did the `ThumbCreator` frame flag get deprecated in the first place?

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a comment.

Post by Kai Uwe Broulik
I'm a bit concerned that removing the frame most of the time blurs the lines between "previews of images" and "actual file icons", especially for the SVG case.

Thing is, this is what users have been clamoring for, especially for SVG icons. In fact, the status quo even has logic to detect them and not draw frames for them. It just doesn't work very well.

Post by Kai Uwe Broulik
Now they all look like proper file icons rather than previews of the file's contents. I can see that an EXE "thumbnail" is a proper file icon but in many other cases it's misleading.

What's misleading exactly?

Post by Kai Uwe Broulik
Why did the `ThumbCreator` frame flag get deprecated in the first place?

No idea...

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Anthony Fieroni]

anthonyfieroni added a comment.

Post by Kai Uwe Broulik
Why did the `ThumbCreator` frame flag get deprecated in the first place?

https://git.reviewboard.kde.org/r/129921/ maybe because it's deprecated before thumbnail got fixed

INLINE COMMENTS

Post by Kai Uwe Broulik
kfilepreviewgenerator.cpp:945
(maxSize.height() > KIconLoader::SizeSmallMedium) &&
- !isIconCandidate;
+ !icon.hasAlpha();
if (!applyFrame) {

Maybe you should add case when it has an alpha but it's nulll.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Stefan Brüns]

bruns added a comment.

Post by Kai Uwe Broulik
I'm a bit concerned that removing the frame most of the time blurs the lines between "previews of images" and "actual file icons", especially for the SVG case.
Now they all look like proper file icons rather than previews of the file's contents. I can see that an EXE "thumbnail" is a proper file icon but in many other cases it's misleading.

I think the distinction should be between "Documents" and "Activatable items". Examples for the second category:

- Folders (clicking enters the folder)
- Executables (starts the program)
- *.desktop files (starts the referenced program)

Clicking on a document opens it in the associated applicaton (mime based).

Now, confusion happens if you see e.g. the "Blender" icon - will a click open blender, or inkscape/gimp? Folder icon - dolphin or image editor?

I think the confusion will be very rare - this only happens when the user browses e.g. /usr/share/icons/highcolor/*. In case a user takes an icon for the application, the worst thing that will happen is the wrong application is started.

On the other hand, mistaking an application for a document may be bad. Think of an *.exe with an embedded icon which looks like a picture. This icon is passed to the UI unaltered. The current frame is hardly noticable form image previews, so confusion is easy - regardless if we add a frame or not. I think we have this somewhat covered by showing a warning if an executable is not in a trusted location.

If we want to avoid **dangerous** confusion, we should not care for previews, but mark executables.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a comment.

Post by Stefan Brüns
If we want to avoid **dangerous** confusion, we should not care for previews, but mark executables.

This seems like an excellent idea to me. It would improve safety even without this patch.

In addition, maybe we could also consider badging thumbnails with the icon of the app that will open them when clicked.

(both material for different patches of course)

INLINE COMMENTS

Post by Stefan Brüns
anthonyfieroni wrote in kfilepreviewgenerator.cpp:945
Maybe you should add case when it has an alpha but it's nulll.

`hasAlpha()` already seems to check for that, in fact: it returns false for images that have an alpha channel but no transparency.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a comment.


So do we have any firm conclusions here? The current state is already rather broken both conceptually and in practice, as illustrated by the "before" images.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham added a comment.


Friendly ping! I'm getting the sense that nobody feels very strongly about this.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Stefan Brüns]

bruns added a comment.


+1 from me ...

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Andres Betts]

abetts accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
R241 KIO

BRANCH
thumbnail-frame-refinement (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.


Alright let's go with this

REPOSITORY
R241 KIO

BRANCH
thumbnail-frame-refinement (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D15071

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham