This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5692/ |
On October 26th, 2010, 8:21 a.m., Sebastian Trueg wrote:
/trunk/KDE/kdelibs/kdecore/date/kdate.h (Diff revision 1) public:607 QString dayOfYearString(KCalendarSystem::StringFormat format = KCalendarSystem::LongFormat) const;an example would be good here since it is rather unclear what the method (and its friends below) return.On October 26th, 2010, 9:48 p.m., John Layt wrote:
I've reworked the apidox. I'm now rethinking the methods entirely. The KCalendarSystem api for yearString() and the rest is needed because you can't just display the year() number in the gui, the localized string form may have a different digit set or even a non-decimal or non-numeric form (Hebrew is the big case here). However, the more recent addition of a formatDate() version to the KCalendarSystem api where you can pass in your own format string (e.g. "%Y" for the long year) makes the reason for having them moot. I could remove them entirely, but the whole purpose of KLocalizedDate is to provide localized versions in a simple way. Perhaps slightly modify the name to remove the "String" and remove the default value to look like: int year() const; QString year(KCalendarSystem::StringFormat format) const; I think that might make it more obvious?On October 29th, 2010, 3 p.m., Sebastian Trueg wrote:
why not formatYear() to match the big brother method?
Nice, I'll take that. I was a little uneasy about using the same method name with different return types. Also allows me to set default values too.
On October 26th, 2010, 8:21 a.m., Sebastian Trueg wrote:
/trunk/KDE/kdelibs/kdecore/date/kdate.h (Diff revision 1) public:736 KDate readDate(const QString &dateString, bool *ok = 0) const;Confusing that this is not a static method.On October 26th, 2010, 10:36 p.m., John Layt wrote:
I had initially made it static, but I wondered if there would then be confusion as to what Calendar System would be used if accessed from an instantiated KDate rather than statically, e.g. called as myDate.readDate() rather than KDate::readDate(). Would the user expect the instantiated date's calendar system to be used rather than the global that it really would? Or should having clear documentation be enough?On October 29th, 2010, 3:02 p.m., Sebastian Trueg wrote:
IMHO a static version with an optional KCaldendarSystem parameter might do the trick. Although the latter might even be superfluous since using a custom caldendarsystem is not a simple usage pattern anymore in which case it would be ok for the developer to go back to using KCalendarSystem.
Yep, I'll probably go with the basic static and a big flashing neon warning :-) The other problem with passing in a calendar system to use is you are likely to be wanting a different locale with it too, i.e. different language or date format, so that's better using KCS anyway.
- John
On October 25th, 2010, 8:54 p.m., John Layt wrote:
Review request for kdelibs.
By John Layt.
Updated 2010-10-25 20:54:08 Description
Testing
Diffs
|