D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80021. marcingu added a comment. Removing extra whitespaces REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=79986=80021 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham Cc: ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack,

D28746: Show previews on encrypted filesystems

marcingu updated this revision to Diff 80022. marcingu added a comment. Removing extra whitespaces REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28746?vs=79991=80022 REVISION DETAIL https://phabricator.kde.org/D28746 AFFECTED FILES

D28797: Removing extra whitespaces

marcingu created this revision. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. marcingu requested review of this revision. REVISION SUMMARY Removing extra whitespaces REPOSITORY R320 KIO Extras REVISION DETAIL

D28746: Show previews on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28746 To: marcingu, ivan, #frameworks, dfaure, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skipping catching of thumbnails on encrytped filesystems

marcingu created this revision. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. marcingu requested review of this revision. REVISION SUMMARY When generating thumbnails for directory, don't cache the previews for files stored on encrypted

D28745: Skipping catching of thumbnails on encrytped filesystems

marcingu added a dependency: D28746: Show previews on encrypted filesystems. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde,

D28746: Show previews on encrypted filesystems

marcingu added a dependent revision: D28745: Skipping catching of thumbnails on encrytped filesystems. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28746 To: marcingu Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28746: Show previews on encrypted filesystems

marcingu created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marcingu requested review of this revision. REVISION SUMMARY Instead of skipping generating previews on encrypted filesystems, do create them but don't cache. BUG: 411919

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, Thanks! I'll check it this weekend. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80458. marcingu added a comment. Checking if file is on the same filesystem as thumbnails cache directory using lstsat REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80021=80458 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added a comment. I improved check if file is on the same filesystem as thumbnails cache, but don't know if we can get rid of KMountPoint completely. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To:

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80526. marcingu added a comment. Review fixes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80458=80526 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES thumbnail/thumbnail.cpp

D28746: Show previews on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28746 To: marcingu, ivan, #frameworks, dfaure, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skipping catching of thumbnails on encrypted filesystems

marcingu updated this revision to Diff 79986. marcingu added a comment. Review fixes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=79826=79986 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES thumbnail/thumbnail.cpp To:

D28746: Show previews on encrypted filesystems

marcingu updated this revision to Diff 79985. marcingu added a comment. Review fixes REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28746?vs=79832=79985 REVISION DETAIL https://phabricator.kde.org/D28746 AFFECTED FILES src/widgets/previewjob.cpp To:

D28745: Skipping catching of thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde,

D28746: Show previews on encrypted filesystems

marcingu updated this revision to Diff 79991. marcingu added a comment. Moving boolean variables to the front of logic statement. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28746?vs=79985=79991 REVISION DETAIL https://phabricator.kde.org/D28746

D28746: Show previews on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28746 To: marcingu, ivan, #frameworks, dfaure, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 81167. marcingu added a comment. Limiting usage of KMountPoint and lstat to max once per directory. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80526=81167 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added a comment. Unless there's way to get rid of KMountPoint completely, this should reduce number of calls to minimum. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:738 > 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. REPOSITORY

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, aprcela,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 81255. marcingu added a comment. Moving check for sharing filesystem with thumbroot into new method. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=81167=81255 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. PING! Is current code fine or should we get rid of KMountPoint completely somehow? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#648036 , @meven wrote: > This is gonna have an hefty toll on perf as it will add a `getmntent` syscall to every thumbnail generation. > Using `Solid::Device::listFromType` would leverage Solid always

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:738 > There is an error with the previous line, one of the two should be > allowDirCached I think. > Here you always ignore result of `sharesFilesystemWithThumbRoot` Could

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676321 , @bruns wrote: > In D28745#676320 , @marcingu wrote: > > > In D28745#676317 , @bruns wrote: > > > > > In

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676303 , @bruns wrote: > Second, I have asked for a full context diff, or even better moving this to invent.kde.org, but @marcingu keeps ignoring this. Sorry about that. It got lost under all

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. I need help from someone with good understanding of Solid to continue. I'm don't know how to determinate if StorageAccess device is encrypted or not. I wanted to use `StorageVolume::usage`, but it's not available for all types of devices and doesn't

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ping! I'm remanding about question early, because I could do much more work if I get to do it on weekend. Question: This code won't save thumbnail for file on any device that isn't `StorageVolume` or is `StorageVolume` with `usage` `UsageType::Encrypded`.

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676317 , @bruns wrote: > In D28745#676313 , @marcingu wrote: > > > Ping! > > I'm remanding about question early, because I could do much more work if I get to

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676566 , @sitter wrote: > In D28745#676452 , @marcingu wrote: > > > !PING. > > I need help from someone with good understanding of Solid to continue. > > >

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I think I have the Solid part done, but as I don't know this code well, I'd be grateful if someone more advanced on the subject checked it. https://invent.kde.org/frameworks/solid/-/merge_requests/19 Unfortunately, I wasn't able to test it on LUKS device,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > -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, 0x is a good "not set yet" value for a dev_t, to be safe >

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83357. marcingu added a comment. Moving allowCache check, so it's done only if thumbnail was created. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83356=83357 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83355. marcingu added a comment. Removing unnecessary includes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83354=83355 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83356. marcingu added a comment. Setting canonical path as value of hadFirstThumbnail REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83355=83356 REVISION DETAIL https://phabricator.kde.org/D28745

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83353. marcingu added a comment. Skipping usage of POSIX functions and types on Windows REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83351=83353 REVISION DETAIL https://phabricator.kde.org/D28745

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > 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. > My suggestion is that we make sure our assumption

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 3 inline comments as done. marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.h:93 > Which check? This is a member variable, and I'm not sure the dev_t type will > be known at all on Windows. > But if you're not sure either, let's wait until CI tells us.

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83351. marcingu added a comment. Adding path to error logs REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83350=83351 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:781 > error check here for `!lstat(QFile::encodeName(path).data(), )` is > important, file might have moved for instance. Please elaborate, I don't see what the problem is. REPOSITORY R320 KIO Extras REVISION

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.h:93 > This will break compilation on Windows, I assume? > > On Unix, is the corresponding include present in this file? The lack of > context makes reviewing difficult

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > 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? If st_dev cannot be 0 on successful call of lstat, I don't think there's need to

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83358. marcingu added a comment. Renaming "deviceIdUnset" to "s_deviceIdUnset" REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83357=83358 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83354. marcingu added a comment. Using special value instead of 0 as unset m_thumbnailDirDeviceId. Changing ifdef checks for Q_OS_WIN for consistency. Moving ifdev check into sharesFilesystemWithThumbRoot, to reduce number of preprocessor directives.

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 9 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. How do we check if file access is encrypted using Solid? Do we need new property/method in `StorageAccess`? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I tried to research Solid using api.kde.org (https://api.kde.org/frameworks/solid/html/classSolid_1_1Device.html, https://api.kde.org/frameworks/solid/html/classSolid_1_1StorageVolume.html) and looking for usages of both Solid::Device and Solid::StorageVolume in

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. > How often do I have to repeat the thumbnailer has to use the canonical path anyway? Please use that. `storageAccessFromPath` converts given path into canonical. I figured it should do so anyway, so I'm not doing it again here. REPOSITORY R320 KIO Extras

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I made merge request for storageAccessFromPath: https://invent.kde.org/frameworks/solid/-/merge_requests/8 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, so far I have implemented `Solid::Device::storageAccessFromPath` by talking all StorageAccess devices, going though all of them and and returning proper one. code: Solid::Device Solid::Device::storageAccessFromPath(const QString ) { // TODO

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ping! I'm not able to continue without help of someone who knows Solid::Device well. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago, bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, so, what I want to do now is to create static method `findByPath` which is going to return Solid::StorageVolume instance (is there a case in which we can expect something different than StorageVolume?). Should it be `StorageVolume

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#675709 , @bruns wrote: > In D28745#675698 , @marcingu wrote: > > > > How often do I have to repeat the thumbnailer has to use the canonical path anyway? Please

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83350. marcingu added a comment. Using solid for getting Device REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=81255=83350 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:776 > `!= -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

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Here's the new merge request: https://invent.kde.org/network/kio-extras/-/merge_requests/75 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu abandoned this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, badbunny, waitquietly,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#677439 , @ngraham wrote: > Is this unblocked now that https://invent.kde.org/frameworks/solid/-/merge_requests/19 has been merged? I need to make some small changes. I'll create new merge request on