It was fixed before but somehow, got back in dolphin on kde 4.8.1 . This issue did not exist on dolphin from kde 4.6.x to kde 4.7.x. System: openSUSE 4.8.1 x86_64 As a side not, thanks for fixing the animations. It is not distracting anymore. Regards, André ps.: I will check if it is not an upstream glitch.
Thanks for the report, but I cannot reproduce the issue with the provided information: Could you please attach a screenshot and probably the file itself where a black frame is shown?
Created attachment 69513 [details] Dolphin 4.7.4 - no frames No frames on icon preview (they are all svg images).
Created attachment 69514 [details] Dolphin 4.8.1 - frames On dolphin 4.8.1 the thumbnails of the svg images have a frame.
Sorry to answer so late, but strangely, the report did not show up on my profile and I thought it was somehow considered or invalid or already fixed. I had to search all reports on dolphin to find it. I think you are having some issues with the new bug system. My system is openSUSE 11.4 with multihead setup with xinerama (1 desktop) and two controllers (intel and nvidia graphics adapters). May you need some more information (like xorg.conf or something else) please let me know (even though, like I said, I dont have this issue with previous dolphin versions). Regards, André
Thanks for the update, now I understand what you mean. The change has been done on purpose, but lets wait for further feedback...
OK, I will keep using dolphin from KDE 4.7.4 for awhile (or will patch it to get the original behavior while you collect the information you need). Regards, André
Since Peter obviously wants feedback on this ;) I'm kinda mixed about it. It looks good on images that don't have any transparent parts (like wallpapers and such) but looks very ugly on icons like in the attachments from Andre. I think it's best to remove it or disable it.
There is a (currently) hidden option available in Dolphin 4.8.3 that disables the black borders and also assures that no upscaling of icons is done. Open your ~/.kde/share/config/dolphinrc file and add EnlargeSmallPreviews=false below the [General] section.
Thanks Peter, It was already boring to patch-compile-install every time a new update on dolphin hits openSUSE. Again, thanks for the awesome work. Regards, Andre
Hi Peter, Tried the fix on openSUSE 11.4 with KDE:48 repository (now with KDE 4.8.3). Still got the same issue. Despite this minor glitch, KDE 4.8.3 and Calligra 2.4.1 are really blazing fast and good looking. Congrats to all developers involved. Regards, André
I just verified this with 4.8.3 and you are right: The black border is still shown (only th upscaling is prevented). > Despite this minor glitch, KDE 4.8.3 and Calligra 2.4.1 are really > blazing fast and good looking. Congrats to all developers involved. Thanks!
Hi Peter, First and foremost, thank you for all your great work on kde and on dolphin specially. At first I got very suspect of what I thought would stupefy one of the kde greatest strengths, its file file manager, but the transition to dolphin was most of the time smooth. Second, I really would like to see a transition on the way kde stores its settings and the settings of its applications. It does not help users discover what control the way the applications behave and the options that are there. I do not think that we should dump all on a structured binary blob or xml file, but the current situation is not practical from any point of view, its a mess. Finally, I got an update from openSUSE to KDE 4.8.4. What was already nice is even better now, but as the problem persisted I decided that this was the right time to take a look on it. The patch attached fix the problem by the less intrusive method I could devise. Thanks again, André ################################################################## --- kfileitemmodelrolesupdater.cpp.orig 2012-04-29 17:38:55.000000000 -0300 +++ kfileitemmodelrolesupdater.cpp 2012-06-27 10:47:04.461429183 -0300 @@ -309,6 +309,15 @@ m_changedItemsTimer->start(); } +int hasDefaultIconSize(short int size) +{ + short int defaultIconSizes[] = {8, 16, 22, 24, 32, 48, 64, 72, 96, 128, 192, 256, 512}; + size_t idx = 0; + + for ( ; size != defaultIconSizes[idx] && idx < sizeof(defaultIconSizes) ; idx++ ); + return idx < sizeof(defaultIconSizes); +} + void KFileItemModelRolesUpdater::slotGotPreview(const KFileItem& item, const QPixmap& pixmap) { m_pendingVisibleItems.remove(item); @@ -333,6 +342,16 @@ const QSize contentSize = KPixmapModifier::sizeInsideFrame(m_iconSize); const bool enlargingRequired = scaledPixmap.width() < contentSize.width() && scaledPixmap.height() < contentSize.height(); + + // Detect icons so that to not display a frame around them + const QString mimeTypeName = mimeType.mid(slashIndex+1); + const bool isIcon = ( mimeTypeName == "png" || mimeTypeName.startsWith("svg+xml") ) && + ( scaledPixmap.width() == scaledPixmap.height() && hasDefaultIconSize(scaledPixmap.width()) ); + + // qDebug() << "isIcon: " << isIcon << "; mime: " << mimeTypeName + // << "; width: (" << scaledPixmap.width() << ',' << m_iconSize.width() << ')' + // << "; height: (" << scaledPixmap.height()<< ',' << m_iconSize.height() << ')' << '\n'; + if (enlargingRequired) { QSize frameSize = scaledPixmap.size(); frameSize.scale(m_iconSize, Qt::KeepAspectRatio); @@ -340,17 +359,22 @@ QPixmap largeFrame(frameSize); largeFrame.fill(Qt::transparent); - KPixmapModifier::applyFrame(largeFrame, frameSize); + // Will only paint them in the middle of the frame + if (! isIcon) + KPixmapModifier::applyFrame(largeFrame, frameSize); QPainter painter(&largeFrame); painter.drawPixmap((largeFrame.width() - scaledPixmap.width()) / 2, - (largeFrame.height() - scaledPixmap.height()) / 2, - scaledPixmap); + (largeFrame.height() - scaledPixmap.height()) / 2, + scaledPixmap); scaledPixmap = largeFrame; } else { // The image must be shrinked as it is too large to fit into // the available icon size - KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + if (! isIcon) + KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + else + KPixmapModifier::scale(scaledPixmap, m_iconSize); } } } else {
Hi Peter, A slightly improved (?) patch. Best regards, André ####################################### --- ./dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp.orig 2012-04-29 17:38:55.000000000 -0300 +++ ./dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 2012-06-28 16:59:28.538355112 -0300 @@ -309,6 +309,19 @@ m_changedItemsTimer->start(); } +int hasDefaultIconSize(unsigned short int size) +{ + // Not critical, but lets speed up a bit the operation + short int defaultIconSizes[] = {512, 8, 16, 22, 24, 32, 48, 512, 64, 72, 512, 512, 96, 512, 512, 512, 128, 192, 256, 512}; + size_t idx; + + if (size > 512) + return 0; + + for (idx = size <= 128 ? size >> 3 : 17 + (size >> 16); size > defaultIconSizes[idx] ; idx++ ); + return size == defaultIconSizes[idx]; +} + void KFileItemModelRolesUpdater::slotGotPreview(const KFileItem& item, const QPixmap& pixmap) { m_pendingVisibleItems.remove(item); @@ -333,6 +346,12 @@ const QSize contentSize = KPixmapModifier::sizeInsideFrame(m_iconSize); const bool enlargingRequired = scaledPixmap.width() < contentSize.width() && scaledPixmap.height() < contentSize.height(); + + // Detect icons so that to not display a frame around them + const QString mimeTypeName = mimeType.mid(slashIndex+1); + const bool isIcon = ( mimeTypeName == "png" || mimeTypeName.startsWith("svg+xml") ) && + ( scaledPixmap.width() == scaledPixmap.height() && hasDefaultIconSize(scaledPixmap.width()) ); + if (enlargingRequired) { QSize frameSize = scaledPixmap.size(); frameSize.scale(m_iconSize, Qt::KeepAspectRatio); @@ -340,7 +359,9 @@ QPixmap largeFrame(frameSize); largeFrame.fill(Qt::transparent); - KPixmapModifier::applyFrame(largeFrame, frameSize); + // Will only paint them in the middle of the frame + if (! isIcon) + KPixmapModifier::applyFrame(largeFrame, frameSize); QPainter painter(&largeFrame); painter.drawPixmap((largeFrame.width() - scaledPixmap.width()) / 2, @@ -350,7 +371,10 @@ } else { // The image must be shrinked as it is too large to fit into // the available icon size - KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + if (! isIcon) + KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + else + KPixmapModifier::scale(scaledPixmap, m_iconSize); } } } else {
Created attachment 72205 [details] Patch do disable frames on regular icons
Resetting assignee to default as per bug #305719
OK, got the same behavior on KDE 4.6.5 from openSUSE. Time to new patch, this one a bit more polished and unfunny. ------------------------------------ --- ./dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp.orig 2012-04-29 17:38:55.000000000 -0300 +++ ./dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 2012-09-12 21:04:32.000000000 -0300 @@ -309,6 +309,17 @@ m_changedItemsTimer->start(); } +inline bool hasDefaultIconSize(unsigned short int size) +{ + switch (size) { + case 8: case 16: case 22: case 24: case 32: case 48: case 56: + case 64: case 72: case 96: case 128: case 192: case 256: case 512: + return true; + default: + return false; + } +} + void KFileItemModelRolesUpdater::slotGotPreview(const KFileItem& item, const QPixmap& pixmap) { m_pendingVisibleItems.remove(item); @@ -324,15 +335,25 @@ const QString mimeType = item.mimetype(); const int slashIndex = mimeType.indexOf(QLatin1Char('/')); const QString mimeTypeGroup = mimeType.left(slashIndex); + + // Detect icons so that to not display a frame around them + const QString mimeTypeName = mimeType.mid(slashIndex+1); + const bool isIcon = ( mimeTypeName == "png" || mimeTypeName.startsWith("svg+xml") ) && + ( scaledPixmap.width() == scaledPixmap.height() && hasDefaultIconSize(scaledPixmap.width()) ); + if (mimeTypeGroup == QLatin1String("image")) { if (m_enlargeSmallPreviews) { - KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + if (! isIcon) + KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + else + KPixmapModifier::scale(scaledPixmap, m_iconSize); } else { // Assure that small previews don't get enlarged. Instead they // should be shown centered within the frame. const QSize contentSize = KPixmapModifier::sizeInsideFrame(m_iconSize); const bool enlargingRequired = scaledPixmap.width() < contentSize.width() && scaledPixmap.height() < contentSize.height(); + if (enlargingRequired) { QSize frameSize = scaledPixmap.size(); frameSize.scale(m_iconSize, Qt::KeepAspectRatio); @@ -340,7 +361,9 @@ QPixmap largeFrame(frameSize); largeFrame.fill(Qt::transparent); - KPixmapModifier::applyFrame(largeFrame, frameSize); + // Will only paint them in the middle of the frame + if (! isIcon) + KPixmapModifier::applyFrame(largeFrame, frameSize); QPainter painter(&largeFrame); painter.drawPixmap((largeFrame.width() - scaledPixmap.width()) / 2, @@ -350,7 +373,10 @@ } else { // The image must be shrinked as it is too large to fit into // the available icon size - KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + if (! isIcon) + KPixmapModifier::applyFrame(scaledPixmap, m_iconSize); + else + KPixmapModifier::scale(scaledPixmap, m_iconSize); } } } else {
Created attachment 73877 [details] No frames on icons on kde 4.8.5, please
Thanks for looking into this issue! First of all, let me say that I was not aware about this issue until I saw your new comments today - I only took over Dolphin's maintainership very recently. I do not think that the decision if a frame should be painted or not should be based on the preview size, in particular if the 'good' sizes are hardcoded. This is just asking for trouble and will lead to unexpected behaviour and sutle bugs in other situations. I see that the frame might not be really useful for icons, but if there really is a big demand to change that, it must be done in another way.
Created attachment 73887 [details] No frames on icons on kde 4.8.5, please
(In reply to comment #18) > Thanks for looking into this issue! > > First of all, let me say that I was not aware about this issue until I saw > your new comments today - I only took over Dolphin's maintainership very > recently. > > I do not think that the decision if a frame should be painted or not should > be based on the preview size, in particular if the 'good' sizes are > hardcoded. This is just asking for trouble and will lead to unexpected > behaviour and sutle bugs in other situations. > > I see that the frame might not be really useful for icons, but if there > really is a big demand to change that, it must be done in another way. I agree. The problem is, icons can be anywhere inside the file system. A better approach would be to have an option to display icons inside a particular directory(ies) without frames, the same way dolphin remember how to display directory contents (detailed, preview, etc). The current situation is really awful if you happen to be a graphic artist browsing the elements you may use as their previews are severely impaired. I would greatly prefer an extra option inside the menu "View" with the option to display images without frames. Would fix the problem for all cases without disturbing the default behavior.
some comments here.... svg dont rely need or should have a dropshadow, its mostlly a design tool not for joe user and I like the preview as it saves me the time from well reaing what ever foo name I have wrinten for it the shadow is not somthing that hads value to it... upscaling is usualy wrong, lets not do it or not do it for anything smaler than 128x128 (in the source image), so my opinion is ..... images smaler than 128x128 no upscaling in the preview, plain old bottom center anchoring. images with alpha no dropshadow at all :) svg's(svgz's) same thing we can spend some time making better dropshadows for all sizes (not sure what we are using now but im sure we can do it better, they look way to big in small previews) Last note dropshadows are great for joe user i want to see my cat pictures... this are just complints from a sub segment of the user base (designers of small pictures) but i think its possible to make every one happy...
(In reply to comment #21) > Last note dropshadows are great for joe user i want to see my cat > pictures... The frame is actually also useful to see at first sight which area "belongs" to the file in the sense that it's clickable. > images smaler than 128x128 no upscaling in the preview, plain old bottom center anchoring. One can have long discussions about this topic ;-) I see that your statement is true from an icon designer's point of view, but I think that upscaling makes the view layout look better for the vast majority of users. Just a side note: AFAIK, Nautilus and Finder scale up as well and don't provide an option. > this are just complints from a sub segment of the user base > (designers of small pictures) but i think its possible to make every one > happy... Unfortunately, it's very hard to make everyone (which includes the person who has to maintain the code!) happy ;-) Special cases in the code unfortunately make the application harder to maintain. Peter's decision to always draw the frame was obviously motivated by the desire to make maintenance easier. I don't really have more time for Dolphin than he did, so I'm inclined not to revert his decision.
"One can have long discussions about this topic ;-) I see that your statement is true from an icon designer's point of view, but I think that upscaling makes the view layout look better for the vast majority of users. Just a side note: AFAIK, Nautilus and Finder scale up as well and don't provide an option." No upscaling is usualy a very bad idea no mater what, if teh image is big enough then its passable, but as a general rule you should never upscale images... "Unfortunately, it's very hard to make everyone (which includes the person who has to maintain the code!) happy ;-) Special cases in the code unfortunately make the application harder to maintain. Peter's decision to always draw the frame was obviously motivated by the desire to make maintenance easier. I don't really have more time for Dolphin than he did, so I'm inclined not to revert his decision." were is that code? is it c++ or qml? The dropshadow in small items looks wrong and IMO should be removed all together.... shadows are biguer in big items and smaler in smaler items to make sence... look at the oxygen mymetypes, each size as its dropshadow.... I undrstand the argument "I have no time to fix it" but not the "Special cases in the code unfortunately make the application harder to maintain" is not what im talking about here, im talking about making the defoult beter for all usecases. and its worth the extra efort in the code bit... and I'm sure we are not talking about days of code here..
I am tempted to provide a patch with just an extra option to be put on menu "view" if you don't bother.
First of all, let me say that I understand your point of view and that I first shared your opinion. However, when I investigated this a bit more, checked when and how it was changed and what reasons Peter had for it, I changed my mind. Second, I think we can find a solution that makes everyone happy. Considering that nobody else complained about upscaling of icons or the frames yet, and that the EnlargeSmallPreviews option is hidden and private anyway, we could change that option the following way: If it is disabled, not only upscaling is prevented, but also drawing frames. Would that be OK for you? (In reply to comment #23) > were is that code? is it c++ or qml? The old code is somewhere in KFilePreviewGenerator. It's C++, no qml is used anywhere in Dolphin or the parts of kdelibs that it uses AFAIK. The "new" code that is responsible for the generation of previews, the upscaling and the frames is in void KFileItemModelRolesUpdater::slotGotPreview(const KFileItem& item, const QPixmap& pixmap) in dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp > I undrstand the argument "I have no time to fix it" but not the "Special > cases in the code unfortunately make the application harder to maintain" is > not what im talking about here, im talking about making the defoult beter > for all usecases. and its worth the extra efort in the code bit... and I'm > sure we are not talking about days of code here.. For someone who maintains an application that is used by many people, there is basically no difference between "I don't have the time to fix it" and "I don't have the time to maintain the new code in the future". Every change that adds a new code path is likely to increase the maintenance burden in the future. Look at the "EnlargeSmallPreviews" option. Looks harmless enough, doesn't it? But now it's broken, and I have to spend some time to fix it. And there are lots of other examples for this: Some code is made a little more complex to fix a bug or implement a small feature, but some time later, a change somewhere else breaks it, or a combination of settings leads to unexpected results. (In reply to comment #24) > I am tempted to provide a patch with just an extra option to be put on menu > "view" if you don't bother. I see that this looks like a good solution at first sight. However, I get requests like "We could just add a new option, and everyone will be happy" every 1 or 2 weeks. Therefore, I'm very conservative about adding new options. If I always said "yes", we could easily have 4 or 5 times as many options in Dolphin, and you probably agree that this would not improve the usability. Whenever a new option or new code in general is added, the maintainer therefore has to evaulate if there is a very good reason for it. I'm sorry, but considering that only two users have complained about this so far, I don't think that Peter's decisions should be reverted. But anyway, I have made a suggestion that I believe could make everyone happy, see above.
"Considering that nobody else complained about upscaling of icons or the frames yet" heee its the sort of things our user base does not complain, beleive-me it is a serius mater and makes us look like amatours... "not only upscaling is prevented, but also drawing frames. Would that be OK for you?" Yes and i would pay youmore than one beer at akademy next year ;) (me notes that spanish beer is not good so we have to bring how hown ;) ) Now let me tell you what went up the last time with peter, (we in oxygen worked alot with Peter) at the time peter was making the new rendering stuf it was not complete and I could not use it kinda like it is now, so i emaild him personaly asking for the no scaling feature so i could work again, it was my plan to work with Peter on this particular issue like we did in many issues before, the hiden option was just a temporary hack to alow-me to get back to work, it was included mid way inbetwen kde releses and all... So I apreciate that you are gona include the option in an hiden form, but I note that the curent implementation is from a presentation POV subpar and not up to the quality UI/UX standards of Dolphin... Dolphin and Oxygen have a long tradition of cooperation on small minute UI details, and there are still loads of things we should improve in the UI/UX, but as usual depends on our free time. P.S. another element that we should have a look at is the excive framing we still have in dolphin and how we can get rid of it... in qt5 qmlscene based widget world we probably will be able 2 (overlaid scrollbars on top of content for the side pannels come to mind as a frist one to do)
(In reply to comment #26) > Yes and i would pay youmore than one beer at akademy next year ;) (me notes > that spanish beer is not good so we have to bring how hown ;) ) I don't know if I'll make it to Akademy, but if I do, you'll certainly get a beer in return from me for your awesome icons! I've attached a patch (not working perfectly) for the hidden option to bug 307522. If you find some time, feel free to test it. > Now let me tell you what went up the last time with peter, (we in oxygen > worked alot with Peter) at the time peter was making the new rendering stuf > it was not complete and I could not use it kinda like it is now, so i emaild > him personaly asking for the no scaling feature so i could work again, it > was my plan to work with Peter on this particular issue like we did in many > issues before, the hiden option was just a temporary hack to alow-me to get > back to work, it was included mid way inbetwen kde releses and all... > > So I apreciate that you are gona include the option in an hiden form, but I > note that the curent implementation is from a presentation POV subpar and > not up to the quality UI/UX standards of Dolphin... Dolphin and Oxygen have > a long tradition of cooperation on small minute UI details, and there are > still loads of things we should improve in the UI/UX, but as usual depends > on our free time. Yes, I'm sure that there are lots of things that can be improved! They should best be discussed on the kfm-devel@kde.org list.
I'm afraid I have to close this bug report for several reasons: 1. Multiple issues are mixed together: the black shadow border, upscaling small images, and how settings are stored. We can't have bugs like this, because one thing may be fixed or nixed, and the remaining issues will be lost. 2. There is an important reason for the black border that nobody has mentioned: it's a visual cue that the file is an image (icons are a type of image), and not, say, an executable application. This visual cue is important for users. And today, as of KF5 Dolphin, the border is a nice shadow--much more attractive 3. There is a workaround in the form of a hidden setting that experts can add to the config file. 4. This is a super duper niche issue, as evidenced by a lack of comments, votes, or duplicate bug reports in the last 5 years.
*** Bug 366730 has been marked as a duplicate of this bug. ***
Looks like I was wrong, and I've found a few duplicate bugs. Also, we have a patch that makes this optional at https://bugs.kde.org/show_bug.cgi?id=378701. Marking as a duplicate of that issue, and I very much hope that patch gets in. *** This bug has been marked as a duplicate of bug 378701 ***
Un-duping since that issue really is different, and I'm preparing to submit a patch that fixes this but not that.
Submitted a patch: https://phabricator.kde.org/D15069
Git commit cf0052c2968a7c3e110e24134dbe7ef13c835995 by Nathaniel Graham. Committed on 06/09/2018 at 20:47. Pushed by ngraham into branch 'master'. Make thumbnail frame-and-shadow drawing criteria match those of the file dialog 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). FIXED-IN: 18.12.0 Test Plan: Icons no longer have frames: {F6214279} Images without transparency still have frames: {F6214280} Nicer presentation for folders with mixed image types (images without transparency get frames; images without it don't): {F6214278} At small sizes, thumbnail clarity is improved by omitting the frame and shadow. Before: {F6214296} After: {F6214294} Reviewers: #dolphin, broulik, elvisangelaccio Reviewed By: #dolphin, broulik, elvisangelaccio Subscribers: markg, anthonyfieroni, elvisangelaccio, kfm-devel Tags: #dolphin Differential Revision: https://phabricator.kde.org/D15069 M +3 -8 src/kitemviews/kfileitemmodelrolesupdater.cpp https://commits.kde.org/dolphin/cf0052c2968a7c3e110e24134dbe7ef13c835995