D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  In D17816#677552 , @ngraham wrote:
  
  > What is the status of this? How do we move forwards? I know this has gone 
on a long time and we're all getting tired, but I think we can push this past 
the finish line without too much trouble, hopefully. :)
  
  
  This has landed long time ago. Unfortunately, in combination with SELINUX 
getfattr returns an error not mentioned in any man page, which causes an 
inifinite loop in the current code.
  
  Fixed by https://invent.kde.org/frameworks/kio/-/merge_requests/368

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  In D17816#677455 , @kdudka wrote:
  
  > @bruns I find your attitude unnecessarily hostile.  If you think that the 
code is perfect as it is, feel free to patch it case by case until it 
eventually works for everybody.  That is your choice.
  
  
  I never said its perfect. There is exactly one case which was missed (and 
only in this one location, for both listxattr and setxattr a return value of -1 
is handled correctly).
  
  > I do not think that KDE users will appreciate some **blindly** coded 
**fancy features** on top of that when the basic functionality gets **totally 
broken** as a result of these **experiments**.
  
  Such well chosen words ...
  
  Btw, there are plenty of more cases where kio deliberately deviates from the 
behavior of low level system tools.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  In D17816#677453 , @kdudka wrote:
  
  > I do not think we have to.  Please have a look at how attr_copy_fd() from 
 is implemented: 
http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_fd.c?id=a4187bec#n111
  This code is quite mature and it is used by cp(1) on GNU/Linux for instance.  
I do not think that KDE users will appreciate some blindly coded fancy features 
on top of that when the basic functionality gets totally broken as a result of 
these experiments.  Please keep the code simple and reliable.
  
  
  Its not blindly coded. Please keep your arrogant comments to yourself.
  
  And obviously, there is something strange going on on your system - 
flistxattr returns a list of keys, but fgetxattr then returns ENOTSUP?

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  In D17816#677451 , @kdudka wrote:
  
  > The man page says that one has to check return value of the second call.  
It does not say that the function needs to be called in a loop indefinitely 
until it succeeds.
  
  
  And if the second call then returns ERANGE you have to start from the 
beginning.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  In D17816#677449 , @kdudka wrote:
  
  > Even after applying the proposed patch, the code still looks problematic to 
me.  I would prefer to have it explained first.  When fgetxattr(..., 0) returns 
-1/ERANGE, what is the point of calling fgetxattr(..., 0) again?  It is still 
going to busy-loop indefinitely in this case, doesn't it?  How many times do we 
actually need to call fgetxattr() on a single file descriptor?  Twice?  Then 
unbounded loop is not the best construction to begin with.
  
  
  Ever heard of a TOCTOU race?
  
  Quoting from man 2 getxattr:
  
  > If size is specified as zero, these calls return the current size of the 
named extended attribute (and leave value unchanged).  This can be used to 
determine the size of the buffer that should be supplied in a subsequent call.  
 (But,  bear  in  mind  that  there is a possibility that the attribute value 
may change between the two calls, so that it is still necessary to check the 
return status from the second call.)

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added a comment.


  Looks ok to me now. Can someone else please double check?

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D23694: Add support for sshfs to the fstab backend

[Stefan Brüns]

bruns accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

D23694: Add support for sshfs to the fstab backend

[Stefan Brüns]

bruns added a comment.


  One possibility to hide it would be the `x-gvfs-*` options, though I am not 
sure if it is possible to pass these to fuse.

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> jobtest.cpp:471
> +
> +void JobTest::setXattr(const QString )
> +{

should be `dest`.

> jobtest.cpp:489
> +qDebug() << resultdest;
> +m_SkipXattr = true;
> +}

Should have a `break` or `return` here.

> arrowd wrote in jobtest.cpp:780
> To be honest, I was confused by your first comment. What was wrong with the 
> first version of this code?
> 
> My understanding was that we want to skip xattr stuff if either source or 
> destination doesn't support it

`setXattr`, called from `checkXattrFsSupport`, unconditionally sets 
`m_SkipXattr`. 
You want something like

  bool canRead =  checkXattrFsSupport(homeDir);
  bool canWrite =  checkXattrFsSupport(otherHomeDir);
  if (canRead && canWrite) {

and get rid of the `m_SkipXAttr` variable.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> bruns wrote in jobtest.cpp:780
> This will set `m_SkipXattr` unconditionally even if the source FS does not 
> support Xattrs.

And now you **only** check the source FS, but no longer the destination FS.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> jobtest.cpp:467
> +path.removeLast();
> +QString writeTest = path.join("/") + "/fsXattrTestFile";
> +createTestFile(writeTest);

const QString

And if you just call this with the target directory (e.g. ` homeTmpDir()`), you 
can skip the split/join dance.

> jobtest.cpp:780
> +checkXattrFsSupport(filePath);
> +checkXattrFsSupport(dest);
> +if (!m_SkipXattr) {

This will set `m_SkipXattr` unconditionally even if the source FS does not 
support Xattrs.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added a comment.


  In D29526#676402 , @meven wrote:
  
  > In D29526#676386 , @bruns wrote:
  >
  > > For all but text the DPR is completely irrelevant, large@1 is identical 
to normal@2.
  >
  >
  > Yes and that's up to thumbnail creators to decide. To take advantage of 
this, we would need to introduce some ThumbnailCreator type that would say 
whether or not generated thumbnail might be influenced by DPR (i.e) text. That 
would necessitate change the ThumbnailCreator API.
  
  
  No, just add a "DisplayPixelRatioDependent=true"  key to the service data of 
the thumbnailer.
  
  > But the implementation will stay a lot simpler if we don't this level of 
complexity and adding will have a limited interest. Storing twice large@1 would 
happen only when a user would change DPR, thumbnails cache size limit will stay 
enforced.
  
  You are changing every single thumbnailer implementation, although the 
results are binary identical for normal@2 and large@1. And that are only the 
thumbnailers in kio-extra.
  
  And on top of all this, the only thumbnailer which is DPR dependent (text) 
does not even cache the data on disk.

REPOSITORY
  R320 KIO Extras

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Stefan Brüns]

bruns added a comment.


  In D29397#663902 , @broulik wrote:
  
  > > At the risk of asking a stupid question, why?
  >
  > The text thumbnailer should be able to produce readable text on high dpi, 
or the folder thumbnailer should render the folder icon sharply and the picture 
frames non-pixelated
  
  
  The folder thumbnailer does not care for DPR, the folder backdrop is rendered 
at full native resolution, and the sub-thumnails are rendered with a scale 
~0.45 on top.
  
  The text thumbnailer currently renders previews in the file list with a 
height of either 256 or 128, and then scales it down. For an icon size of e.g. 
64 the 128px thumb is scaled down, so the effective DPR becomes nativeDpr * 
0.5. Assuming a native DPR of 2.0 (HiDPI), should the text in this case be 
rendered with a DPR of 1.0 or 2.0? One will be blurry, and the other one will 
hardly contain any text at all (how many characters can you squeeze into 
48x64px?).
  
  For dolphin, there is actually another reason for blurriness. The thumbnail 
icons are scaled so after adding the drop shadow the icon has the selected size 
- at 256px, the 256px thumbnail is scaled to 250px. For natural images (Photos, 
Video thumbnails, cover images ...) this is hardly visible, but very noticeable 
for text.

REPOSITORY
  R241 KIO

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> imagecreator.cpp:48
> +ir.setQuality(75); // set quality so that the jpeg handler will use a 
> high quality downscaler
> +
>  img = ir.read();

This whole hunk should not be part of this submission. Also, the comment 
("jpeg") is likely wrong.

REPOSITORY
  R320 KIO Extras

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

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

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> thumbnail.cpp:143
>  
> +bool createThumbnail(ThumbCreator *creator, const QString , int width, 
> int height, QImage , qreal devicePixelRatio = 1.0)
> +{

Has to be static or in an anonymous namespace.

> thumbnail.cpp:149
> +} else {
> +return creator->create(path, width, height, img);
> +}

`width * dpr, height * dpr` gives the wanted result even without explicit 
support for DPR for the majority of the thumbnailer.

REPOSITORY
  R320 KIO Extras

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

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

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  For all but text the DPR is completely irrelevant, large@1 is identical to 
normal@2.

REPOSITORY
  R320 KIO Extras

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Stefan Brüns]

bruns added a comment.


  Most thumbnailers are completely DPR agnostic, and will create identical 
thumbnails for large@1 and normal@2. Having both is a waste of disk space and 
CPU time.
  
  The thumbnailer should have a key in its metadata to tell if it depends on 
the DPR, and only then different thumbnails should be created.

INLINE COMMENTS

> previewjob.cpp:699
>  }
> -shmid = shmget(IPC_PRIVATE, cacheWidth * cacheHeight * 4, IPC_CREAT 
> | 0600);
> +shmid = shmget(IPC_PRIVATE, cacheWidth * cacheHeight * 4 * 
> devicePixelRatio * devicePixelRatio, IPC_CREAT | 0600);
>  if (shmid != -1) {

mixing floating point and integers for a memory-allocation like function is a 
real bad idea.

REPOSITORY
  R241 KIO

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D28745: Skip caching thumbnails on encrypted filesystems

[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

[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

[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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> file_unix.cpp:583
> +qCDebug(KIO_FILE) << "the file don't have any xattr";
> +return false;
> +}

Should be `true`

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D28745: Skip caching thumbnails on encrypted filesystems

[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

[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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> bruns wrote in jobtest.cpp:487
> this obviously needs test cases with a key ley/value len > 512, to check the 
> the array-to-short/resize path.

not done ...

> file_unix.cpp:620
> +#endif
> +qDebug(KIO_FILE) << valuelen << " " << key << " " << value;
> +if (valuelen > 0 && value.size() == 0) {

No extra spaces for qDebug, inserts spaces itself.

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D28745: Skip caching thumbnails on encrypted filesystems

[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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> arrowd wrote in file_unix.cpp:625
> Why? `ERANGE` means we need to come up with new value for `valuelen`, so we 
> set it to zero and start over. On the new iteration it gets passed into 
> `fgetxattr`/`extattr_get_fd` to find out sufficient value.

"0" is a regular return value. Try your code with the following file:

  touch t
  setfattr -n user.foo -v "" t

and preferably add a test case ...

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> file_unix.cpp:580
> +if (listlen == 0) {
> +qCDebug(KIO_FILE) << "file" << src_fd << "don't have any xattr";
> +return false;

`src_fd` is quite meaningless as debug output

> file_unix.cpp:625
> +continue;
> +}
> +} while (true);

Infinite loop on valuelen == 0

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D29207: [Indexers] Ignore name-based mimetype for initial indexing decisions

[Stefan Brüns]

bruns closed this revision.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28745: Skip caching thumbnails on encrypted filesystems

[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

D29207: [Indexers] Ignore name-based mimetype for initial indexing decisions

[Stefan Brüns]

bruns edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28745: Skip caching thumbnails on encrypted filesystems

[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

[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

[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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> file_unix.cpp:142
> +ssize_t listlen = 0;
> +QByteArray keylist(listlen, Qt::Uninitialized);
> +do {

just `QByteArray keylist;`

> file_unix.cpp:143
> +QByteArray keylist(listlen, Qt::Uninitialized);
> +do {
> +keylist.resize(listlen);

`while (true)` instead of `do {} while(true)`

> file_unix.cpp:164
> +if (errno == ENOTSUP) {
> +qCDebug(KIO_FILE) << "source filesystem don't support 
> xattrs";
> +}

"does not"

> file_unix.cpp:175
> +keylist.resize(listlen);
> +keylist.squeeze();
> +

why `.squeeze()` ?

> file_unix.cpp:177
> +
> +// Linux and MacOS return = list of null terminated string, each string 
> = [data,'\0']
> +// BSD return = list of items, each item prepended of 1 byte size = 
> [size, data]

"return a list of null terminated strings"

> file_unix.cpp:178
> +// Linux and MacOS return = list of null terminated string, each string 
> = [data,'\0']
> +// BSD return = list of items, each item prepended of 1 byte size = 
> [size, data]
> +QByteArray::const_iterator keyPtr = keylist.cbegin();

"BSDs return a list of items, ..." or "BSD returns a list of items, ..."
"..., each item consisting of the size byte prepended to the key."

> file_unix.cpp:185
> +while (keyPtr != keylist.cend()) {
> +// Get the size of key
> +#if HAVE_SYS_XATTR_H

"key size" or "size of the key"

> file_unix.cpp:226
> +if (destlen == -1 && errno == ENOTSUP) {
> +qCDebug(KIO_FILE) << "Destination filesystem don't support 
> xattrs";
> +return false;

"does not"

REPOSITORY
  R241 KIO

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> file_unix.cpp:206
> +if (valuelen == -1 && errno == ERANGE) {
> +value.resize(valuelen + 512);
> +}

WRONG WRONG WRONG !!!

REPOSITORY
  R241 KIO

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> jobtest.cpp:487
> +attrs["user.fattr.with.a.lot.of.namespaces"] = "true";
> +attrs["user.name with space"] = "value with spaces";
> +return attrs;

this obviously needs test cases with a key ley/value len > 512, to check the 
the array-to-short/resize path.

> file_unix.cpp:155
> +if (errno == ERANGE) {
> +keylist.resize(listlen + 512);
> +}

This is still definitely wrong ...

> file_unix.cpp:200
> +if (valuelen == -1 && errno == ERANGE) {
> +value.resize(valuelen + 512);
> +}

also wrong ...

REPOSITORY
  R241 KIO

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> jobtest.cpp:502
> +// arguments  0:"-n" 1:"name" 2:"-v", 3:"value" 4:src
> +arguments = QStringList {"-n", "", "-v", "", "-h", src};
> +keyIndex = 1;

`std::function 
m_setXattrFormatArgs;`
...

  m_setXattrFormatArgs = [](const QString& attrName, const QString& value, 
const QString& fileName) {
  return QStringList{QLatin1String("-n"), attrName, QLatin1String("-v"), 
value, fileName};
  }

and do it when doing the path lookup, once.

REPOSITORY
  R241 KIO

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

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Stefan Brüns]

bruns accepted this revision.

REPOSITORY
  R245 Solid

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

To: meven, #frameworks, bruns, sitter, dfaure, ngraham
Cc: ngraham, dfaure, broulik, ervin, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

D29810: Don't use the setenv function after fork

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kcrash.cpp:689
>  {
> +static char autorestarted_envvar[] = "KCRASH_AUTO_RESTARTED";
> +char** environ_end;

move this to the lambda scope, you can also make the var name shorter then.

> kcrash.cpp:698
> +}
> +auto end = std::copy(environ, environ_end, environ_data.begin());
> +

`std::copy_if`, instead of a separate `remove_if` pass.

> kcrash.cpp:703
> + return strncmp(autorestarted_envvar, s, 
> sizeof(autorestarted_envvar)-1) == 0 &&
> + s[sizeof(autorestarted_envvar)-1] == '=';
> + });

the part after the `&&` can be removed when you include the '=' in the static 
string

REPOSITORY
  R285 KCrash

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

To: jpalecek, #frameworks
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29597: file extractor: fix linking

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  Your build system seems to be broken. The "missing" methods are statically 
linked, see 
src/file/extractor/CMakeFiles/baloo_file_extractor.dir/baloosettings.cpp.o

REPOSITORY
  R293 Baloo

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

To: eszlari, bruns
Cc: bruns, kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
cblack, fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D28745: Skip caching thumbnails on encrypted filesystems

[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

[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

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:0975ed45af79: [TaglibExtractor] Add support for Audible 
Enhanced Audio audio books (authored by bruns).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29454?vs=82017=82044

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

AFFECTED FILES
  autotests/samplefiles/no-meta/test.aax
  autotests/samplefiles/nocoverage_test.aax
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.json

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

bruns added a comment.


  In D29454#664222 , @ngraham wrote:
  
  > Nice!
  
  
  and when you combine it with 
https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/577, 
Elisa can play Audible audio books ;-)

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Frameworks, Baloo, ngraham, astippich, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  Audible AAX files are standard ISO Base Media File Format (ISOBMFF) files,
  often called "MP4" or "Quicktime". Metadata can thus be extracted with
  the taglib "MP4" code path.
  
  The test files have been created with ffmpeg and then edited with okteta/
  exiftool to resemble real AAX files, which can not be included for
  copyright reasons.
  
  https://gitlab.freedesktop.org/xdg/shared-mime-info/-/merge_requests/76
  is required to correctly detect AAX files.

TEST PLAN
  ctest

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  autotests/samplefiles/no-meta/test.aax
  autotests/samplefiles/nocoverage_test.aax
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.json

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:7605f4d7f7c4: Store filename terms just once (authored by 
bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28932?vs=80439=81937

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

AFFECTED FILES
  autotests/integration/querytest.cpp
  src/engine/termgenerator.cpp
  src/engine/termgenerator.h
  src/file/basicindexingjob.cpp
  src/lib/searchstore.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

bruns added a comment.


  This has been pending for more than two weeks now, without any sort of review 
...
  
  @ngraham If you have any questions, please ask!

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

bruns edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29188: [FileWatch] Remove redundant watchIndexedFolders() slot

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:1793785420b7: [FileWatch] Remove redundant 
watchIndexedFolders() slot (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29188?vs=81201=81347

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  src/file/filewatch.cpp
  src/file/filewatch.h
  src/file/mainhub.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29199: honor the extractMetaData flag

[Stefan Brüns]

bruns accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  honorFlag

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

To: astippich, #baloo, bruns, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29207: [Indexers] Ignore name-based mimetype for initial indexing decisions

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The name based mime type is inaccurate, so it should not be used to
  decide if a file should be indexed. In case a specific extension should
  be skipped this can still be done accurately by the name based filters,
  e.g.  instead of "image/png" "*.png" can be used, or the whole directory
  can be excluded.
  
  This inaccuracy is also confusing for the user, as a file without
  extension will be added to the index, but adding an extension removes
  the file from the index. The file extension may also be ambiguous.
  
  This also matches the current list of excluded mime types, which are
  source files for various languages. These blow up the full text index
  and thus should be excluded (by default), but just adding the file names
  increases the index size only marginally.
  
  The 'inability' to find files is a recurring user complaint.
  
  Depends on D28932 

REPOSITORY
  R293 Baloo

BRANCH
  submit

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

AFFECTED FILES
  src/file/extractor/app.cpp
  src/file/firstrunindexer.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/unindexedfileiterator.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

bruns added a dependent revision: D29207: [Indexers] Ignore name-based mimetype 
for initial indexing decisions.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29199: honor the extractMetaData flag

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> exiv2extractor.cpp:172
>  
> -if (image->pixelHeight()) {
> -result->add(Property::Height, image->pixelHeight());
> -}
> +if (result->inputFlags() & ExtractionResult::ExtractMetaData) {
>  

`if (!) return`

> astippich wrote in taglibextractor.cpp:99
> true, that was still written with D25517  
> and D25515  in mind

Still trivially possible:

  if (savedProperties.isEmpty()) {
  return;
  }
  if (extractFulltext) {
  // extract lyrics
  }
  if (!extractMetadata) {
  return
  }

> bruns wrote in xmlextractor.cpp:103
> correct place for the flags check

skipped, `if (!) { continue; }`

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29191: [FileWatch] Fix watch updates on config changes

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:c76397081d1e: [FileWatch] Fix watch updates on config 
changes (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29191?vs=81206=81243

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  src/file/filewatch.cpp
  src/file/filewatch.h

To: bruns, #baloo, ngraham, meven
Cc: meven, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29189: [KInotify] Fix path matching when removing watches

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:73183acf00a2: [KInotify] Fix path matching when removing 
watches (authored by bruns).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29189?vs=81204=81242#toc

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29189?vs=81204=81242

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp

To: bruns, #baloo, ngraham, meven
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29199: honor the extractMetaData flag

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> office2007extractor.cpp:79
> +
> +if (docPropsEntries.contains(QStringLiteral("core.xml")) && 
> result->inputFlags() & ExtractionResult::ExtractMetaData) {
>  QDomDocument coreDoc(QStringLiteral("core"));

Long line

  bool extractMetaData = ...
  if (extractMetaData && docPropsEntries.contains(QStringLiteral("core.xml")) {

> office2007extractor.cpp:144
>  
> -if (docPropsEntries.contains(QStringLiteral("app.xml"))) {
> +if (docPropsEntries.contains(QStringLiteral("app.xml")) && 
> result->inputFlags() & ExtractionResult::ExtractMetaData) {
>  QDomDocument appDoc(QStringLiteral("app"));

Long line

  if (extractMetaData && docPropsEntries.contains(QStringLiteral("app.xml")) {

> office2007extractor.cpp:185
>  //
>  bool extractPlainText = (result->inputFlags() & 
> ExtractionResult::ExtractPlainText);
>  

... like done here

> taglibextractor.cpp:99
>  {
>  if (savedProperties.isEmpty()) {
>  return;

when you add the flag check here, you can avoid the extra indentation below. 
Likewise in some other places.

> xmlextractor.cpp:103
>  
>  if (e.localName() == QLatin1String("metadata")) {
>  auto rdf = e.firstChildElement(QLatin1String("RDF"));

correct place for the flags check

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29199: honor the extractMetaData flag

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29189: [KInotify] Fix path matching when removing watches

[Stefan Brüns]

bruns edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29189: [KInotify] Fix path matching when removing watches

[Stefan Brüns]

bruns added a dependent revision: D29191: [FileWatch] Fix watch updates on 
config changes.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29191: [FileWatch] Fix watch updates on config changes

[Stefan Brüns]

bruns edited the summary of this revision.
bruns added a dependency: D29189: [KInotify] Fix path matching when removing 
watches.

REPOSITORY
  R293 Baloo

BRANCH
  submit

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

To: bruns, #baloo, ngraham, meven
Cc: meven, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:1a3c23f076d2: [FileWatchTest] Extend coverage to config 
updates (authored by bruns).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29190?vs=81207=81229#toc

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29190?vs=81207=81229

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp

To: bruns, #baloo, ngraham, meven
Cc: meven, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns marked an inline comment as done.

REPOSITORY
  R293 Baloo

BRANCH
  fwtest

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

To: bruns, #baloo, ngraham, meven
Cc: meven, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> meven wrote in filewatchtest.cpp:223
> Maybe less, to keep the test reasonably fast.

Does not really matter as soon as the code is fixed (review pending). Default 
would be 5000.

REPOSITORY
  R293 Baloo

BRANCH
  fwtest

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

To: bruns, #baloo, ngraham, meven
Cc: meven, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns updated this revision to Diff 81207.
bruns added a comment.


  whitespace

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29190?vs=81205=81207

BRANCH
  fwtest

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns added a dependent revision: D29191: [FileWatch] Fix watch updates on 
config changes.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29191: [FileWatch] Fix watch updates on config changes

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  Track the current list of include/exclude folders, and on a config change
  add/remove watches for the differences. Previously, when an include
  folder was removed the corresponding watches were never removed.
  
  Depends on D29190 

TEST PLAN
  ctest -R filewatchtest

REPOSITORY
  R293 Baloo

BRANCH
  submit

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  src/file/filewatch.cpp
  src/file/filewatch.h

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns added a dependent revision: D29189: [KInotify] Fix path matching when 
removing watches.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29189: [KInotify] Fix path matching when removing watches

[Stefan Brüns]

bruns edited the summary of this revision.
bruns added a dependency: D29190: [FileWatchTest] Extend coverage to config 
updates.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29190: [FileWatchTest] Extend coverage to config updates

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  Currently, config changes are not covered in the
  tests, add a test case

TEST PLAN
  ctest -R filewatchtest

REPOSITORY
  R293 Baloo

BRANCH
  fwtest

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29189: [KInotify] Fix path matching when removing watches

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  As in removeWatch the path is passed in with a trailing slash, no path
  is removed.

TEST PLAN
  ctest -R filewatch

REPOSITORY
  R293 Baloo

BRANCH
  fwtest

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  src/file/kinotify.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29188: [FileWatch] Remove redundant watchIndexedFolders() slot

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  updateIndexedFoldersWatches() has the same effect, so there is no need
  for an additional function.
  
  Update the updateIndexedFoldersWatches() API documentation to reflect
  the current use.

TEST PLAN
  ctest

REPOSITORY
  R293 Baloo

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

AFFECTED FILES
  autotests/unit/file/filewatchtest.cpp
  src/file/filewatch.cpp
  src/file/filewatch.h
  src/file/mainhub.cpp

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29181: Use KFileMetaData for XAttr support instead of private reimplementation

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:774bd228b749: Use KFileMetaData for XAttr support instead 
of private reimplementation (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29181?vs=81185=81192

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/lib/baloo_xattr_p.h
  autotests/unit/lib/xattrdetector.cpp
  autotests/unit/lib/xattrdetector.h

To: bruns, #baloo, ngraham, apol
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in fakeopticaldisc.cpp:43
> I was talking about something like Solid::OpticalDrive::MediumType(0); 
> ContentType has Solid::OpticalDisc::NoContent whereas MediumType has nothing 
> similar.
> 
> IIUC named casts are preferred over old style casts, so I suggest either we 
> add a MediumType enumerator e.g. NotSupported = 0x0 or use a static_cast.

Thats not a cast but a constructor, see QFlags documentation

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29181: Use KFileMetaData for XAttr support instead of private reimplementation

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  This removes a lot of duplicate code, and on top it actually works
  as it no longer hardcodes a number of locations (e.g. /tmp/ was always
  blacklisted).

TEST PLAN
  ctest

REPOSITORY
  R293 Baloo

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/lib/baloo_xattr_p.h
  autotests/unit/lib/xattrdetector.cpp
  autotests/unit/lib/xattrdetector.h

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28980: Revert "add Baloo DBus signals for moved or removed files"

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:c2865b207907: Revert add Baloo DBus signals for 
moved or removed files (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28980?vs=80540=81180

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/metadatamovertest.cpp
  autotests/unit/file/metadatamovertest.h
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/CMakeLists.txt
  src/file/filewatch.cpp
  src/file/filewatch.h
  src/file/mainhub.cpp
  src/file/mainhub.h
  src/file/metadatamover.cpp
  src/file/metadatamover.h

To: bruns, #baloo, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28956: [QML Monitor] Show remaining time as soon as possible

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:60766d57999c: [QML Monitor] Show remaining time as soon 
as possible (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28956?vs=80489=81179

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

AFFECTED FILES
  src/qml/experimental/monitor.cpp
  src/qml/experimental/monitor.h

To: bruns, #baloo, ngraham
Cc: apol, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in fakeopticaldisc.cpp:43
> It doesn't make the code more readable; and using QMap::constFind() while 
> iterating is more recognizable/widely-used.

You are contradicting yourself. You use value() for content, but for 
supportedMedia it is a bad idea? Also, `supportedMedia |= 0` is obviously a 
noop, while for any named flag one has to lookup the flag value.

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D28932: Store filename terms just once

[Stefan Brüns]

bruns added a comment.


  In D28932#657011 , @ngraham wrote:
  
  > I'll get around to reviewing this soon. I'm trying to figure out of I think 
the loss is acceptable.
  
  
  There is no loss, there is even a gain (queries work correctly in all 
constellations).

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28776: FstabDevice: Avoid recurrent construction of emblems QStringList

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28777: FstabDevice: Reevaluate emblems only when state changes

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R245 Solid

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

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

D28956: [QML Monitor] Show remaining time as soon as possible

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: apol, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28932: Store filename terms just once

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in fakeopticaldisc.cpp:43
> /* brown paper bag */, removed the lines.
> 
> I don't see a default a-la-NoContent MediumType member.

Just use `0`.

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in fakeopticaldisc.cpp:43
> Nice. :)
> 
> I think Solid::OpticalDisc::NoContent is more readable.

You forgot to remove the old lines.

Can also be applied to supportedMedia

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fakeopticaldisc.cpp:43
>  }
>  }
>  

These four lines can be written as
`content |= map.value(type, Solid::OpticalDisc::ContentType(0));`

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Stefan Brüns]

bruns added a comment.


  In D28590#656133 , @meven wrote:
  
  > In D28590#654139 , @bruns wrote:
  >
  > > Do not create m_storageAccess in the constructor
  >
  >
  > Hmm, you told me `the f (m_displayName.isEmpty()) block belongs here`, I 
don't see why instantiating `m_storageAccess` here is bad but mehh.
  
  
  Please read again.
  
  1. m_storageAccess is comparatively expensive
  2. I showed you how to get the mount state without StorageAccess
  3. I told you where to initialize it, yes. Doing it correctly of course still 
applies.

REPOSITORY
  R245 Solid

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

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fakeopticaldisc.cpp:38
> +
> +for (auto it = map.cbegin(); it != map.cend(); ++it) {
> +if (content_typelist.contains(it.value())) {

dito

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29138: [Solid] Replace foreach (deprecated) with range/index for

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fakecdrom.cpp:48
> +
> +for (auto it = map.cbegin(); it != map.cend(); ++it) {
> +if (supported_medialist.contains(it.value())) {

The obviously better solution is to iterate over `supported_medialist` and swap 
the key and value of the map.

REPOSITORY
  R245 Solid

BRANCH
  l-foreach (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, meven
Cc: bruns, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29095: New much simpler mouse icon that works in both light and dark breeze

[Stefan Brüns]

bruns added a comment.


  > Bruns's comment about right-handed mice (gradient) is due to the quite 
asymmetrical shape many modern mice use for ergonomic reasons (RSI).
  
  No, even for a ball bottom right is darkest. Shapes should be drawn as if 
there were a light at the top left, i.e. with a shadow at the bottom right.

REPOSITORY
  R266 Breeze Icons

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

To: saligari, #vdg
Cc: bruns, ouwerkerk, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
michaelh

D29095: New much simpler mouse icon that works in both light and dark breeze

[Stefan Brüns]

bruns added a comment.


  F8255905: bitmap.png 

REPOSITORY
  R266 Breeze Icons

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

To: saligari, #vdg
Cc: bruns, ouwerkerk, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
michaelh

D29095: New much simpler mouse icon that works in both light and dark breeze

[Stefan Brüns]

bruns added a comment.


  The new one looks very oldschool. Some ideas how to improve that:
  
  - buttons are typically only separated from each other, but not from the 
body, try removing the horizontal line
  - the body should be narrower at the center
  - the gradient does not match the shape. Bottom right should be darkest

REPOSITORY
  R266 Breeze Icons

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

To: saligari, #vdg
Cc: bruns, ouwerkerk, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
michaelh

D29034: Add systemd user service file for kded

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> davidedmundson wrote in plasma-kded.service.in:5
> @CMAKE_INSTALL_FULL_BINDIR@

How about using some LIBEXEC dir instead, this should never be called directly, 
or am I missing something?

REPOSITORY
  R297 KDED

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

To: broulik, #plasma, #frameworks
Cc: bruns, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> hallas wrote in fstabhandling.cpp:374
> @bruns - I am little confused now. You wrote previously that you could see 
> the point in the `id()` function, but now you want it moved? 
> What kind of unit test are you missing? I can't quite follow the added 
> mangling in `FstabHandling::deviceList()`? I don't think we have (or ever 
> have) had any unit test of FstabHandling, but that was one of the things I am 
> trying to achieve was this patch series that breaks up and decouples the code 
> pieces into smaller bits.
> 
> Could I ask you to take a look at it again? Sorry for nagging, I just really 
> want to close this up soon and move on with the next patches ;D

I never said anything like that. "here" == fstabhandling.cpp

The name is exported by deviceList(), after mangling it. Thats the reason any 
further processing should go there, not into fstentry, but that is matter for 
another patch.

The code should be be split up, but the splitting should be done at the right 
places.

REPOSITORY
  R245 Solid

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

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  Do not create m_storageAccess in the constructor

REPOSITORY
  R245 Solid

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

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28780: [FstabWatcher] Fix loosing of fstab watcher

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:5f97573881fe: [FstabWatcher] Fix loosing of fstab watcher 
(authored by bruns).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28780?vs=79954=80741

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

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabwatcher.cpp
  src/solid/devices/backends/fstab/fstabwatcher.h

To: bruns, #frameworks, ngraham, apol
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28776: FstabDevice: Avoid recurrent construction of emblems QStringList

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28780: [FstabWatcher] Fix loosing of fstab watcher

[Stefan Brüns]

bruns added a reviewer: ngraham.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, ngraham
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28780: [FstabWatcher] Fix loosing of fstab watcher

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added a comment.


  Ping!

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, ngraham
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28995: Add imperial gallon and US pint

[Stefan Brüns]

bruns added a comment.


  In D28995#652764 , @ngraham wrote:
  
  > Aren't Imperial and US the same thing?The USA is the only country that 
still officially uses the imperiaI system.
  
  
  Every time you use "is the same as" in reference to imperial measures, a 
kitten dies ...
  
  https://en.wikipedia.org/wiki/Gallon

REPOSITORY
  R292 KUnitConversion

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

To: riiga, #frameworks, ngraham
Cc: bruns, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh