Changeset View
Standalone View
thumbnail/thumbnail.cpp
Context not available. | |||||
28 | #ifndef Q_OS_WIN | 28 | #ifndef Q_OS_WIN | ||
---|---|---|---|---|---|
29 | #include <sys/ipc.h> | 29 | #include <sys/ipc.h> | ||
30 | #include <sys/shm.h> | 30 | #include <sys/shm.h> | ||
31 | #include <sys/stat.h> | ||||
31 | #include <unistd.h> // nice() | 32 | #include <unistd.h> // nice() | ||
32 | #endif | 33 | #endif | ||
33 | 34 | | |||
Context not available. | |||||
55 | #include <KMimeTypeTrader> | 56 | #include <KMimeTypeTrader> | ||
56 | #include <KServiceTypeTrader> | 57 | #include <KServiceTypeTrader> | ||
57 | #include <KPluginLoader> | 58 | #include <KPluginLoader> | ||
59 | #include <Solid/Device> | ||||
60 | #include <Solid/StorageVolume> | ||||
58 | 61 | | |||
59 | #include <kaboutdata.h> | 62 | #include <kaboutdata.h> | ||
bruns: What are all these includes for? | |||||
60 | #include <kiconloader.h> | 63 | #include <kiconloader.h> | ||
Context not available. | |||||
62 | #include <kio/thumbcreator.h> | 65 | #include <kio/thumbcreator.h> | ||
63 | #include <kio/thumbsequencecreator.h> | 66 | #include <kio/thumbsequencecreator.h> | ||
64 | #include <kio/previewjob.h> | 67 | #include <kio/previewjob.h> | ||
68 | #include "thumbnail-logsettings.h" | ||||
65 | 69 | | |||
66 | #include <iostream> | 70 | #include <iostream> | ||
67 | #include <limits> | 71 | #include <limits> | ||
Context not available. | |||||
552 | } | 556 | } | ||
553 | 557 | | |||
554 | dir.next(); | 558 | dir.next(); | ||
559 | QString canonicalFilePath = dir.fileInfo().canonicalFilePath(); | ||||
555 | 560 | | |||
556 | if (validThumbnails > 0 && hadFirstThumbnail == dir.filePath()) { | 561 | if (validThumbnails > 0 && hadFirstThumbnail == canonicalFilePath) { | ||
557 | break; // Never show the same thumbnail twice | 562 | break; // Never show the same thumbnail twice | ||
558 | } | 563 | } | ||
Obviously broken, as hadFirstThumbnail is not set from a canonical path .. Or not so obvious, as this is still lacking context. bruns: Obviously broken, as hadFirstThumbnail is not set from a canonical path ..
Or not so obvious… | |||||
559 | 564 | | |||
Context not available. | |||||
563 | continue; | 568 | continue; | ||
564 | } | 569 | } | ||
565 | 570 | | |||
566 | if (!drawSubThumbnail(p, dir.filePath(), segmentWidth, segmentHeight, xPos, yPos, frameWidth)) { | 571 | if (!drawSubThumbnail(p, canonicalFilePath, segmentWidth, segmentHeight, xPos, yPos, frameWidth)) { | ||
567 | continue; | 572 | continue; | ||
568 | } | 573 | } | ||
569 | 574 | | |||
Context not available. | |||||
573 | } | 578 | } | ||
574 | 579 | | |||
575 | if (hadFirstThumbnail.isEmpty()) { | 580 | if (hadFirstThumbnail.isEmpty()) { | ||
576 | hadFirstThumbnail = dir.filePath(); | 581 | hadFirstThumbnail = canonicalFilePath; | ||
577 | } | 582 | } | ||
578 | 583 | | |||
579 | ++validThumbnails; | 584 | ++validThumbnails; | ||
ngraham: There are extra spaces on this line | |||||
Seems suboptimal to copy all the items just to be able to check whether ant of them satisfy a requirement - std::any_of could be an alternative. Or, possibly even better, mountsList.findByPath(...) and then check the filesystem type. ivan: Seems suboptimal to copy all the items just to be able to check whether ant of them satisfy a… | |||||
meven: Add a space after // | |||||
This can be checked much faster by comparing the device ids from a stat call. Then the code would also better match the comment. Though, being on the same FS is a sufficient condition to create the thumbnail, it is not required. So the condition should probably read: dev_t fileDevId = ... if (m_thumbnailDirDeviceId != fileDevId) { bool isEncrypted = ... if (isEncrypted) { continue; // or return or whatever ... } } bruns: This can be checked much faster by comparing the device ids from a stat call. Then the code… | |||||
Calling currentMountPoints() for every created file is definitely a bad idea ... https://github.com/KDE/kio/blob/master/src/core/kmountpoint.cpp#L272 bruns: Calling currentMountPoints() for every created file is definitely a bad idea ...
https… | |||||
Context not available. | |||||
726 | if (!thumbnail.load(thumbPath.absoluteFilePath(thumbName))) { | 731 | if (!thumbnail.load(thumbPath.absoluteFilePath(thumbName))) { | ||
Please do the stat for m_thumbBasePath just once. Also, toStdString is wrong, use the string encoding functions from QFIle. bruns: Please do the stat for m_thumbBasePath just once.
Also, toStdString is wrong, use the string… | |||||
727 | // no cached version is available, a new thumbnail must be created | 732 | // no cached version is available, a new thumbnail must be created | ||
728 | 733 | | |||
734 | // If file is on encrypted filesystem, we can't cache it unless it's on the same filesystem as thumbnail directory | ||||
735 | bool allowCache; | ||||
736 | allowCache = sharesFilesystemWithThumbRoot(filePath); | ||||
737 | if (!allowCache) { | ||||
This will have some impact when creating thumbnails for any external or secondary storage (i.e. everything not in the HOME partition). bruns: This will have some impact when creating thumbnails for any external or secondary storage (i.e. | |||||
738 | Solid::Device device = Solid::Device::storageAccessFromPath(filePath); | ||||
meven: Can't you move this out of the loop ? Since `m_thumbBasePath` does not change. | |||||
I've moved this check into new method, so it should make more sense now. marcingu: I've moved this check into new method, so it should make more sense now. | |||||
There is an error with the previous line, one of the two should be allowDirCached I think. meven: There is an error with the previous line, one of the two should be allowDirCached I think.
Here… | |||||
marcingu: Could you elaborate? I don't see it. | |||||
739 | if (device.is<Solid::StorageVolume>()) { | ||||
740 | allowCache = device.as<Solid::StorageVolume>()->usage() != Solid::StorageVolume::UsageType::Encrypted; | ||||
741 | } | ||||
dfaure: join with previous line (this isn't C) ;) | |||||
742 | } | ||||
743 | | ||||
How about encrypted block storage, which is superior to those encrypted fs and the recommended solution? thiago: How about encrypted block storage, which is superior to those encrypted fs and the recommended… | |||||
marcingu: Any pointers how to do that? | |||||
bruns: Why is all this code called even when ThumbCreator::create fails? | |||||
729 | QSaveFile thumbnailfile(thumbPath.absoluteFilePath(thumbName)); | 744 | QSaveFile thumbnailfile(thumbPath.absoluteFilePath(thumbName)); | ||
730 | bool savedCorrectly = false; | 745 | bool savedCorrectly = false; | ||
731 | if (subCreator->create(filePath, cacheSize, cacheSize, thumbnail)) { | 746 | if (subCreator->create(filePath, cacheSize, cacheSize, thumbnail)) { | ||
Context not available. | |||||
733 | 748 | | |||
734 | // The thumbnail has been created successfully. Store the thumbnail | 749 | // The thumbnail has been created successfully. Store the thumbnail | ||
735 | // to the cache for future access. | 750 | // to the cache for future access. | ||
736 | if (thumbnailfile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { | 751 | if (allowCache && thumbnailfile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { | ||
737 | savedCorrectly = thumbnail.save(&thumbnailfile, "PNG"); | 752 | savedCorrectly = thumbnail.save(&thumbnailfile, "PNG"); | ||
738 | } | 753 | } | ||
739 | } else { | 754 | } else { | ||
Context not available. | |||||
750 | return true; | 765 | return true; | ||
751 | } | 766 | } | ||
752 | 767 | | |||
768 | bool ThumbnailProtocol::sharesFilesystemWithThumbRoot(const QString& path) | ||||
769 | { | ||||
770 | #ifndef Q_OS_WIN | ||||
771 | if (m_thumbnailDirDeviceId == deviceIdUnset) { | ||||
772 | struct stat baseStat; | ||||
773 | if (lstat(QFile::encodeName(m_thumbBasePath).data(), &baseStat) == -1) { | ||||
774 | qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about filesystem under path \"" << m_thumbBasePath << "\""; | ||||
775 | return false; | ||||
776 | } | ||||
meven: `!= -1`, Add a warning with errno should it fail. | |||||
No. The m_thumbnailDirDeviceId is set to 0 and only changed if lstat executes properly. Then it takes value of st_dev. I don't know if st_dev can be 0 though and I couldn't find this information in manual either. If it can, we should change it. marcingu: No. The m_thumbnailDirDeviceId is set to 0 and only changed if lstat executes properly. Then it… | |||||
I don't see how stat would succeed and st_dev would be 0, but I could be wrong. Maybe assert that it's not 0, at least? dfaure: I don't see how stat would succeed and st_dev would be 0, but I could be wrong. Maybe assert… | |||||
If st_dev cannot be 0 on successful call of lstat, I don't think there's need to change anything. m_thumbnailDirDeviceId will remain zero only if lstat returns error and as long it's zero sharesFilesystemWithThumbRoot will return false. marcingu: If st_dev cannot be 0 on successful call of lstat, I don't think there's need to change… | |||||
I agree with your second sentence. I never said otherwise. It will work, *if* indeed we are both right that st_dev would never be 0 when stat() succeeds. dfaure: I agree with your second sentence. I never said otherwise. It will work, *if* indeed we are… | |||||
How about I set it to -1 at start and check for that instead? I checked definitions and it looks like dev_st is unsigned, but it shouldn't be a problem. marcingu: How about I set it to -1 at start and check for that instead? I checked definitions and it… | |||||
-1 isn't exactly a great value for an unsigned int :-) Or rather, it's not great enough (in the "greater than" sense) :-) But yeah, 0xFFFFFFFF is a good "not set yet" value for a dev_t, to be safe against the possibility of 0. dfaure: -1 isn't exactly a great value for an unsigned int :-)
Or rather, it's not great enough (in… | |||||
"-1 isn't exactly a great value for an unsigned int :-)" I believe "-1" guaranties unsigned to be set to its maximum value, due to how type conversion works, but I could be wrong. Do you know if size of dev_st is fixed or depends on architecture? I'd like to define const rather than use literals for checks, but I don't know if I can simply define it as 0xFFFFFFFF or if it's more complicated. marcingu: "-1 isn't exactly a great value for an unsigned int :-)"
I believe "-1" guaranties unsigned to… | |||||
unsigned(-1) is guaranteed to be added to the modulo value until it becomes positive. That is, it is (UINT_MAX + 1) - 1 Note that this will generate a warning with some compilers about change of sign, so either make the cast explicit or simply use ~0U. As for what type it is... that's why we have std::numeric_limits. thiago: ```
unsigned(-1)
```
is guaranteed to be added to the modulo value until it becomes positive. | |||||
777 | m_thumbnailDirDeviceId = baseStat.st_dev; | ||||
778 | } | ||||
779 | struct stat fileStat; | ||||
dfaure: print out the path in the warning (same on line 786) | |||||
780 | if (lstat(QFile::encodeName(path).data(), &fileStat) == -1) { | ||||
781 | qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about filesystem under path \"" << path << "\""; | ||||
error check here for !lstat(QFile::encodeName(path).data(), &fileStat) is important, file might have moved for instance. meven: error check here for `!lstat(QFile::encodeName(path).data(), &fileStat)` is important, file… | |||||
marcingu: Please elaborate, I don't see what the problem is. | |||||
782 | return false; | ||||
783 | } | ||||
784 | return m_thumbnailDirDeviceId == fileStat.st_dev; | ||||
785 | #else | ||||
dfaure: missing space after `if`
s/data/constData/ | |||||
786 | return false; | ||||
787 | #endif | ||||
788 | } | ||||
789 | | ||||
753 | void ThumbnailProtocol::scaleDownImage(QImage& img, int maxWidth, int maxHeight) | 790 | void ThumbnailProtocol::scaleDownImage(QImage& img, int maxWidth, int maxHeight) | ||
754 | { | 791 | { | ||
755 | if (img.width() > maxWidth || img.height() > maxHeight) { | 792 | if (img.width() > maxWidth || img.height() > maxHeight) { | ||
Context not available. |
What are all these includes for?