Review Request: Implementation of KParts listing filter extension for Dolphin

[Dawit Alemayehu]

> On Sept. 1, 2012, 12:03 p.m., Frank Reininghaus wrote:
> > First of all, thanks for the patch!
> > 
> > I was not aware of any "constant breakage of the Konqueror directory filtering plugin once Konqueror was switched to use Dolphin's part for filemanagement", and I think neither was Peter.
> > 
> > I checked it now, and can confirm that mime type filtering does not work (to be honest, I never noticed this menu entry in Konqueror before), but did this actually ever work in the DolphinPart? A quick look at the DolphinView code from KDE 4.7 didn't show me anything that filters mime types. What else is broken about the filtering functionality at the moment? Filtering using ~/*.jpg in Konqueror's location bar works for me.
> > 
> > Moreover, what are the new signals itemsAdded() and itemsDeleted() needed for? They could be confused easily with the existing signals itemsInserted() and itemsRemoved(). Therefore, I think we should not add these new signals unless there is a *very* good reason to have them.
> 
> Dawit Alemayehu wrote:
>     The breakage is not really the fault of the DolphinPart, but rather the fact that access to crucial signals from KDirLister, the equivalent of the "itemsAdded" and "itemsDeleted", was no longer available as it was in the original Konqueror filemanagement module. It is also true that the plugin never worked correctly once Konqueror was ported to use DolphinPart for filemanagement. However, that situation was sort of addressed by myself (due to a bug report) using a very ugly hack a while back. The hack simply attempts to walk through the a Part's child classes and find an instance of a KDirLister and connect to its newItems and itemsDeleted signals. As is the case with most hacks, any change in the code is bound to break any assumptions made by the hack and that is exactly what transpired once Dolphin was re-written for its 2.0 release.
>     
>     As far as the two new signals I added to KFileItemModel and DolphinView, itemsAdded and itemsDeleted, this whole patch will be useless without them. They are very important because these new signals are not about what is happening to the current DolphinView. Rather they are about what is going on in the actual directory the view is displaying. Simply put, the new signals are ONLY emitted when the user adds or deletes an item (files or sub directories) in current directory. The old signals on the other hand are emitted when anything changes in the view itself, e.g. the view is filtered by a name.
> 
> Frank Reininghaus wrote:
>     Thanks, but I still don't get why you need these signals. If filtering is done inside KFileItemModel/DolphinView, why do you need access to the KDirLister's signals?
> 
> Dawit Alemayehu wrote:
>     To connect them to the corresponding signals in KParts::ListingFilterExtension and we need those signals in the new listing filter extension because without them, and this is perhaps specific to the directory filter plugin, we would not be able to correctly update the list of file types we show in Konqueror's directory filter plugin menu. More specifically if the user deletes all of the PDF files in the current listing, we need to remove the "PDF document" filter entry from the plugin menu. Without those signals letting us know about the removal we would not be able to do this easily. The same thing applies if a new document type gets added after the listing is displayed. Additionally, the plugin also shows the count of each file type present in the current listing and as such needs to keep track of the number of specific file types present. Those two signals make all of that possible without having to use yet another extension, KParts::FileInfoExtension.
>     
>     If those two signals were not present, then not only we would have to retrieve all the items in the current listing every time the "View Filter" menu is clicked using the KParts::FileInfoExtension extension, but also attempt to find the difference between the prior and current retrievals in order to determine if there were any changes. The matter becomes even more complicated once a filter is applied to the current view. That is why those two signals are very essential. You connect to them and you know everything the directory lister knows about the contents of the current directory. No need to use yet another extension to manually retrieve the listings evertime the plugin menu is shown, or attempt to diff two listings just to look for any changes.
>     
>
> 
> Frank Reininghaus wrote:
>     I see, thanks for the explanation!
>     
>     However, I still think that exposing the KDirLister's signals in the public API is the wrong way to go. If there is some functionality that KFileItemModel does not provide at the moment, but which is needed by Konqueror, wouldn't it be much better to integrate this into KFileItemModel and emit a signal if a mimetype appears or disappears inside the model?

Nope. I have already stated these reasons above, but I will list them here again:

* If we do not get live notification about files and folders being added or removed, we would have to resort to getting that information manually through KParts::FileInfoExtension. Doing this causes its own complications because what we get back will change depending on whether a filter is already set or not.

* We need more information than a certain mime-type appearing and disappearing. For example, we use the iconName(), mimeComment() APIs directly from KFileItem. We also provide the user with the actual count of each file type, e.g. PDF document (4). Additionally, this extension API should allow future plugins to enable filtering based on other criteria which are already available in the KFileItem API.

Personally, I do not see how re-emitting KDirLister's signals can be considered exposing them in the public API. As far as the users of the KFileItemModel API are concerned, all they would see is the new signals in KFileItemModel. How those signals are emitted is an unimportant implementation detail. Only what those signals provide matters to them. In fact, nothing would stop use from changing the circumstance under which those signals are emitted from KFileItemModel at anytime. Even if this is considered exposing the signals in a public API, I fail to see the downside of that since the signals only provide informational data that has no effect on Dolphin's own behavior. I guess I just do not see why you think re-emitting those signals is the "wrong way to go".


- Dawit


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


On Sept. 3, 2012, 3:04 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106289/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2012, 3:04 a.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/dolphinpart.h e5693b3 
>   dolphin/src/dolphinpart.cpp fff7dc0 
>   dolphin/src/kitemviews/kfileitemmodel.h d9bebdf 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 
>   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 
> 
> 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/20120903/165f4431/attachment.htm>