D15069: Don't draw a frame around AppImage apps and images detected as likely to be icons

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Dolphin, broulik.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
ngraham requested review of this revision.

REVISION SUMMARY
The file dialog in KIO already has logic to avoid drawing frames around images detected as likely to be icons. This patch brings that same feature to Dolphin. Also don't draw frames around AppImage apps, just like we don't draw frames around Windows executables.

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

BUG: 295526
FIXED-IN: 18.12.0

TEST PLAN
Icons no longer have frames:

Non-icons still have frames:

REPOSITORY
R318 Dolphin

BRANCH
no-frame-around-icons (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik
Cc: kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham updated this revision to Diff 40396.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=40395&id=40396

BRANCH
no-frame-around-icons (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik
Cc: kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Anthony Fieroni]

anthonyfieroni added subscribers: elvisangelaccio, anthonyfieroni.
anthonyfieroni added a reviewer: elvisangelaccio.
anthonyfieroni added a comment.

Post by Nathaniel Graham
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 ?

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham planned changes to this revision.
ngraham added a comment.

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

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.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham updated this revision to Diff 40400.
ngraham added a comment.


Use same logic as D15071 <https://phabricator.kde.org/D15071>, which is vastly more reliable (and also remove the AppImage change, which will go in a separate patch)

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=40396&id=40400

BRANCH
no-frame-around-icons (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

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".
ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham updated this revision to Diff 40401.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=40400&id=40401

BRANCH
no-frame-around-icons (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

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".
ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Mark Gaiser]

markg added a comment.


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.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


@markg. Thanks for your comments. Do you think we could take the discussion to D15071 <https://phabricator.kde.org/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 <https://phabricator.kde.org/D15071> so we can have it on one place?

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
@markg. Thanks for your comments. Do you think we could take the discussion to D15071 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham updated this revision to Diff 41074.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=40401&id=41074

BRANCH
arcpatch-D15069

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Kai Uwe Broulik]

broulik added a comment.


Sweet!

INLINE COMMENTS

kfileitemmodelrolesupdater.cpp:499
+ if (!pixmap.hasAlpha() &&
+ (m_iconSize.width() > KIconLoader::SizeSmallMedium &&
+ m_iconSize.height() > KIconLoader::SizeSmallMedium)) {

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

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham updated this revision to Diff 41075.
ngraham added a comment.


Fix style issues

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=41074&id=41075

BRANCH
arcpatch-D15069

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Kai Uwe Broulik]

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

REPOSITORY
R318 Dolphin

BRANCH
arcpatch-D15069

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


Anyone else in #dolphin <https://phabricator.kde.org/tag/dolphin/> wanna weigh in? Since D15071 <https://phabricator.kde.org/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. :)

REPOSITORY
R318 Dolphin

BRANCH
arcpatch-D15069

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
R318 Dolphin

BRANCH
arcpatch-D15069

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

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:cf0052c2968a: Make thumbnail frame-and-shadow drawing criteria match those of the file dialog (authored by ngraham).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15069?vs=41075&id=41122

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

AFFECTED FILES
src/kitemviews/kfileitemmodelrolesupdater.cpp

To: ngraham, #dolphin, broulik, elvisangelaccio
Cc: markg, anthonyfieroni, elvisangelaccio, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp