Review Request: KHolidays: New Holiday Region Selection widget

[David Jarvie]

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


I wonder whether it would be better to group countries by continent, as for time zone selection. One reason might be if there were cases where a user's country is not in the list, but a neighbouring country might supply suitable holidays - in this case, grouping by continent would make it easier to find a neighbouring country (bearing in mind that not all other countries will be in the list either).

I don't think the default column heading "Select" is intuitive enough when "Days Off" is displayed alongside. I suspect that many people (myself included) would normally equate "holiday" with "day off". In this case, it isn't obvious what the "Select" column would be for. Having some acquaintance with KDE holiday files, I think I may know what the distinction is, but without knowing KDE holiday files I'd be completely in the dark. In any case, it would be more natural for the "Days Off" column to precede the "Select" column (suitably renamed).

In the documentation in the header, there should be an explanation as to why or why not a developer should choose to display the Language column. It isn't obvious, to me at least, that language would be relevant to choosing a holiday region. If one language was spoken in one region and another was spoken in another region, then I'd expect the holidays to be different, but that would be because it was a different region. And if a single holiday had a different name in one language from another, I'd expect the displayed name to follow the user's locale language, not the holiday file's.

It's a pity that your indent will have to be changed to 2 spaces - it's much more legible using 4 spaces.


/trunk/KDE/kdepimlibs/kholidays/holidayregionselector.h
<http://svn.reviewboard.kde.org/r/5518/#comment8214>

    Change "the Holiday Region" to "a Holiday Region" would make it easier to understand. 



/trunk/KDE/kdepimlibs/kholidays/holidayregionselector.h
<http://svn.reviewboard.kde.org/r/5518/#comment8213>

    serach -> search


- David


On 2010-10-02 18:21:32, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5518/
> -----------------------------------------------------------
> 
> (Updated 2010-10-02 18:21:32)
> 
> 
> Review request for kdelibs and KDE PIM.
> 
> 
> Summary
> -------
> 
> Implement a standard widget in the KHolidays library for clients to use to allow users to select holiday regions to be displayed.  The widget provides a number of modes and features to fit most use case scenarios I can think of:
> * A display only mode
> * A single Holiday Region selection mode
> * A multiple Holiday Region selection mode with optional secondary selection for Days Off / preferred Holiday Region
> * A search line
> * Ability to modify level of detail displayed when space is limited
> 
> Some points to think about:
> 1) I've implemented Qt Designer plugin support for the widget using a KHolidays specific plugin, but should the Designer support actually be at kdepimlibs level instead, i.e. all kdepimlibs widgets in a  single shared plugin?
> 2) Naming of the widget, enum and api calls
> 3) The layout of the selection boxes and columns
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepimlibs/includes/KHolidays/HolidayRegionSelector PRE-CREATION 
>   /trunk/KDE/kdepimlibs/kholidays/CMakeLists.txt 1180311 
>   /trunk/KDE/kdepimlibs/kholidays/holidayregionselector.h PRE-CREATION 
>   /trunk/KDE/kdepimlibs/kholidays/holidayregionselector.cpp PRE-CREATION 
>   /trunk/KDE/kdepimlibs/kholidays/holidayregionselector.ui PRE-CREATION 
>   /trunk/KDE/kdepimlibs/kholidays/kholidays.widgets PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5518/diff
> 
> 
> Testing
> -------
> 
> Test client implementation done in the Plasma clock/calendar.
> 
> 
> Screenshots
> -----------
> 
> Widget example
>   http://svn.reviewboard.kde.org/r/5518/s/523/
> 
> 
> Thanks,
> 
> John
> 
>

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