D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  thumbnail/thumbnail.cpp

To: marcingu, ivan, broulik, #dolphin, ngraham
Cc: ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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
  src/widgets/previewjob.cpp

To: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28797: Removing extra whitespaces

[Marcin Gurtowski]

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
  https://phabricator.kde.org/D28797

AFFECTED FILES
  thumbnail/thumbnail.cpp

To: marcingu
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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

[Marcin Gurtowski]

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 filesystems.
  BUG: 411919
  This change is only part of solution and will be added as dependency for the 
rest of the fix.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp

To: marcingu
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28745: Skipping catching of thumbnails on encrytped filesystems

[Marcin Gurtowski]

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, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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

[Marcin Gurtowski]

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28746

AFFECTED FILES
  src/widgets/previewjob.cpp

To: marcingu
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, 
rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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

[Marcin Gurtowski]

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: marcingu, ivan, broulik, #dolphin
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skipping catching of thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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

AFFECTED FILES
  src/widgets/previewjob.cpp

To: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28746: Show previews on encrypted filesystems

[Marcin Gurtowski]

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

[Marcin Gurtowski]

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
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  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, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 up-to-date 
(using events rather thane sysalls) device cache.
  >  I am not sure in the end this is preferable though.
  
  
  Unfortunately I'm new to the project and have no idea what would be the best 
way of checking if path is on encrypted filesystem.
  
  One thing I could do is to move check into thumbForDirectory and transfer it 
through drawSubThumbnail and createSubThumbnail. Which will run the check once 
per directory, instead of running it up to four times.
  The big downside however is that we no longer will be able to skip the check 
if thumbnail already exist, so that likely would be slower in most cases.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham
Cc: meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 you elaborate? I don't see it.

> thiago wrote in thumbnail.cpp:743
> How about encrypted block storage, which is superior to those encrypted fs 
> and the recommended solution?

Any pointers how to do that?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

marcingu added a comment.


  In D28745#676321 , @bruns wrote:
  
  > In D28745#676320 , @marcingu 
wrote:
  >
  > > 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 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`.
  > > >
  > > >
  > > > The whole block can never return true, so it should just be removed, 
along with all its dependencies.
  > >
  > >
  > > I tested it once more and it returns true when it should, as expected. 
What makes you think it doesn't?
  >
  >
  > Even worse, its almost random:
  >
  >   udi = '/org/kde/fstab///pebbles/foo:/mnt'
  > parent = '/org/kde/fstab'  (string)
  > vendor = 'pebbles'  (string)
  > product = 'foo:/mnt'  (string)
  > description = 'foo:/mnt on pebbles'  (string)
  > icon = 'network-server'  (string)
  > StorageAccess.accessible = false  (bool)
  > StorageAccess.filePath = '/mnt'  (string)
  > StorageAccess.ignored = false  (bool)
  > NetworkShare.type = 'Cifs'  (0x2)  (enum)
  > NetworkShare.url = 'smb://pebbles/foo:/mnt'  (string)
  >
  >
  > `if (device.is())` -> false, though it should be 
cached
  >
  >   udi = '/org/freedesktop/UDisks2/block_devices/dm_2d2'
  > parent = '/'  (string)
  > vendor = ''  (string)
  > product = ''  (string)
  > description = '100,0 GiB Hard Drive'  (string)
  > icon = 'drive-harddisk-root'  (string)
  > Block.major = 254  (0xfe)  (int)
  > Block.minor = 2  (0x2)  (int)
  > Block.device = '/dev/dm-2'  (string)
  > StorageAccess.accessible = true  (bool)
  > StorageAccess.filePath = '/'  (string)
  > StorageAccess.ignored = true  (bool)
  > StorageVolume.ignored = false  (bool)
  > StorageVolume.usage = 'FileSystem'  (0x2)  (enum)
  > StorageVolume.fsType = 'btrfs'  (string)
  > StorageVolume.label = ''  (string)
  > StorageVolume.uuid = '5832ebfa-bf02-40d2-bdc7-90403b207b62'  (string)
  > StorageVolume.size = 107374182400  (0x19)  (qulonglong)
  >
  >
  > This is an LUKS encrypted volume so should not be cached ...
  
  
  Ok, so once more, let's go back to my question, but in more detail.
  
  Currently I get device as StorageAccess, because storage volumes don't 
include all of the mount points, such as mounted encfs encrypted data.
  This could be a problem as StorageAccess doesn't have an information about 
encryption (as far as I can tell) . As mentioned before, from lack of better 
solution, I assume false for storage devices that aren't StorageVolume, but 
it's not ideal.
  
  For the rest of devices I was relaying on `usage` from `StorageVolume`, but 
as it was pointed out, it holds `FileSystem` for LUKS encrypted volume, so it's 
either a bug in Solid or I'm using it wrong and I should get information about 
encryption elsewhere. I wasn't able to find anything else that could help 
though.
  
  When it comes to NetworkShare, I tested it with ssh and MTP. KIO won't even 
attempt to make previews for directories, so catching is out of question. I 
don't think we should change this behavior as a part of this story, but it 
would be good for code to work for them anyway, because we might decide to make 
those previews in the future or  use 
   the same mechanism for catching rest of the files.
  
  Do we want to store caches for NetworkShare devices and external storage? 
Currently we do, with exception of directories on NetworkShare and I see no 
need to change it, but it might be good to make sure.
  Are there any other corner cases we're forgetting?
  
  I also have a personal request for @bruns . Your responses tend to be 
laconic, often missing suggested solution for given issue and sometimes even 
pointing out what the problem is. This leads to me wasting a lot of time to 
find out something that would take you few moments to communicate.
  I'm willing to spend as much time as it takes to make this code as good as 
possible and I'm thankful for all responses so far, but you need to remember 
I'm only starting work with KDE code, so things that are obvious to you, often 
won't be as clear to me.
  
  In D28745#676322 , @thiago wrote:
  
  > BTW, have you checked if the thumbnails are still generated for 
non-removable but encrypted filesystems? My whole system is encrypted (except 
for /boot), so it would be a performance loss if no thumbnails were ever cached.
  
  
  Non-removable – not. Currently I do cache encrypted files when they are on 
the same device as thumbnails directory 

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 suggestions. I'll post the next 
change to invent.kde.org and link it here.
  
  In D28745#676303 , @bruns wrote:
  
  > @marcingu - have you even verified this works? - I am quite sure it does 
not, neither for fuse.encfs, fuse.cryfs (as used by Vaults), nor for any LUKS 
encrypted devices.
  
  
  I double checked and I believe there's space for discussion with more 
involved developers/designers.
  The code behaves in expected way (thumbnails are not saved), but not because 
`usage` for those devices is `UsageType::Encrypted`, but rather they cannot be 
treated as `StorageVolume` (line :728).
  This isn't my area of expertise and I don't know what behavior should be 
expected from different kinds of StorageAccess devices.
  Maybe if we got broader information about it, we might prepare some tests as 
well, so we don't run into problems with this code in the future.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 equal `encrypted` for LUKS encrypted volumes.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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`.
  Is this fine?
  Should we take something else into consideration?
  Do we want that feature tested to avoid regression in the future?

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 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`.
  >
  >
  > The whole block can never return true, so it should just be removed, along 
with all its dependencies.
  
  
  I tested it once more and it returns true when it should, as expected. What 
makes you think it doesn't?
  
  >> Is this fine?
  >>  Should we take something else into consideration?
  >>  Do we want that feature tested to avoid regression in the future?
  > 
  > This code duplicates functionality already present in the thumbnailer code 
in KIO core. It can be replaced by a trivial "CacheThumbnail" flag provided by 
the caller.
  
  I wasn't able to prevent catching of directory thumbnails from KIO. The fact 
that files used for making a preview can be on different storage, makes it 
extra tricky.
  But I'm new the project, so if you do know how to make it simpler I'm all 
ears.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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.
  > >
  > > 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 equal `encrypted` for LUKS encrypted volumes.
  >
  >
  > At a glance the problem here is actually that depending on the setup the 
mounted volume isn't necessarily the encrypted volume. e.g. if you make a LUKS 
encrypted btrfs on /dev/sda1 then sda1 is type LUKS encrypted, but to actually 
use it that device gets decrypted and mapped into a different name 
/dev/mapper/luks-123 which is type btrfs and not encrypted. It is only the 
mapped one that gets mounted which is why the encryption context gets lost 
along the way.
  >
  > I'm afraid this will need some adjustment in solid because currently we 
don't carry that information anywhere that I can see. It is however in the data 
of udiks2 behind the scenes (dbus property 
org.freedesktop.UDisks2.Block.CryptoBackingDevice which is a dbuspath of the 
encrypted backing object) so it should be readily available, just needs putting 
somewhere in solid. This is also represented in the sysfs through a slave 
relationship between the two block devices, but I'm guessing using that over 
udiks2 isn't nearly as portable.
  >  Where to put it I don't really know, probably a new method on one of the 
interface classes CryptoBackingUDI which returns the UDI representing the 
backing device.
  
  
  Could this be delegated onto someone who knows Solid or should I try figuring 
it out by myself?
  
  > On an entirely unrelated note I for one would simplify the 
sharesFilesystemWithThumbRoot to check if the thumb root is encrypted and 
always cache the thumb if that is the case. Whether or not the devices are the 
same seems unnecessarily nitpicky so long as the results are encrypted if the 
originals were. Otherwise you run into awkward cases where a users has a system 
drive and a data drive, both encrypted, but because they are different we 
effectively disable all thumbnail caching for the data drive.
  
  I was thinking about that, but decided against it on the slim chance that 
person having access to encrypted home might not necessarily have same access 
to a vault or other volume that got mounted on the system, by for example 
different user of the same machine.

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, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, because I had 
problems mounting it.

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, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 
> against the possibility of 0.

"-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 0x or if it's more complicated.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 is correct -- at least, 
> that we are told if it ever happens to be incorrect, by adding 
> assert(baseStat.st_dev != 0) after the first lstat succeeds, and 
> assert(fileStat.st_dev != 0) after the second lstat succeeds.

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.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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.

Oh, I missed it, because types.h it's not explicitly included. I also removed 
lstat usagen on Windows. Thanks for pointing that out.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 indeed. Any chance to move this to gitlab? 
> (see invent.kde.org)

On Windows this check is assumed false. So in case encrypted file shares 
filesystem with thumbnails directory, it can take longer as those won't be 
cached, but it will work.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 change anything. m_thumbnailDirDeviceId will remain zero only if lstat 
returns error and as long it's zero sharesFilesystemWithThumbRoot will return 
false.

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, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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.

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28745?vs=83353=83354

REVISION DETAIL
  https://phabricator.kde.org/D28745

AFFECTED FILES
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 code but 
I'm not able to get StorageVolume instance for given file/directory. Could 
someone help me with that?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 check if symlinks are in the path
QFileInfo fileInfo = QFileInfo(path);
if (!fileInfo.exists()) {
//TODO error handling
}
QSet checked; //To avoid weird infinete loops
checked.insert(fileInfo.path());
while (fileInfo.isSymLink()) {
fileInfo = QFileInfo(fileInfo.symLinkTarget());
if (checked.contains(fileInfo.path())) {
//TODO error handling
}
checked.insert(fileInfo.path());
}
QDir dir = fileInfo.dir();
QString canonPath = dir.canonicalPath();
QList list = 
Solid::Device::listFromType(DeviceInterface::Type::StorageAccess);
Device match;
int match_length = 0;
for (Device device: list) {
StorageAccess *storageAccess = device.as();
if (canonPath.startsWith(storageAccess->filePath()) && 
storageAccess->filePath().size() > match_length) {
match_length = storageAccess->filePath().size();
match = device;
}
}
return match;
}
  
  and in the thumbnail.cpp:
  
Solid::Device device = Solid::Device::storageAccessFromPath(filePath);
if (device.is()) {
allowCache = device.as()->usage() != 
Solid::StorageVolume::UsageType::Encrypted;
}
  
  It works, but it might be better to hold tree structure with all StorageAcces 
devices where position on tree would be determined by mountpoint of device, 
which would allow us to go straight to the desired StorageAccess without 
checking all of them.
  On the other hand, to do that we would have to slice path into parts and 
compare those parts with nodes on the tree and also create special structure 
for that. I'm not sure which approach is better.
  
  Another problem with storing StorageAccess tree is that I looked at 
DeviceManager classes and it might be too difficult for me, as I don't know 
Solid well.
  
  Which should it be and do we go about it?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 Device::findByPath(QString)` or rather 
`StorageVolume StorageVolume::findByPath(QString)`?
  
  For implementation itself I want to create structure with mountpoints and 
StorageVolumes which will be updated if new Device is added/removed and we 
learn this via Solid notifications.
  I am thinking it should either be part of `DeviceManagerStorage` or separate 
class similar to DeviceManagerStorage. Not sure which.
  
  I don't know how to get a mountpoint for StorageVolume.
  
  What do you think about it?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 use that.
  > >
  > > `storageAccessFromPath` converts given path into canonical. I figured it 
should do so anyway, so I'm not doing it again here.
  >
  >
  > And thats wrong. You need the canonical path in the thumbnailer itself.
  
  
  Ok. Should this be done here in `createSubThumbnail` or earlier so the whole 
object uses canonical path?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 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.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, thiago, bruns, meven, ngraham, kde-frameworks-devel, 
kfm-devel, badbunny, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, 
fprice, LeGast00n, koehn, cblack, fbampaloukas, alexde, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, rdieter, 
mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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, azyx, nikolaik, pberestov, iasensio, aprcela, 
fprice, LeGast00n, koehn, cblack, fbampaloukas, alexde, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, rdieter, 
mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Marcin Gurtowski]

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 
invent.kde.org and link it here.

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, azyx, nikolaik, pberestov, iasensio, aprcela, 
fprice, LeGast00n, koehn, cblack, fbampaloukas, alexde, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, emmanuelp, rdieter, 
mikesomov