Re: Review Request: Implementation of KParts listing filter extension for Dolphin
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106289/#review18542
---
Ship it!
Thanks for your understanding and for the new patch, looks good from my point
of view! Maybe I'll check at some point if this can be optimised in master by
sharing more code between setMimeTypeFilters() and setNameFilter(), but in the
stable branch, it's better not to change the existing code too much. You can
push to 4.9 after fixing the little issues below.
dolphin/src/kitemviews/kfileitemmodel.cpp
http://git.reviewboard.kde.org/r/106289/#comment14697
Please remove the space before '('.
dolphin/src/kitemviews/kfileitemmodel.cpp
http://git.reviewboard.kde.org/r/106289/#comment14698
Q_FOREACH - foreach (consistent with existing Dolphin code).
dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp
http://git.reviewboard.kde.org/r/106289/#comment14696
Can be removed.
dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp
http://git.reviewboard.kde.org/r/106289/#comment14699
Remove the space before '('.
dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp
http://git.reviewboard.kde.org/r/106289/#comment14700
Q_FOREACH(...) - foreach (...), see above.
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/106289/#comment14701
Remove the space before '('.
- Frank Reininghaus
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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
On Sept. 5, 2012, 9:27 a.m., Frank Reininghaus wrote: Thanks for your understanding and for the new patch, looks good from my point of view! Maybe I'll check at some point if this can be optimised in master by sharing more code between setMimeTypeFilters() and setNameFilter(), but in the stable branch, it's better not to change the existing code too much. You can push to 4.9 after fixing the little issues below. Ooops... I guess I posted an earlier version of the patch. :( I have already factored out the common code in those two functions. Except for part that sets the filters, they share the same exact code. Anyhow, I will post that version of the patch along with the changes for the minor issues you raised. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18542 --- 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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Sept. 5, 2012, 1:28 p.m.) Review request for Dolphin and KDE Base Apps. Changes --- - Factored out the same exact code in setNameFilter and setMimeTypeFilters into a shared, applyFilters. - Addressed the minor issues raised. 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 (updated) - dolphin/src/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18552 --- Ship it! Thanks, looks good and is OK to commit (unless David has any comments about the DolphinPart changes, of course). - Frank Reininghaus On Sept. 5, 2012, 1:28 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Sept. 5, 2012, 1:28 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/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18555 --- This review has been submitted with commit cb79ee6a881e2b4418bccc22480e3e269e5b0af9 by Dawit Alemayehu to branch KDE/4.9. - Commit Hook On Sept. 5, 2012, 1:28 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Sept. 5, 2012, 1:28 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/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18556 --- This review has been submitted with commit 603a93268efb9d09f8c6255907f46928c651fdbd by Dawit Alemayehu to branch master. - Commit Hook On Sept. 5, 2012, 1:28 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Sept. 5, 2012, 1:28 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/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18482 --- 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. - Frank Reininghaus On Sept. 3, 2012, 9:03 p.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, 9:03 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/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- 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. Changes --- Removed the notification related code out of the filtering extension. 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 (updated) - 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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
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. 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? - Frank
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Sept. 3, 2012, 9:03 p.m.) Review request for Dolphin and KDE Base Apps. Changes --- Final version. This one is thoroughly tested against both dolphin and Konqueror. In addition to fixing Konqueror's directory filter plugin, it addresses the following issues: * Allow both name and mime-type filters to be used at the same time (aggregated filtering). * Fix behavior of the wildcard matching support in Konqueror's location bar. 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 (updated) - dolphin/src/views/dolphinview.h 10f63c5 dolphin/src/views/dolphinview.cpp 8050415 dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 816d356 dolphin/src/kitemviews/private/kfileitemmodelfilter.h 9bdf1fd dolphin/src/dolphinpart.h e5693b3 dolphin/src/dolphinpart.cpp fff7dc0 dolphin/src/kitemviews/kfileitemmodel.h d9bebdf dolphin/src/kitemviews/kfileitemmodel.cpp 6936af4 Diff: http://git.reviewboard.kde.org/r/106289/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
--- 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. Changes --- Avoid conflict between the name and type filters by storing the filtered items in different containers. 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 (updated) - 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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
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. 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? - Frank --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18384 --- On Aug. 31, 2012, 7:30 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/ --- (Updated Aug. 31, 2012, 7:30 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/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
Re: Review Request: Implementation of KParts listing filter extension for Dolphin
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? 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. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/#review18384 --- On Aug. 31, 2012, 7:30 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106289/
