| broulik | |
| cfeck | |
| abetts |
| Frameworks | |
| Dolphin | |
| VDG |
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
Previously most would get frames, but some wouldn't, depending on their dimensions. Now all do.
Before:
After:
Previously most would get frames, but some wouldn't, depending on their dimensions. There was also a bug where screenshots that includes shadows got a double shadow (one from the image itself, another from the frame-and-shadow. Now none get the frame.
Before:
After:
Before:
After:
Before:
After:
Before:
After:
Before:
After:
Previously most would get frames, but some wouldn't, depending on their dimensions. Now all do.
Before:
After:
Previously most (but not all) icon SVGs would not get frames, but some would, depending on their dimensions. Non-icon SVGs would mostly get frames, but some would not, depending on their dimension. Now they only do if they have transparency.
Before:
After:
Before:
After:
| No Linters Available |
| No Unit Test Coverage |
| Buildable 2220 | |
| Build 2238: arc lint + arc unit |
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?
@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 two new versions of this:
The two images that have no actual transparency get frames; the one with transparency doesn't. So at least on X11 (which is where I tested this), the QPixmap::hasAlpha() function seems to test for the presence of actual transparency, not merely the alpha channel itself. IMHO this is correct and desirable.
Proof; see the three images in the top-left corner of this screenshot:
(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.
@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 other file managers. 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. :)
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?
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...
https://git.reviewboard.kde.org/r/129921/ maybe because it's deprecated before thumbnail got fixed
| src/filewidgets/kfilepreviewgenerator.cpp | ||
|---|---|---|
| 945 | Maybe you should add case when it has an alpha but it's nulll. | |
I think the distinction should be between "Documents" and "Activatable items". Examples for the second category:
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.
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)
| src/filewidgets/kfilepreviewgenerator.cpp | ||
|---|---|---|
| 945 | hasAlpha() already seems to check for that, in fact: it returns false for images that have an alpha channel but no transparency. | |
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.