D28745: Skip caching thumbnails on encrypted filesystems

2021-02-24 Thread 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

2021-02-24 Thread Nathaniel Graham
ngraham added a comment.


  Cool, thanks! Can you go to Add Action > Abandon?

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

2021-02-24 Thread 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

2021-02-23 Thread Nathaniel Graham
ngraham added a comment.


  OK great! Thanks for your patience through this very long process. :)

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

2021-02-23 Thread 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


D28745: Skip caching thumbnails on encrypted filesystems

2021-02-23 Thread Nathaniel Graham
ngraham added a comment.


  Is this unblocked now that 
https://invent.kde.org/frameworks/solid/-/merge_requests/19 has been merged?

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

2020-10-18 Thread 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

2020-10-09 Thread Harald Sitter
sitter added a comment.


  > Could this be delegated onto someone who knows Solid or should I try 
figuring it out by myself?
  
  Alas, I'm not sure there is anyone who feels responsible enough for solid, so 
you'll probably have to give it a go yourself.

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

2020-10-08 Thread 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

2020-10-08 Thread Harald Sitter
sitter added a comment.


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

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

2020-10-07 Thread 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

2020-09-26 Thread 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

2020-09-14 Thread 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

2020-09-05 Thread 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

2020-09-04 Thread Thiago Macieira
thiago added a comment.


  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.

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

2020-09-04 Thread Stefan Brüns
bruns added a comment.


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

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

2020-09-04 Thread 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

2020-09-04 Thread Stefan Brüns
bruns added a comment.


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

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

2020-09-04 Thread 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

2020-09-02 Thread 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

2020-09-01 Thread Stefan Brüns
bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  @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.
  
  Second, I have asked for a full context diff, or even better moving this to 
invent.kde.org, but @marcingu keeps ignoring this.
  
  So a full NACK from me.

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

2020-08-29 Thread Nathaniel Graham
ngraham added a comment.


  @bruns and @meven?

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

2020-08-29 Thread David Faure
dfaure accepted this revision.

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

2020-08-27 Thread 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

2020-08-27 Thread 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

2020-08-27 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> thumbnail.cpp:741
> +bool allowCache;
> +allowCache = sharesFilesystemWithThumbRoot(filePath);
> +if (!allowCache) {

join with previous line (this isn't C) ;)

> thumbnail.h:95
> +#ifndef Q_OS_WIN
> +static const dev_t deviceIdUnset = std::numeric_limits::max();
> +dev_t m_thumbnailDirDeviceId = deviceIdUnset;

Suggestion: name it s_deviceIdUnset (s for static, to match m for member).

This makes things more readable when looking at the .cpp file (looks like a 
local variable otherwise).

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

2020-08-25 Thread 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

2020-08-25 Thread 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

2020-08-25 Thread 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

2020-08-25 Thread 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

2020-08-25 Thread 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

2020-08-25 Thread 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

2020-08-24 Thread Stefan Brüns
bruns added inline comments.

INLINE COMMENTS

> thumbnail.cpp:62
> +#include 
> +#include 
>  

What are all these includes for?

> thumbnail.cpp:563
>  
> -if (validThumbnails > 0 && hadFirstThumbnail == dir.filePath()) {
> +if (validThumbnails > 0 && hadFirstThumbnail == 
> canonicalFilePath) {
>  break; // Never show the same thumbnail twice

Obviously broken, as hadFirstThumbnail is not set from a canonical path ..

Or not so obvious, as this is still lacking context.

> thumbnail.cpp:743
> +allowCache = device.as()->usage() 
> != Solid::StorageVolume::UsageType::Encrypted;
> +}
> +}

Why is all this code called even when ThumbCreator::create fails?

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

2020-08-24 Thread 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

2020-08-24 Thread 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

2020-08-23 Thread Thiago Macieira
thiago added inline comments.

INLINE COMMENTS

> marcingu wrote in thumbnail.cpp:776
> "-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.

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.

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

2020-08-23 Thread 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

2020-08-22 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> marcingu wrote in thumbnail.cpp:776
> 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.

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

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

2020-08-22 Thread 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

2020-08-22 Thread 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

2020-08-22 Thread 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

2020-08-22 Thread 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

2020-08-21 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> marcingu wrote in thumbnail.cpp:776
> 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.

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.

> marcingu wrote in thumbnail.h:93
> 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.

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.

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

2020-08-20 Thread 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

2020-08-20 Thread 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

2020-08-20 Thread 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

2020-08-20 Thread 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

2020-08-20 Thread 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

2020-08-19 Thread David Faure
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> marcingu wrote in thumbnail.cpp:776
> 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.

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?

> thumbnail.cpp:779
> +if (lstat(QFile::encodeName(m_thumbBasePath).data(), ) == 
> -1) {
> +qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about 
> filesystem";
> +return false;

print out the path in the warning (same on line 786)

> thumbnail.cpp:785
> +struct stat fileStat;
> +if(lstat(QFile::encodeName(path).data(), ) == -1) {
> +qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about 
> filesystem";

missing space after `if`

s/data/constData/

> thumbnail.h:93
>  qint64 m_maxFileSize;
> +dev_t m_thumbnailDirDeviceId = 0;
>  };

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)

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

2020-08-17 Thread Nathaniel Graham
ngraham added a subscriber: dfaure.
ngraham added a comment.


  @dfaure, what do you think here?

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
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

2020-08-16 Thread 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

2020-08-16 Thread 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

2020-07-04 Thread 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

2020-07-01 Thread 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

2020-06-30 Thread Stefan Brüns
bruns added a comment.


  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.

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

2020-06-30 Thread 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

2020-06-29 Thread Stefan Brüns
bruns added a comment.


  In D28745#675685 , @marcingu wrote:
  
  >   Solid::Device device = Solid::Device::storageAccessFromPath(filePath);
  >   if (device.is()) {
  >   allowCache = device.as()->usage() != 
Solid::StorageVolume::UsageType::Encrypted;
  >   }
  >
  
  
  How often do I have to repeat the thumbnailer has to use the canonical path 
anyway? Please use 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

2020-06-29 Thread Thiago Macieira
thiago added a comment.


  >   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;
  >   }
  >
  
  This search is affected by the same sibling match bug that QStorageInfo was. 
See https://bugreports.qt.io/browse/QTBUG-49498.

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

2020-06-29 Thread 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

2020-06-11 Thread Méven Car
meven added a comment.


  In D28745#675033 , @marcingu wrote:
  
  > 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?).
  
  
  I don't think so, Storage Volume is any block device.
  This function may fail if the input path is not valid for instance.
  
  > Should it be `StorageVolume Device::findByPath(QString)` or rather 
`StorageVolume StorageVolume::findByPath(QString)`?
  
  My preference is `StorageVolume Device::findByPath(QString)`, to keep all 
entry points to Solid in Device and somewhat similar to the `Device::listFrom*` 
functions.
  
  > 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.
  
  Yeah that's what I imagined as well, but this is already taken care of by the 
base DeviceManager.
  
  > I am thinking it should either be part of `DeviceManagerStorage` or 
separate class similar to DeviceManagerStorage. Not sure which.
  
  Either way you can simply wrap the base DeviceManager.
  Your method should only be a utility function not adding new Structures, 
everything is in Solid already.
  
  > I don't know how to get a mountpoint for StorageVolume.
  
  You make me realize you should filter StorageVolume (any block device) that 
are also StorageAccess (anything that can be mounted) which has the mountpoints 
in `filePath()`.
  
  https://api.kde.org/frameworks/solid/html/classSolid_1_1StorageVolume.html 
  https://api.kde.org/frameworks/solid/html/classSolid_1_1StorageAccess.html
  
  > What do you think about it?
  
  I like 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

2020-06-11 Thread 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

2020-06-10 Thread Stefan Brüns
bruns added a comment.


  In D28745#674863 , @meven wrote:
  
  > In D28745#674827 , @bruns wrote:
  >
  > > In D28745#674711 , @meven 
wrote:
  > >
  > > >
  > >
  > >
  > >
  > >
  > > > Solid does not provide straight `folder => StorageVolume` yet, but I 
think Solid could have such a utility feature added.
  > > >  Something like `Solid::Device::findByPath()`, it would need to 
canonically and recursively resolves the path parent to pay attention to 
symlinks.
  > > >  This would also help D26407 
  > >
  > > No recursion needed, `stat` provides the device.
  >
  >
  > Only when the file is not a symlink, if so we need to check the symlink 
target recursively, that's what I meant.
  
  
  Haven't checked the code here en detail, but the whole thumbnailing code has 
to work on the resolved symlinks anyway - for the data, for the thumbnail 
filename [1], and for the save policy.
  
  [1] 
https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html#THUMBSAVE
  
  > 1. You need the **absolute canonical URI** for the original file, as stated 
in URI RFC 2396. In particular this defines to use three '/' for local 'file:' 
resources (see example below).
  > 2. Calculate the MD5 hash for this URI. Not for the file it points to! This 
results in a 128bit hash, which is representable by a hexadecimal number in a 
32 character long string.
  
  
  
  >> And can you please use arc to upload the patch - it is nearly impossible 
to do a review with the missing context
  > 
  > Or pushing it to https://community.kde.org/Infrastructure/GitLab
  
  That would even be better, yes.

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

2020-06-07 Thread Méven Car
meven added a comment.


  In D28745#674827 , @bruns wrote:
  
  > In D28745#674711 , @meven wrote:
  >
  > >
  >
  >
  >
  >
  > > Solid does not provide straight `folder => StorageVolume` yet, but I 
think Solid could have such a utility feature added.
  > >  Something like `Solid::Device::findByPath()`, it would need to 
canonically and recursively resolves the path parent to pay attention to 
symlinks.
  > >  This would also help D26407 
  >
  > No recursion needed, `stat` provides the device.
  
  
  Only when the file is not a symlink, if so we need to check the symlink 
target recursively, that's what I meant.
  
  > And can you please use arc to upload the patch - it is nearly impossible to 
do a review with the missing context
  
  Or pushing it to https://community.kde.org/Infrastructure/GitLab

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

2020-06-07 Thread Stefan Brüns
bruns added a comment.


  And can you please use arc to upload the patch - it is nearly impossible to 
do a review with the missing context

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

2020-06-07 Thread Stefan Brüns
bruns requested changes to this revision.

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

2020-06-07 Thread Stefan Brüns
bruns added a comment.


  In D28745#674711 , @meven wrote:
  
  >
  
  
  
  
  > Solid does not provide straight `folder => StorageVolume` yet, but I think 
Solid could have such a utility feature added.
  >  Something like `Solid::Device::findByPath()`, it would need to canonically 
and recursively resolves the path parent to pay attention to symlinks.
  >  This would also help D26407 
  
  No recursion needed, `stat` provides the device.

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

2020-06-06 Thread Méven Car
meven added a comment.


  In D28745#674294 , @marcingu wrote:
  
  > 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.
  
  
  The good starting point would be the main Solid Tutorial : 
  https://techbase.kde.org/Development/Tutorials/Solid/Device_Discovery
  
  Solid does not provide straight `folder => StorageVolume` yet, but I think 
Solid could have such a utility feature added.
  Something like `Solid::Device::findByPath()`, it would need to canonically 
and recursively resolves the path parent to pay attention to symlinks.
  This would also help D26407 
  
  KMountPoint::findByPath has it although it is not perfect. Compared to solid 
it does not refresh automatically and can't send signals and does not know 
about encryption.
  
  > Could someone help me with that?
  
  Sure. I have been dreaming about such feature for a while.

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

2020-06-06 Thread 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

2020-05-31 Thread 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

2020-05-09 Thread Stefan Brüns
bruns added a comment.


  In D28745#667225 , @thiago wrote:
  
  > Use Solid and ask it if the device is on an encrypted device. If Solid does 
not have such an API, add it.
  
  
  I fully second 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

2020-05-09 Thread Thiago Macieira
thiago added a comment.


  Use Solid and ask it if the device is on an encrypted device. If Solid does 
not have such an API, add 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

2020-05-09 Thread Stefan Brüns
bruns added a comment.


  I think the patch as is has some structural problems. I think it would be 
significantly cleaner when:
  
  - the "cacheAllowed" checking code is moved into a separate function
  - the global `allowCache` variable is removed, and the value is passed into 
`createSubThumbnail(...)`/`drawSubThumbnail(...)` as parameter
  
  You can then call the check function once on the directory, at the same place 
the `QDirIterator` is instantiated (line 538).
  
  And can you please use `arc` for uploading the patch, it is really hard to do 
a review without context.

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

2020-05-09 Thread 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

2020-05-03 Thread Thiago Macieira
thiago added inline comments.

INLINE COMMENTS

> thumbnail.cpp:743
> +const auto mount =  mountsList.findByPath(filePath);
> +allowCache = !(mount->mountType() == 
> QLatin1String("fuse.cryfs") || mount->mountType() == 
> QLatin1String("fuse.encfs"));
> +}

How about encrypted block storage, which is superior to those encrypted fs and 
the recommended solution?

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

2020-05-03 Thread Méven Car
meven requested changes to this revision.
This revision now requires changes to proceed.

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

2020-05-03 Thread Méven Car
meven added inline comments.

INLINE COMMENTS

> thumbnail.cpp:738
> +#else
> +allowCache = false;
> +#endif

There is an error with the previous line, one of the two should be 
allowDirCached I think.
Here you always ignore result of `sharesFilesystemWithThumbRoot`

> thumbnail.cpp:776
> +struct stat baseStat;
> +if (!lstat(QFile::encodeName(m_thumbBasePath).data(), )) {
> +m_thumbnailDirDeviceId = baseStat.st_dev;

`!= -1`,  Add a warning with errno should it fail.

> thumbnail.cpp:781
> +struct stat fileStat;
> +return m_thumbnailDirDeviceId && !lstat(QFile::encodeName(path).data(), 
> ) && m_thumbnailDirDeviceId == fileStat.st_dev;
> +}

error check here for `!lstat(QFile::encodeName(path).data(), )` is 
important, file might have moved for instance.

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

2020-05-02 Thread Nathaniel Graham
ngraham accepted this revision.
ngraham added a comment.


  @bruns?

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

2020-05-02 Thread 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

2020-04-26 Thread 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

2020-04-26 Thread 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

2020-04-26 Thread 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

2020-04-26 Thread Méven Car
meven added inline comments.

INLINE COMMENTS

> thumbnail.cpp:738
> +if (!m_thumbnailDirDeviceId) {
> +struct stat baseStat;
> +if (!lstat(QFile::encodeName(m_thumbBasePath).data(), 
> )) {

Can't you move this out of the loop ? Since `m_thumbBasePath` does not change.

> thumbnail.h:87
> +dev_t m_thumbnailDirDeviceId = 0;
> +short allowDirCatche;
>  };

s/allowDirCatche/allowDirCached

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

2020-04-25 Thread 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

2020-04-25 Thread 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

2020-04-19 Thread 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

2020-04-19 Thread 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


D28745: Skip caching thumbnails on encrypted filesystems

2020-04-18 Thread Stefan Brüns
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> thumbnail.cpp:731
> +struct stat baseStat, fileStat;
> +if (!lstat(m_thumbBasePath.toStdString().c_str(), ) && 
> lstat(filePath.toStdString().c_str(), )) {
> +allowCache = baseStat.st_dev == fileStat.st_dev;

Please do the stat for m_thumbBasePath just once.

Also, toStdString is wrong, use the string encoding functions from `QFIle`.

> thumbnail.cpp:737
> +// If file is on encrypted filesystem different than 
> thumbnail directory, we can't cache it
> +const auto mountsList = KMountPoint::currentMountPoints();
> +const auto mount =  mountsList.findByPath(filePath);

This will have some impact when creating thumbnails for any external or 
secondary storage (i.e. everything not in the HOME partition).

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

2020-04-18 Thread 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

2020-04-18 Thread 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

2020-04-16 Thread 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

2020-04-16 Thread Stefan Brüns
bruns added inline comments.

INLINE COMMENTS

> thumbnail.cpp:726
>  
> +const auto mountsList = KMountPoint::currentMountPoints();
> +const auto mount =  mountsList.findByPath(filePath);

Calling currentMountPoints() for every created file is definitely a bad idea ...

https://github.com/KDE/kio/blob/master/src/core/kmountpoint.cpp#L272

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

2020-04-16 Thread Stefan Brüns
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> thumbnail.cpp:729
> +const auto thumbRootMount = 
> mountsList.findByPath(m_thumbBasePath);
> +//If file is on the same filesystem as thumbnail directory, we 
> can cache it.
> +const bool isEncrypted = mount != thumbRootMount && 
> (mount->mountType() == QLatin1String("fuse.cryfs") || mount->mountType() == 
> QLatin1String("fuse.encfs"));

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

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

2020-04-16 Thread Méven Car
meven accepted this revision.
meven added a comment.


  In D28745#649011 , @marcingu wrote:
  
  > 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.
  
  
  AFAICT this is not clear at the moment. This is too bad it stumbles upon a 
new contributor to deal with it.
  I recently learned about QStorageInfo that covers the same use case as 
KMountPoint.
  
  > 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.
  
  We should avoid absolutely avoid that, this is far worse than saving 1 
syscall per thumbnail.
  
  And after a second though a sysall is not much given we are generating a 
thumbnail that will be far longer to compute, so the price of the syscall 
should be unnoticeable.

INLINE COMMENTS

> thumbnail.cpp:729
> +const auto thumbRootMount = 
> mountsList.findByPath(m_thumbBasePath);
> +//If file is on the same filesystem as thumbnail directory, we 
> can cache it.
> +const bool isEncrypted = mount != thumbRootMount && 
> (mount->mountType() == QLatin1String("fuse.cryfs") || mount->mountType() == 
> QLatin1String("fuse.encfs"));

Add a space after //

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven
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

2020-04-15 Thread 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

2020-04-14 Thread Méven Car
meven added a comment.


  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.

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

2020-04-13 Thread 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


D28745: Skip caching thumbnails on encrypted filesystems

2020-04-13 Thread 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

2020-04-13 Thread Nathaniel Graham
ngraham retitled this revision from "Skipping catching of thumbnails on 
encrypted filesystems" to "Skip caching thumbnails on encrypted filesystems".
ngraham edited the summary of this revision.

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