Review Request: A Facet and search GUI API for Nepomuk

John Layt johnlayt at googlemail.com
Fri Oct 22 20:05:34 BST 2010 

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


You're too fast :-)  Added some comments to r2 on the Date stuff.  I'll add some new api to KCalendarSystem to make your life easier.


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

    Isn't None a bit too generic?  Perhaps NoDateRange?



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

    Shouldn't assume 7 days in a week, use calendar()->daysInWeek(date) instead



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

    Already here :-)  Yes, the first / last days of the year will need localizing, but it's not pretty.  In fact, it's so ugly, I'll add some public convenience methods to KCalendarSystem for them instead as it's easy to get t wrong (I already have private ones).  I'll get that done this weekend, If not ready in time free free to commit and I'll clean up afterward :-)
    
    (The whole calendar api is just awkward anyway, I really need to do a KDate class to wrap it all).



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

    Now, where else in KDE have a seen a DateRange class?  Somewhere in kdepim?  Perhaps we need one in kdelibs?



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

    Bools are bad in public api, you should define an enum.



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

    You need to check is valid in the local calendar instead using calendar()->isValid(date)



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

    Technically, you should be using:
      int dayOfWeek = KLocale::locale()->calendar()->dayOfWeek( date )
    But there's no calendar systems yet that return a different value so its not wrong either :-)



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

    Should be localized, but I'll add startOfMonth() and endOfMonth() methods to KCalendarSystem as well to make it easy.



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

    Not fully localized, year needs to be included and may get last day for wrong month?  Anyway, new methods to come.



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

    Perhaps I need to supply an addWeeks() method?



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

    Wouldn't it be easier to use calendar()->addMonths(date,n) instead?



trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.ui
<http://svn.reviewboard.kde.org/r/5671/#comment8591>

    Why not KDatePicker or KDateTable?  It localizes the calendar to the KDE calendar, the Qt one will only ever give you Gregorian.  If you need extra features added let me know.


- John


On 2010-10-22 18:53:14, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5671/
> -----------------------------------------------------------
> 
> (Updated 2010-10-22 18:53:14)
> 
> 
> 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/Mainpage.dox 1187872 
>   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/20101022/1330c419/attachment.htm>

More information about the kde-core-devel mailing list