Review Request: A Facet and search GUI API for Nepomuk

[Christoph Feck]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5671/#review8297
-----------------------------------------------------------


Hi Sebastian, lots of good code to review :)

It would help for the API review, if you could mark private headers with the *_p.h convention.

For example, I was to suggest to make the SearchLineEdit be a simple QWidget with an QLineEdit accessor (similar to QComboBox), so that enhancements can be made (such as using a KComboBox internally later). But it looks like this class isn't exported, so this suggestions is void.

What I didn't understand from looking at the comments is what a "selection" in Facet terms means. It doesn't mean selecting the query results, does it?

If there is some general overview document somewhere, it would help understanding what the API wants to offer, and that again would help the review process.


trunk/KDE/kdelibs/nepomuk/utils/datefacet.h
<http://svn.reviewboard.kde.org/r/5671/#comment8571>

    Private* const d;



trunk/KDE/kdelibs/nepomuk/utils/datefacet.cpp
<http://svn.reviewboard.kde.org/r/5671/#comment8572>

    Hm, having reviewed bug reports in KDE for nearly a year now, I can only warn about code that uses local event loops.
    
    In other words, if there is no way around it, you should warn the user of the class in the documentation for that function.


- Christoph


On 2010-10-20 16:07:36, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5671/
> -----------------------------------------------------------
> 
> (Updated 2010-10-20 16:07:36)
> 
> 
> Review request for kdelibs and Vishesh Handa.
> 
> 
> Summary
> -------
> 
> I have been struggling with creating a good facet API for a long time now. I finally reached a point where I am happy with the result. IMHO this is essential enough to get into kdelibs/nepomuk. The first usage will obviously be Dolphin, closely followed by the file dialog. All in all this will make it so much simpler to provide Nepomuk powered search capabilities in applications.
> Anyway, this review request is intended for an API review before I commit.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/includes/CMakeLists.txt 1187872 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/DynamicResourceFacet PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/Facet PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/FacetWidget PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/ResourceModel PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/SearchWidget PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/SimpleFacet PRE-CREATION 
>   trunk/KDE/kdelibs/includes/Nepomuk/Utils/SimpleResourceModel PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/CMakeLists.txt 1187872 
>   trunk/KDE/kdelibs/nepomuk/utils/datefacet.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/datefacet.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/daterange.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/daterange.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.ui PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/dynamicresourcefacet.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/dynamicresourcefacet.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facet.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facet.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetdelegate.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetdelegate.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetfiltermodel.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetfiltermodel.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetmodel.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetmodel.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetwidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/facetwidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/resourcemodel.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/resourcemodel.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/searchlineedit.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/searchlineedit.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/searchwidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/searchwidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/searchwidget_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/simplefacet.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/simplefacet.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/simpleresourcemodel.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/utils/simpleresourcemodel.cpp PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5671/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
>

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