D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[Nathaniel Graham]

ngraham added a comment.


  In D15071#316156 , @bruns wrote:
  
  > 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

> 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

D15071: Don't draw frames and shadows around images with transparency

[Stefan Brüns]

bruns added a comment.


  In D15071#315839 , @broulik wrote:
  
  > 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

D15071: Don't draw frames and shadows around images with transparency

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D15071#315839 , @broulik wrote:
  
  > 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

> 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

D15071: Don't draw frames and shadows around images with transparency

[Nathaniel Graham]

ngraham added a comment.


  In D15071#315839 , @broulik wrote:
  
  > 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.
  
  > 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?
  
  > 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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[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 
, 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

D15071: Don't draw frames and shadows around images with transparency

[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

D15071: Don't draw frames and shadows around images with transparency

[Mark Gaiser]

markg added a comment.


  (repost from 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 
. 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

D15071: Don't draw frames and shadows around images with transparency

[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