Review Request: Implementation of KParts listing filter extension for Dolphin

[Dawit Alemayehu]

> On Sept. 4, 2012, 5:59 a.m., Frank Reininghaus wrote:
> > Thanks for your explanations!
> > 
> > First of all, let me say that I greatly appreciate all the awesome work you're doing in Konqueror and kdelibs. I know that you made many great contributins to areas that many others have little interest in.
> > 
> > I see now why you propose to add these signals, but I ask you to also try to understand my point of view. One of my main goals is to keep Dolphin's code readable and maintainable. If we follow your suggestion, KFileItemModel would have the signals
> > 
> > itemsAdded(const KFileItemList&)
> > itemsDeleted(const KFileItemList&)
> > itemsInserted(const KItemRangeList&)
> > itemsRemoved(const KItemRangeList&)
> > 
> > which have completely different semantics. This would be quite confusing, not only at first sight, and seriously harm the readability of the code IMHO.
> > 
> > But I think that you can achieve what you want quite easily without these signals. You could create a dir lister inside the dir filter plugin that watches the directory. This would give you access to all files in the directory without the need to add those signals. You would just have to make sure that this dir lister has the correct "show hidden files" setting, but this should be doable because DolphinView has a signal hiddenFilesShownChanged(bool).
> > 
> > If this solution is acceptable for you, I'm happy to add the mime filter functionality to KFileItemModel.
> > 
> > Just for the record, I also discussed this with Peter last night, just to make sure that I don't tell you complete nonsense here. He agrees that adding those signals to KFileItemModel and DolphinView would be a very bad idea.

First thank you for the kind words. Secondly I most definitely understand your dilema. I undestand what it takes to keep APIs clean and maintaintable and I can appreciate your desire to maintain Dolphin's in such manner. However, I can most definitely tell you that the solution you suggested is a complete non-starter for me. Though the solution might seem the cleanest way to handle this issue, duplicating the work the embedded part already performs penalizes those Konqueror users who have enabled the directory filter plugin (the default) unnecessarily. That would not only be true for local file systems, but also remote ones that support listing like FTP and SFTP. To me that is simply not acceptable. The reason I use a hack in the directory filter plugin to locate the KDirLister from the part was to avoid the very exact thing you suggested above.

Anyhow, I have split out the notification related code into its own extension as you can see in REVIEW:106288. That way the filtering extension can stand on its own and if you guys still disagree with a new solution I came up with to solve this issue, then at least the filtering extension can be committed. I will post a new review for the implementation of the notification extension.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106289/#review18482
-----------------------------------------------------------


On Sept. 4, 2012, 8:01 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106289/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2012, 8:01 p.m.)
> 
> 
> Review request for Dolphin and KDE Base Apps.
> 
> 
> Description
> -------
> 
> The attached patch provides an implementation of KParts' ListingFilterExtension for Dolphin. The extension allows the Dolphin KPart to provide directory/file filtering services without requiring direct linking against Dolphin itself.
> 
> The review for the new KPart listing filter extension (ListingFilterExtension) can be found at https://git.reviewboard.kde.org/r/106288/
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd 
>   dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 
>   dolphin/src/views/dolphinview.h 10f63c5 
>   dolphin/src/views/dolphinview.cpp 8050415 
>   dolphin/src/kitemviews/kfileitemmodel.h d9bebdf 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 
>   dolphin/src/dolphinpart.h e5693b3 
>   dolphin/src/dolphinpart.cpp fff7dc0 
> 
> Diff: http://git.reviewboard.kde.org/r/106289/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120904/e7ef4bcc/attachment.htm>