Review Request 129205: [kcoredirlister] Ability to watch files changes

[David Faure]

> On Oct. 30, 2016, 10:38 p.m., David Faure wrote:
> > KCoreDirLister is complex enough, I'd rather not add features to it.
> > 
> > Let's take a step back.
> > 
> > This looks like an already solved problem to me, if I understand it correctly:
> >  - a kioslave creates a virtual filesystem on top of the local file system (there are many doing that: kio_desktop, kio_stash, 3 baloo ones (baloosearch, timeline, tags) in fact there are at least 21 (those with Class=:local) but they don't all care about deletion, e.g. because readonly)
> >  - the user deletes/renames a local file (or even adds a file, that should be added to the virtual filesystem)
> >  - somehow (see below) the app (via KCoreDirLister) needs to be notified using a URL of that virtual filesystem (not just file:///).
> > right?
> > 
> > The way this has always been done, where implemented (kio_desktop, kio_stash, and in the past some others) is the following
> >  - because kioslaves are short-lived, they can't do the watching (this replies to Mark's suggestion number 1)
> >  - a kded module is created, to do the watching using KDirWatch - of only those files that the kioslave is listing
> >  - the kded module emits KDirNotify DBus signals when a file is removed/renamed, using the kioslave's URL scheme.
> > 
> > You can see these existing kded modules here:
> > - for desktop: plasma-workspace/kioslave/desktop/desktopnotifier.cpp
> > - for baloo: frameworks/baloo/src/kioslaves/kded/
> > - for stash: playground/utils/kio-stash/src/iodaemon/
> > 
> > It seems to me that simply filenamesearch needs its own similar kded module. I don't see any in kio-extras/filenamesearch.
> 
> Albert Astals Cid wrote:
>     Given we seem to have "a few" of those kded modules, is there something we can do to help people making them and not having the same trouble over and over again?

What do you mean? Documentation, yes. Shared code? I'm not sure. We're talking about one class with one constructor and 2 or 3 methods, and what to react to, depends on the kioslave. And how to react (converting from file:// to custom://) depends on the kioslave too.

(BTW use desktopnotifier or kio-stash as example, baloo is a bit different because it's listening to another daemon (so it didn't really need a kded module in the first place)


> On Oct. 30, 2016, 10:38 p.m., David Faure wrote:
> > src/core/kcoredirlister.h, line 429
> > <https://git.reviewboard.kde.org/r/129205/diff/3/?file=482930#file482930line429>
> >
> >     You can't add virtual methods to a public class, this is not binary compatible.
> 
> Anthony Fieroni wrote:
>     That because it's added at last position in public scope, i wasn't sure to add it here or even after qsignals.

No no. Adding a virtual method is binary incompatible, where-ever you put it. (it changes the size of the vtable).
Please read https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
(but anyway for this particular issue I'm recommending a kded module so this is moot anyway)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129205/#review100411
-----------------------------------------------------------


On Oct. 24, 2016, 7:05 a.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129205/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2016, 7:05 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> David, i will discard review if you don't like it, cause watching files changes can be *really* expensive. I try to:
> 1. to not break abi compability
> 2. to extend filenamesearch with this option
> 3. to fix https://git.reviewboard.kde.org/r/129141/
> 
> 
> Diffs
> -----
> 
>   src/core/kcoredirlister.h e6ba2ac 
>   src/core/kcoredirlister.cpp 508516e 
>   src/core/kcoredirlister_p.h 9a3cc7b 
>   tests/kdirlistertest_gui.h 8316b20 
>   tests/kdirlistertest_gui.cpp 11e89cf 
> 
> Diff: https://git.reviewboard.kde.org/r/129205/diff/
> 
> 
> Testing
> -------
> 
> For 3. i still can't figure out why in filenamesearch signal for delete item(s) is not triggered.
> 1. Search for file (by name) in dolphin
> 2. When appear in view delete him
> 3. Signal ItemsDeleted is not triggered, file stays in the view even if new search is performed and reload is needed. The cache look good, tests pass, works but in filenamesearch.
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161031/b8a55865/attachment.html>