[Development] Calendar Systems proposal

[Thiago Macieira]

On Saturday, 5 August 2017 05:08:23 PDT Soroush Rabiei wrote:
> I believe our proposed change (containing locale backend , date and time
> classes and related widgets) is ready to be merged (maybe after some minor
> improvements)
> 
> I would like to ask someone to review this change:
> 
> https://codereview.qt-project.org/#/c/182341/

Ok, a quick review's quick conclusions:

I mostly like your API. It looks simple and well integrated into QDateTime and 
QLocale. There are a couple of minor problems in the API itself, like 
QCalendarWidget::calendar() returning a reference. That's just wrong and needs 
work.

But there are bigger problems with the implementation, starting with 
QAbstractCalendar having a static protected QMap member. The big problem is 
how you've implemented the new API in QDateTime and QLocale. There's code 
duplication that cannot be there in QLocale, but the way you've removed the 
duplication in QDateTime also needs changing for performance reasons.

int QDate::year() const
{
    return year(QGregorianCalendar());
}

This creates a polymorphic object and makes a call that ends up delegating to 
it in

	    if (cal.julianDayToDate(jd, y, m, d))

The commit also includes changes that look like unrelated clean-ups and will 
need to be split into different commits. It's at this point lacking 
documentation and there are a couple of coding style mistakes.

In all, good work and I like how this is coming along, but it's not "ready for 
beta" right now, so it shouldn't be included in dev until after 5.11 opens for 
new features.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center