Review Request: KLocale / KCalendarSystem: Add new enum for CalendarSystem

John Layt johnlayt at googlemail.com
Fri Oct 29 19:33:28 BST 2010 


> On 2010-10-29 15:08:23, Sebastian Trueg wrote:
> > /trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.h, line 204
> > <http://svn.reviewboard.kde.org/r/5704/diff/1/?file=40339#file40339line204>
> >
> >     just as a sidenote: If in KDE5 you would make methods in this class virtual it would mean that one could implement custom calendarsystems.
> >     However, these would then not be reflected in the enum. Thus, they could not be managed in the same way, i.e. listed via calendarSystemList() and so on.

Ah, I'd forgotten that use case.  KCS was originally all virtual for that very reason, so that you could use a custom calendar object in the date widgets, but it never did allow you to set one as your global or system calendar, or allow you to return it in the KCS::create() or KCS::calendarSystems() static methods.  I guess a QString would allow you to load a dynamic list of names from a config file, but I don't see how you could use that to create a new object and return it?  Would a plugin work?  Would it even be worth it for the very arcane corner case at such a low level of kdecore?  I actually know of 2 people who did implement their own calendars, but they had hacked them into their own version of kdelibs.

Unfortunately I didn't know about reserving more virtual slots when we froze 4.0, so all new api since then has been non-virtual but using the virtual d_ptr spaghetti code, which killed the custom KCS stone dead.  In KDE5 I hope to get that back hence the 'KDE5 make virtual' comment on every new method, but it is something that needs thinking about, e.g. do I allow people to set their custom calendar as the global calendar for their app rather than just in isolated widgets?  It would make sense I think.  Unfortunately I can see no way to allow someone to do this at a desktop/system level, for that they need to compile it into kdelibs.  There's also the interdependency between KLocale and KCS that gets complicated.  But hey, I intend to support most calendars, if someones implemented something useful I'll integrate it, it's only historic or imaginary ones like the Mayan and French Revolutionary that I won't do :-)


- John


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


On 2010-10-28 12:56:04, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5704/
> -----------------------------------------------------------
> 
> (Updated 2010-10-28 12:56:04)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This is related to the KLocalizedDate review 5692.  I realised in that review that KLocalizedDate should now be the only api people use to localize dates, and so rather than exactly matching the old KCalendarSystem api I could make some changes to simplify / improve the api.  (The bits matching the QDate api won't change).
> 
> As part of this, I want to introduce an enum for CalendarSystem to replace the current QString Calendar Type, e.g. replace "hebrew" with KLocale::HebrewCalendar.  This is obviously clearer, safer and less error prone.
> 
> Note that I had to put the enum in KLocale rather than KCalendarSystem as the KCalendarSystem api already uses KLocale enums in the header, so I can't use any KCalendarSystem enums in the KLocale api and header (you can't forward-declare enums).
> 
> The awkward part was naming the enums, particularly the default QDate hybrid Julian/Gregorian system.  I chose to call this KLocale::QDateCalendar rather than KLocale::GregorianCalendar which I've used for the proper one.  Any better suggestions welcome.  I've also renamed the Hijri to IslamicCivilCalendar to distinguish from the lunar calendar.  I'll probably rename the derived calendar classes to match the enum, which should be BC safe as they are not exported, but I'll do that later to save cluttering up the diff. [Random thought: do we even need the private classes derived from the base class anymore, the derived private d_ptr class should be enough?]
> 
> You probably don't want to look too closely at the KCalendarSystem spaghetti, you'll go cross-eyed, the KLocale is of more interest :-)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemcoptic.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemcopticprivate_p.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemethiopian.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemgregorian.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemgregorianproleptic.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemgregorianprolepticprivate_p.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhebrew.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhijri.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemindiannational.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemjalali.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemjapanese.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemjulian.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemminguo.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemprivate_p.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/date/kcalendarsystemthai.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_p.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/tests/kcalendartest.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/tests/kcalendartest.cpp 1190608 
>   /trunk/KDE/kdelibs/kdecore/tests/klocaletest.h 1190608 
>   /trunk/KDE/kdelibs/kdecore/tests/klocaletest.cpp 1190608 
> 
> Diff: http://svn.reviewboard.kde.org/r/5704/diff
> 
> 
> Testing
> -------
> 
> All existing unit tests pass, new unit tests written.  (Note, ignore the KLocalizedDate tests below, it's just the overlap from the other review, they will be committed separately.)
> 
> 
> Thanks,
> 
> John
> 
>

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

More information about the kde-core-devel mailing list