D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

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

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D4911?vs=14953=33246#toc

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=14953=33246

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/metadatamovertest.cpp
  autotests/unit/file/metadatamovertest.h
  src/CMakeLists.txt
  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
  src/file/org.kde.BalooWatcherApplication.xml

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, ashaposhnikov, michaelh, astippich, 
spoorun, bruns

D4911: add Baloo DBus signals for moved or removed files

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  My objections no longer apply, thanks for the redesign for more performance.

REPOSITORY
  R293 Baloo

BRANCH
  master

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, ashaposhnikov, michaelh, astippich, 
spoorun, bruns

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  > I did some test today on tbaloo and noticed one problem when fetching the 
results from a query. The paths are encoded like URLs but without the scheme. I 
had to modify my code to use it. Apart from that, nice work. It just works. Are 
you still interested to work on that ?
  
  I would be willing to work more on that, if there is consensus that it should 
replace baloo and we use the synergy with tracker instead of doing all things 
tracker does just once again for the sake of it (beside running in all the same 
security problems).
  But until now, that doesn't look like there is such a consensus.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In D4911#199527 , @cullmann wrote:
  
  > @cullmann: T7860  lists some of my 
observations wrt samba shares. I don't know NFS, can you enlighten me regarding 
deviceids and inodes on NFS?
  >
  > I can only tell that baloo can't work at all on NFS/SMB home dirs with the 
current DB and that its idea of inode number usage to encode the file won't 
work there either (it already doesn't work on large file systems with 64-bit 
inodes).
  >
  > I am very happy that somebody is working on fixing the current bugs, but I 
am still not sure it would not just be better to port away to some better 
maintained and more secure thing like tracker, but my  
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ port stagnated
  >  as there was no interest.
  
  
  I did some test today on tbaloo and noticed one problem when fetching the 
results from a query. The paths are encoded like URLs but without the scheme. I 
had to modify my code to use it. Apart from that, nice work. It just works. Are 
you still interested to work on that ?

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  @cullmann: https://phabricator.kde.org/T7860 lists some of my observations 
wrt samba shares. I don't know NFS, can you enlighten me regarding deviceids 
and inodes on NFS?
  
  I can only tell that baloo can't work at all on NFS/SMB home dirs with the 
current DB and that its idea of inode number usage to encode the file won't 
work there either (it already doesn't work on large file systems with 64-bit 
inodes).
  
  I am very happy that somebody is working on fixing the current bugs, but I am 
still not sure it would not just be better to port away to some better 
maintained and more secure thing like tracker, but my  
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ port stagnated
  as there was no interest.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, michaelh, spoorun, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Michael Heidelbach]

michaelh added a comment.


  Considering that I'm more or less the only one working on baloo, I'd prefer 
to postpone this for a while instead of adding more complexity. At least until 
I have understood the inner workings of baloo.
  Also the list of bugs reporting baloo's crashes is quite long. Fixing those 
has become my top priority.
  
  @mgallien: Unless it really thwarts you I ask for your patience.
  
  > I would actually not even do that, as baloo isn't working that well e.g. on 
NFS and co. and that will limit the use of your application (which is cool 
btw.!).
  
  @cullmann: T7860  lists some of my 
observations wrt samba shares. I don't know NFS, can you enlighten me regarding 
deviceids and inodes on NFS?

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, michaelh, spoorun, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Nathaniel Graham]

ngraham added a comment.


  By the way, I take back everything I said up-thread about abandoning Baloo. 
We've got some new blood working on it and I feel confident about its future 
now! So it would be great to get some experienced eyeballs on this patch.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, michaelh, spoorun, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Michael Heidelbach]

michaelh added a reviewer: Baloo.
michaelh added a project: Baloo.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, michaelh, spoorun, alexeymin

D4911: add Baloo DBus signals for moved or removed files

[Nathaniel Graham]

ngraham added a reviewer: michaelh.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure, michaelh
Cc: ngraham, cullmann, apol, #frameworks, michaelh

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  Ping

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: ngraham, cullmann, apol, #frameworks, michaelh

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  Hello @ngraham and @cullmann,
  I lack time to properly maintain stuff I added to KFileMetaData. I do not 
think I may be able to put a lot of energy in Baloo.
  At the same time, this is a solution that quite work for me and the usage I 
do in Elisa. I am just trying to come to a conclusion in this review such that 
I can move on.
  I still plan to properly support tracker since it is used in a lot of default 
configuration of Linux distributions, just not now.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: ngraham, cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  Unfortunately I never finished the 
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ port :/
  Even more unfortunately there are close to zero useful baloo commits either 
since that time.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: ngraham, cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Nathaniel Graham]

ngraham added a comment.


  Not to throw cold water on your patch (it's nice to see someone working on 
Baloo!), but perhaps we should continue @cullmann's work to move over to 
Tracker instead. It's an active project, and we would benefit from all the work 
that the GNOME people are putting into it. I fear that Baloo is destined to 
suffer a slow death from bitrot unless it gains at least one full-time 
maintainer. If that happens, Elisa won't be able to rely on it anyway, and that 
code will need to be removed or ported over to use Tracker (or whatever else we 
use instead).

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: ngraham, cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  Ping

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien updated this revision to Diff 14953.
mgallien added a comment.


  add a new method to register applications wanting to watch moved files and
  define (via xml) the interface to implement for applications

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=12137=14953

BRANCH
  master

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/metadatamovertest.cpp
  autotests/unit/file/metadatamovertest.h
  src/CMakeLists.txt
  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
  src/file/org.kde.BalooWatcherApplication.xml

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In https://phabricator.kde.org/D4911#106088, @dfaure wrote:
  
  > Could this be made more lightweight by adding a precise subscription method 
rather than signal broadcasts?
  
  
  I will work on this solution.
  
  > The interested application would register into baloo (using dbus or C++, 
not sure what's applicable here), saying "please do inform me about changes" or 
maybe even "please inform me about changes  or ". And the MetadataMover code would only emit a dbus signal if 1) 
someone registered, 2) the signal passes the dir or mimetype filter.
  > 
  > This way, nothing changes performance-wise for people not using an app that 
needs these signals.
  
  Thanks David for your suggestion. This looks like a great idea and should 
really help me find a solution that works for applications.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[David Faure]

dfaure added a comment.


  Could this be made more lightweight by adding a precise subscription method 
rather than signal broadcasts?
  
  The interested application would register into baloo (using dbus or C++, not 
sure what's applicable here), saying "please do inform me about changes" or 
maybe even "please inform me about changes  or ". And the MetadataMover code would only emit a dbus signal if 1) 
someone registered, 2) the signal passes the dir or mimetype filter.
  
  This way, nothing changes performance-wise for people not using an app that 
needs these signals.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a reviewer: dfaure.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien reclaimed this revision.
mgallien added a comment.


  Reopening since I am still convinced that getting signals from Baloo is a lot 
more sane than adding workaround in each users of Baloo.
  
  My point is that for a music player like the one I am working on (Elisa), I 
have to do the following:
  
  - ask Baloo for an initial list of files (using Baloo APIs) ;
  - add file system watches for all those files (needed for removed files or 
directory and moved files or directories) ;
  - listen to some DBus signals from Baloo (needed to understand new files have 
been detected by Baloo in directories I am watching or not). May be used to 
detect changes to already known files ;
  - hope that my code is good enough to not miss any changes.
  
  One of the worst thing in this schema is that Baloo is slower than file 
system watches. Changes will not be detected in the order they happen. Changes 
detected through Baloo will happen after changes detected by watches. User 
Experience may suffer from that (needs more test to see if this is real).
  
  As I already said, if the cost of getting exact changes is too high, just a 
signal saying something changed is good enough if we get notified of *all* 
changes.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  > This is one way to get tracks in parallel with other ways to get tracks. If 
Baloo is active I can only hope that the user as made actions to ensure that it 
is not just crashing., If Baloo is not active, I should check that my 
application still behave nominally.
  
  That sounds very reasonable!
  
  Greetings
  Christoph

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In https://phabricator.kde.org/D4911#94076, @cullmann wrote:
  
  > > I will limit Elisa usage of Baloo to getting an initial list of files 
hopefully faster than by looking at file system.
  >
  > I would actually not even do that, as baloo isn't working that well e.g. on 
NFS and co. and that will limit the use of your application (which is cool 
btw.!).
  
  
  This is one way to get tracks in parallel with other ways to get tracks. If 
Baloo is active I can only hope that the user as made actions to ensure that it 
is not just crashing., If Baloo is not active, I should check that my 
application still behave nominally.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  > I will limit Elisa usage of Baloo to getting an initial list of files 
hopefully faster than by looking at file system.
  
  I would actually not even do that, as baloo isn't working that well e.g. on 
NFS and co. and that will limit the use of your application (which is cool 
btw.!).

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien abandoned this revision.
mgallien added a comment.


  I think this review is going nowhere. I prefer to cancel it instead of 
spending my energy on it.
  I will limit Elisa usage of Baloo to getting an initial list of files 
hopefully faster than by looking at file system. All later events will go 
through other APIs.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  In https://phabricator.kde.org/D4911#93007, @mgallien wrote:
  
  > > I was hoping to support the same file indexer that Plasma is using hence 
my work on Baloo. On my roadmap, I plan to also support Tracker.
  > >  For Windows and Android, I have implemented an indexer with Qt + 
KFileMetaData APIs. I was just hoping that Baloo would provide live refresh of 
queries. I have to check if Tracker does it or not.
  >
  > I have installed again Tracker and an application using it on my Debian 
unstable. With qdbusviewer, I see a DBus signal for signaling changes to its 
database.  i have to check but I think this is exactly the feature I need to be 
able to really use Baloo. This way, an application know when to execute once 
more its query to get an updated result.
  
  
  Than I would more go the "use tracker" route than implement this in baloo.
  
  > I will also modify my patch to implement only one signal fired without 
parameters (i.e. no list of modified content) when Baloo database is modified. 
Vishesh do you think this would be problematic performance wise ? This would 
allow my application to always get an updated list of audio files whenever 
files are added, removed or modified. This would avoid duplicating file system 
watches.
  
  Hmm, baloo can modify the database the whole day around, do we really want 
signals on that?
  
  > I will try using Tracker with your work compatibility layer. Did you get 
feedback from frameworks maintainer ?
  
  None, I play around with baloo-search, that seemed to work fine with my patch 
on my tracker DB, but given I really not use a lot of baloo features (as my 
normal experience is that of instant segfault or OOM), I can't comment on the 
usability.
  (beside baloo is a large security hole, tracker at least partly fixed the 
biggest issues, remembering the "oh, tracker loads your exploit on the fly 
after downloading some hacked file" flames)

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In https://phabricator.kde.org/D4911#92732, @mgallien wrote:
  
  > In https://phabricator.kde.org/D4911#92718, @cullmann wrote:
  >
  > > Just my 2 cents from the sideline:
  > >
  > > 1. baloo is unmaintained and the bugs pile up, just check bugs.kde.org 
for that, not sure if adding yet-an-other feature to it is a good idea
  > > 2. as vhanda said, perhaps better use other API for that
  >
  >
  > Those APIs are not free and takes some kernel resources.
  >
  > > I tried to replace baloo with tracker, see 
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ but failed out of time 
(searching works, but not all API), that won't support such things anyways.
  >
  > I was hoping to support the same file indexer that Plasma is using hence my 
work on Baloo. On my roadmap, I plan to also support Tracker.
  >  For Windows and Android, I have implemented an indexer with Qt + 
KFileMetaData APIs. I was just hoping that Baloo would provide live refresh of 
queries. I have to check if Tracker does it or not.
  
  
  I have installed again Tracker and an application using it on my Debian 
unstable. With qdbusviewer, I see a DBus signal for signaling changes to its 
database.  i have to check but I think this is exactly the feature I need to be 
able to really use Baloo. This way, an application know when to execute once 
more its query to get an updated result.
  
  I will also modify my patch to implement only one signal fired without 
parameters (i.e. no list of modified content) when Baloo database is modified. 
Vishesh do you think this would be problematic performance wise ? This would 
allow my application to always get an updated list of audio files whenever 
files are added, removed or modified. This would avoid duplicating file system 
watches.
  
  I will try using Tracker with your work compatibility layer. Did you get 
feedback from frameworks maintainer ?

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In https://phabricator.kde.org/D4911#92718, @cullmann wrote:
  
  > Just my 2 cents from the sideline:
  >
  > 1. baloo is unmaintained and the bugs pile up, just check bugs.kde.org for 
that, not sure if adding yet-an-other feature to it is a good idea
  > 2. as vhanda said, perhaps better use other API for that
  
  
  Those APIs are not free and takes some kernel resources.
  
  > I tried to replace baloo with tracker, see 
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ but failed out of time 
(searching works, but not all API), that won't support such things anyways.
  
  I was hoping to support the same file indexer that Plasma is using hence my 
work on Baloo. On my roadmap, I plan to also support Tracker.
  For Windows and Android, I have implemented an indexer with Qt + 
KFileMetaData APIs. I was just hoping that Baloo would provide live refresh of 
queries. I have to check if Tracker does it or not.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Christoph Cullmann]

cullmann added a comment.


  Just my 2 cents from the sideline:
  
  1. baloo is unmaintained and the bugs pile up, just check bugs.kde.org for 
that, not sure if adding yet-an-other feature to it is a good idea
  2. as vhanda said, perhaps better use other API for that
  
  I tried to replace baloo with tracker, see 
https://cgit.kde.org/clones/baloo/cullmann/tbaloo.git/ but failed out of time 
(searching works, but not all API), that won't support such things anyways.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: cullmann, apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a comment.


  In https://phabricator.kde.org/D4911#92473, @vhanda wrote:
  
  > I'm not the maintainer of Baloo any more, so I don't want to give it a 
clear Yes / No.
  >
  > This patch is going to be a big CPU hog. For files this will barely have an 
impact, but for folders of a large enough size, it's going to result in tons of 
dbus signals, and more importantly, tons of database lookups and full path 
constructions. It really will add up. In an earlier version of Baloo, we used 
to store the full file paths, which meant when a folder moved, every 
sub-file/folder's URL had to be updated. That was a huge CPU burner. This patch 
isn't that bad, but it's half way there.
  
  
  For any top folders, only one signal is sent with my patch. Not sure that 
would add significant CPU overhead.
  If I understand you correctly you fear that fetching the names of the files 
under the directory will add significant CPU overhead. Do you have any numbers ?
  I believe I could do some benchmarks to see how much CPU is used to get the 
list of removed paths and not only the Baloo internal ids. What would convince 
you ?
  I believe one worst case would be somebody deleting a lot of folders selected 
in dolphin and each one having a very deep hierarchy. Each top folders would 
mean a DBus signal with a lot of content. Do you think I should benchmarks this 
worst case scenario ?
  Currently for changed files, one property changes and two signals are sent.
  
  > I cannot see what advantage these signals add, apart from possibly making 
Baloo more introspect-able. If it's to build applications on top of Baloo, I 
would recommend against it. The kernel provides file system monitoring APIs 
which should be used instead.
  
  This is the most important part of the question.
  I think that software like Baloo should provide a way to have live refresh of 
queries for applications using it. Currently Baloo does not provide this.
  This a real blocker in my opinion for an application. Sure, I could do 
polling but the user experience would not be ideal at all.
  
  The other alternative is applications using Baloo also watch the file system. 
That would mean that the user of an application using Baloo would have double 
quantity of inotify watches. This would also mean that if there is a limit in 
their number, that would be reached more quickly. Those duplicated watches 
would exist only to know when the results of the Baloo queries have changed all 
done by each application using it.
  
  I will probably do that since this way I am independent of any new patches 
for Baloo and release for me will be easier (no need to bump a dependency, ...).

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Vishesh Handa]

vhanda added a comment.


  I'm not the maintainer of Baloo any more, so I don't want to give it a clear 
Yes / No.
  
  This patch is going to be a big CPU hog. For files this will barely have an 
impact, but for folders of a large enough size, it's going to result in tons of 
dbus signals, and more importantly, tons of database lookups and full path 
constructions. It really will add up. In an earlier version of Baloo, we used 
to store the full file paths, which meant when a folder moved, every 
sub-file/folder's URL had to be updated. That was a huge CPU burner. This patch 
isn't that bad, but it's half way there.
  
  I cannot see what advantage these signals add, apart from possibly making 
Baloo more introspect-able. If it's to build applications on top of Baloo, I 
would recommend against it. The kernel provides file system monitoring APIs 
which should be used instead.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Aleix Pol Gonzalez]

apol added a reviewer: vhanda.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien updated this revision to Diff 12137.
mgallien added a comment.


  add checks that the correct signal is sent during tests of MetadataMover

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=12134=12137

BRANCH
  master

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

AFFECTED FILES
  autotests/unit/file/metadatamovertest.cpp
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/metadatamover.cpp

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Aleix Pol Gonzalez]

apol added a comment.


  :) good, thanks, can you look into the autotests?

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien marked 2 inline comments as done.

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien updated this revision to Diff 12134.
mgallien added a comment.


  fix two new issues reported by apol

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=12116=12134

BRANCH
  master

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

AFFECTED FILES
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/metadatamover.cpp

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Aleix Pol Gonzalez]

apol added a comment.


  autotests would be very welcome!

INLINE COMMENTS

> transaction.cpp:134
> +
> +const QVector children = docUrlDB.getChildren(parentId);
> +

Return right away?

> apol wrote in metadatamover.cpp:49
> const auto &

Uh you are right, no need for a const&, I assumed it were QString for some 
reason.

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien marked 4 inline comments as done.

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien updated this revision to Diff 12116.
mgallien added a comment.


  Fix issues reported by apol
  
  Not sure the list iteration on quint64 needs a const auto &.
  
  Will add automatic tests later and as soon as possible

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=12109=12116

BRANCH
  master

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

AFFECTED FILES
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/metadatamover.cpp

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> metadatamover.cpp:47
> +{
> +auto childrenIds = tr.childrenDocumentId(parentId);
> +

make const

> metadatamover.cpp:49
> +
> +for (auto oneChildren : childrenIds) {
> +fileList.push_back(QFile::decodeName(tr.documentUrl(oneChildren)));

const auto &

> metadatamover.cpp:83
> +QVariantList vl;
> +vl.reserve(1);
> +vl << QVariant(fileList);

That `reserve` is wrong, it should be 3.

How about this?
`message.setArguments({QVariant(fileList), from, to});`

> metadatamover.cpp:128
> +vl << QVariant(fileList);
> +message.setArguments(vl);
> +

This would be much more readable using the initializer syntax.

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: apol, #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a task: T4931: Incremental changes from Baloo (new tracks, 
modified tracks, removed tracks).

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien removed a reviewer: kde-frameworks-devel.

REPOSITORY
  R293 Baloo

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

To: mgallien
Cc: #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien added a reviewer: kde-frameworks-devel.
View RevisionREPOSITORYR293 BalooREVISION DETAILhttps://phabricator.kde.org/D4911EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: mgallien, kde-frameworks-develCc: Frameworks

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

mgallien created this revision.Restricted Application added a project: Frameworks.Restricted Application added a subscriber: Frameworks.
View RevisionREVISION SUMMARYAdd new DBus signals sent by Baloo indexer for removed or moved files.
I still need to extend existing automatic tests with checks that the signals are sentREPOSITORYR293 BalooBRANCHmasterREVISION DETAILhttps://phabricator.kde.org/D4911AFFECTED FILESsrc/engine/transaction.cpp
src/engine/transaction.h
src/file/metadatamover.cppEMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: mgallienCc: Frameworks