Make thumbnail frame-and-shadow drawing criteria match those of the file dialog
ClosedPublic

Authored by ngraham on Aug 25 2018, 3:10 AM.

Details

Summary

KIO's file dialog already has logic to avoid drawing frames around images detected as likely to be icons, which is improved with D15071. Since Dolphin doesn't use KIO for any of this (boo) we have to pull it over here too; this patch brings the same feature to Dolphin, as well as the feature to disable frames and shadows for all thumbnails at very small sizes, which improves clarity.

With this patch, Dolphin's frame drawing behavior becomes consistent with that of the file dialog (as of D15071).

BUG: 295526
FIXED-IN: 18.12.0

Test Plan

Icons no longer have frames:

Images without transparency still have frames:

Nicer presentation for folders with mixed image types (images without transparency get frames; images without it don't):

At small sizes, thumbnail clarity is improved by omitting the frame and shadow. Before:

After:

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Aug 25 2018, 3:10 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptAug 25 2018, 3:10 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham requested review of this revision.Aug 25 2018, 3:10 AM
ngraham updated this revision to Diff 40396.Aug 25 2018, 3:15 AM

Limit to SVGs; allowing the logic to apply to PNGs causes too many false positives

Since Dolphin doesn't use KIO for any of this (boo) we have to pull it over here too.

So it's better to be in KIO, can you make to it? @elvisangelaccio ?

ngraham planned changes to this revision.Aug 25 2018, 4:27 AM

Since Dolphin doesn't use KIO for any of this (boo) we have to pull it over here too.

So it's better to be in KIO, can you make to it? @elvisangelaccio ?

Unfortunately the view engines have diverged, so it's not an easy task. But I do agree with you.

However I'm rethinking whether this set of criteria is optimal after reading through https://bugs.kde.org/show_bug.cgi?id=258514 and I plan to change it.

ngraham updated this revision to Diff 40400.Aug 25 2018, 4:53 AM

Use same logic as D15071, which is vastly more reliable (and also remove the AppImage change, which will go in a separate patch)

ngraham retitled this revision from Don't draw a frame around AppImage apps and images detected as likely to be icons to Don't draw a frame around icons and images with transparency.Aug 25 2018, 4:59 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 40401.Aug 25 2018, 5:06 AM

One final change: port over the feature to stop drawing frames and icons for very small sized thumbnails

ngraham retitled this revision from Don't draw a frame around icons and images with transparency to Make thumbnail frame-and-shadow drawing criteria match those of the file dialog.Aug 25 2018, 5:11 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Sweet! How does it work with PNG thumbnails? They could be solid but likely still have alpha?

markg added a subscriber: markg.Aug 25 2018, 11:28 AM

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.

@markg. Thanks for your comments. Do you think we could take the discussion to D15071? I imagine that making Dolphin's frame drawing behavior consistent with the file dialog is probably not very controversial, but I can see how the determination regarding what we draw a frame around in the first place might require more discussion.

Therefore, I'd like to use this patch to make Dolphin use whatever we decide for the file dialog. Therefore do you think we can take the discussion regarding regarding what we draw a frame around over to D15071 so we can have it on one place?

markg added a comment.Aug 25 2018, 4:50 PM

@markg. Thanks for your comments. Do you think we could take the discussion to D15071? I imagine that making Dolphin's frame drawing behavior consistent with the file dialog is probably not very controversial, but I can see how the determination regarding what we draw a frame around in the first place might require more discussion.

Therefore, I'd like to use this patch to make Dolphin use whatever we decide for the file dialog. Therefore do you think we can take the discussion regarding regarding what we draw a frame around over to D15071 so we can have it on one place?

Yes, sure :)
It's difficult to follow which thread is the leading one as there are quite a few now. I will just post my comment from earlier in there as well.

ngraham updated this revision to Diff 41074.Sep 5 2018, 7:40 PM

Simplify the conditional even more, since the alpha check already encompasses all of the prior exceptions. It also fixes the bug of font previews getting a frame when they weren't supposed to

Sweet!

src/kitemviews/kfileitemmodelrolesupdater.cpp
499

Those parentheses are superfluous as you just do a conjunction for all of them; also move the operator to the beginning of the line

ngraham updated this revision to Diff 41075.Sep 5 2018, 7:47 PM

Fix style issues

ngraham marked an inline comment as done.Sep 5 2018, 7:47 PM
broulik accepted this revision.Sep 5 2018, 7:51 PM
This revision is now accepted and ready to land.Sep 5 2018, 7:51 PM

Anyone else in Dolphin wanna weigh in? Since D15071 has already landed, all this really does is make Dolphin consistent with the file dialog. It turns out that hasAlpha() is pretty smart, and I think this will be a nice UI improvement overall. :)

elvisangelaccio accepted this revision.Sep 6 2018, 8:44 PM
This revision was automatically updated to reflect the committed changes.