Fix handling of day-of-week in QDateTimeParser and QDateTimeEdit

QDTP's absoluteMax(), setDigit() and getDigit() simply treated
day-of-week as synonym for day-of-month.
Consequently, QDTE::stepBy() did the same.

This meant that wrapping happened at the month boundary, so would jump
within the week if it wrapped around, otherwise the up/down arrow
would "jam" at a particular day of the week when further steps would
leave the month. Instead, when wrapping, wrap round the week while
still moving the day-of-month to match, jumping back or forward a week
to stay within the month on hitting a month boundary; otherwise, stop
backwards stepping on hitting the locale-specific day of the week, or
forward stepping when the step would be to or past this first day.
Fixed various bugs found in the course of testing this.

[ChangeLog][QtWidgets][QDateTimeEdit] Corrected handling of weekdays.
Previously, changes to the week-day were simply changes to the day of
the month. Weekday fields are now handled as such: changes to them do
change the day of the month, but a change that would step past the end
(or start) of the month is adjusted to the relevant day of the nearest
week within the month. When wrapping is disabled, the locale-specific
first and last days of the week are the bounds. Formats which specify
day of week but not day of month will now preserve day of week when
changing month or year, selecting the nearest day of month that
matches.

Change-Id: I7868b000fea7a4bc17a1b5687c44bcd56d42ae90
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

3 files changed, 138 insertions, 40 deletions

diff --git a/src/corelib/time/qdatetimeparser.cpp b/src/corelib/time/qdatetimeparser.cpp
index 0741ddcc57..9117e74375 100644
--- a/src/corelib/time/qdatetimeparser.cpp
+++ b/src/corelib/time/qdatetimeparser.cpp
@@ -96,7 +96,7 @@ int QDateTimeParser::getDigit(const QDateTime &t, int index) const
     case MonthSection: return t.date().month(calendar);
     case DaySection: return t.date().day(calendar);
     case DayOfWeekSectionShort:
-    case DayOfWeekSectionLong: return t.date().day(calendar);
+    case DayOfWeekSectionLong: return calendar.dayOfWeek(t.date());
     case AmPmSection: return t.time().hour() > 11 ? 1 : 0;
 
     default: break;
@@ -108,6 +108,34 @@ int QDateTimeParser::getDigit(const QDateTime &t, int index) const
 }
 
 /*!
+    \internal
+    Difference between two days of the week.
+
+    Returns a difference in the range from -3 through +3, so that steps by small
+    numbers of days move us through the month in the same direction as through
+    the week.
+*/
+
+static int dayOfWeekDiff(int sought, int held)
+{
+    const int diff = sought - held;
+    return diff < -3 ? diff + 7 : diff > 3 ? diff - 7 : diff;
+}
+
+static bool preferDayOfWeek(const QList<QDateTimeParser::SectionNode> &nodes)
+{
+    // True precisely if there is a day-of-week field but no day-of-month field.
+    bool result = false;
+    for (const auto &node : nodes) {
+        if (node.type & QDateTimeParser::DaySection)
+            return false;
+        if (node.type & QDateTimeParser::DayOfWeekSectionMask)
+            result = true;
+    }
+    return result;
+}
+
+/*!
   \internal
   Sets a digit in a datetime. E.g.
 
@@ -127,9 +155,12 @@ bool QDateTimeParser::setDigit(QDateTime &v, int index, int newVal) const
         return false;
     }
 
-    QCalendar::YearMonthDay date = calendar.partsFromDate(v.date());
+    const QDate oldDate = v.date();
+    QCalendar::YearMonthDay date = calendar.partsFromDate(oldDate);
     if (!date.isValid())
         return false;
+    int weekDay = calendar.dayOfWeek(oldDate);
+    enum { NoFix, MonthDay, WeekDay } fixDay = NoFix;
 
     const QTime time = v.time();
     int hour = time.hour();
@@ -150,8 +181,6 @@ bool QDateTimeParser::setDigit(QDateTime &v, int index, int newVal) const
     case YearSection: date.year = newVal; break;
     case MonthSection: date.month = newVal; break;
     case DaySection:
-    case DayOfWeekSectionShort:
-    case DayOfWeekSectionLong:
         if (newVal > 31) {
             // have to keep legacy behavior. setting the
             // date to 32 should return false. Setting it
@@ -159,6 +188,15 @@ bool QDateTimeParser::setDigit(QDateTime &v, int index, int newVal) const
             return false;
         }
         date.day = newVal;
+        fixDay = MonthDay;
+        break;
+    case DayOfWeekSectionShort:
+    case DayOfWeekSectionLong:
+        if (newVal > 7 || newVal <= 0)
+            return false;
+        date.day += dayOfWeekDiff(newVal, weekDay);
+        weekDay = newVal;
+        fixDay = WeekDay;
         break;
     case TimeZoneSection:
         if (newVal < absoluteMin(index) || newVal > absoluteMax(index))
@@ -176,9 +214,27 @@ bool QDateTimeParser::setDigit(QDateTime &v, int index, int newVal) const
     if (!(node.type & DaySectionMask)) {
         if (date.day < cachedDay)
             date.day = cachedDay;
+        fixDay = MonthDay;
+        if (weekDay > 0 && weekDay <= 7 && preferDayOfWeek(sectionNodes)) {
+            const int max = calendar.daysInMonth(date.month, date.year);
+            if (max > 0 && date.day > max)
+                date.day = max;
+            const int newDoW = calendar.dayOfWeek(calendar.dateFromParts(date));
+            if (newDoW > 0 && newDoW <= 7)
+                date.day += dayOfWeekDiff(weekDay, newDoW);
+            fixDay = WeekDay;
+        }
+    }
+
+    if (fixDay != NoFix) {
         const int max = calendar.daysInMonth(date.month, date.year);
-        if (date.day > max)
-            date.day = max;
+        // max > 0 precisely if the year does have such a month
+        if (max > 0 && date.day > max)
+            date.day = fixDay == WeekDay ? date.day - 7 : max;
+        else if (date.day < 1)
+            date.day = fixDay == WeekDay ? date.day + 7 : 1;
+        Q_ASSERT(fixDay != WeekDay
+                 || calendar.dayOfWeek(calendar.dateFromParts(date)) == weekDay);
     }
 
     const QDate newDate = calendar.dateFromParts(date);
@@ -231,9 +287,10 @@ int QDateTimeParser::absoluteMax(int s, const QDateTime &cur) const
     case MonthSection:
         return calendar.maximumMonthsInYear();
     case DaySection:
+        return cur.isValid() ? cur.date().daysInMonth(calendar) : calendar.maximumDaysInMonth();
     case DayOfWeekSectionShort:
     case DayOfWeekSectionLong:
-        return cur.isValid() ? cur.date().daysInMonth(calendar) : calendar.maximumDaysInMonth();
+        return 7;
     case AmPmSection:
         return 1;
     default:
@@ -927,19 +984,21 @@ QDateTimeParser::parseSection(const QDateTime &currentValue, int sectionIndex, i
 /*!
   \internal
 
-  Returns a day-number, in the same month as \a rough and as close to \a rough's
-  day number as is valid, that \a calendar puts on the day of the week indicated
-  by \a weekDay.
+  Returns the day-number of a day, as close as possible to the given \a day, in
+  the specified \a month of \a year for the given \a calendar, that falls on the
+  day of the week indicated by \a weekDay.
 */
 
-static int weekDayWithinMonth(QCalendar calendar, QDate rough, int weekDay)
+static int weekDayWithinMonth(QCalendar calendar, int year, int month, int day, int weekDay)
 {
     // TODO: can we adapt this to cope gracefully with intercallary days (day of
     // week > 7) without making it slower for more widely-used calendars ?
-    int day = rough.day(calendar) + weekDay - calendar.dayOfWeek(rough);
+    const int maxDay = calendar.daysInMonth(month, year); // 0 if no such month
+    day = maxDay > 1 ? qBound(1, day, maxDay) : qMax(1, day);
+    day += dayOfWeekDiff(weekDay, calendar.dayOfWeek(QDate(year, month, day, calendar)));
     if (day <= 0)
         return day + 7;
-    if (day > rough.daysInMonth(calendar))
+    if (maxDay > 0 && day > maxDay)
         return day - 7;
     return day;
 }
@@ -1027,7 +1086,7 @@ static QDate actualDate(QDateTimeParser::Sections known, const QCalendar &calend
 
     if ((known & QDateTimeParser::DaySection) == 0) {
         // Relatively easy to fix.
-        day = weekDayWithinMonth(calendar, actual, dayofweek);
+        day = weekDayWithinMonth(calendar, year, month, day, dayofweek);
         actual = QDate(year, month, day, calendar);
         return actual;
     }
@@ -1300,24 +1359,26 @@ QDateTimeParser::scanString(const QDateTime &defaultValue, bool fixup) const
             }
         }
 
+        const auto fieldType = sectionType(currentSectionIndex);
         const QDate date(year, month, day, calendar);
-        if (dayofweek != calendar.dayOfWeek(date)
+        if ((!date.isValid() || dayofweek != calendar.dayOfWeek(date))
                 && state == Acceptable && isSet & DayOfWeekSectionMask) {
             if (isSet & DaySection)
                 conflicts = true;
-            const SectionNode &sn = sectionNode(currentSectionIndex);
-            if (sn.type & DayOfWeekSectionMask || currentSectionIndex == -1) {
-                // dayofweek should be preferred
-                day = weekDayWithinMonth(calendar, date, dayofweek);
+            // Change to day of week should adjust day of month;
+            // when day of month isn't set, so should change to year or month.
+            if (currentSectionIndex == -1 || fieldType & DayOfWeekSectionMask
+                    || (!conflicts && (fieldType & (YearSectionMask | MonthSection)))) {
+                day = weekDayWithinMonth(calendar, year, month, day, dayofweek);
                 QDTPDEBUG << year << month << day << dayofweek
                           << calendar.dayOfWeek(QDate(year, month, day, calendar));
             }
         }
 
         bool needfixday = false;
-        if (sectionType(currentSectionIndex) & DaySectionMask) {
+        if (fieldType & DaySectionMask) {
             cachedDay = day;
-        } else if (cachedDay > day) {
+        } else if (cachedDay > day && !(isSet & DayOfWeekSectionMask && state == Acceptable)) {
             day = cachedDay;
             needfixday = true;
         }
diff --git a/src/widgets/widgets/qdatetimeedit.cpp b/src/widgets/widgets/qdatetimeedit.cpp
index 5b3d0f3c2a..b150827a4d 100644
--- a/src/widgets/widgets/qdatetimeedit.cpp
+++ b/src/widgets/widgets/qdatetimeedit.cpp
@@ -2101,15 +2101,45 @@ QDateTime QDateTimeEditPrivate::stepBy(int sectionIndex, int steps, bool test) c
         v = q->dateTimeFromText(str);
     int val = getDigit(v, sectionIndex);
 
-    val += steps;
-
     const int min = absoluteMin(sectionIndex);
     const int max = absoluteMax(sectionIndex, value.toDateTime());
 
-    if (val < min) {
-        val = (wrapping ? max - (min - val) + 1 : min);
-    } else if (val > max) {
-        val = (wrapping ? min + val - max - 1 : max);
+    if (sn.type & DayOfWeekSectionMask) {
+        // Must take locale's first day of week into account when *not*
+        // wrapping; min and max don't help us.
+#ifndef QT_ALWAYS_WRAP_WEEKDAY // (documentation, not an actual define)
+        if (!wrapping) {
+            /* It's not clear this is ever really a desirable behavior.
+
+               It refuses to step backwards from the first day of the week or
+               forwards from the day before, only allowing day-of-week stepping
+               from start to end of one week. That's strictly what non-wrapping
+               behavior must surely mean, when put in locale-neutral terms.
+
+               It is, however, likely that users would prefer the "more natural"
+               behavior of cycling through the week.
+            */
+            const int first = int(locale().firstDayOfWeek()); // Mon = 1 through 7 = Sun
+            val = qBound(val < first ? first - 7 : first,
+                         val + steps,
+                         val < first ? first - 1 : first + 6);
+        } else
+#endif
+        {
+            val += steps;
+        }
+
+        // Restore to range from 1 through 7:
+        val = val % 7;
+        if (val <= 0)
+            val += 7;
+    } else {
+        val += steps;
+        const int span = max - min + 1;
+        if (val < min)
+            val = wrapping ? val + span : min;
+        else if (val > max)
+            val = wrapping ? val - span : max;
     }
 
     const int oldDay = v.date().day(calendar);
@@ -2132,7 +2162,8 @@ QDateTime QDateTimeEditPrivate::stepBy(int sectionIndex, int steps, bool test) c
     const QDateTime minimumDateTime = minimum.toDateTime();
     const QDateTime maximumDateTime = maximum.toDateTime();
     // changing one section should only modify that section, if possible
-    if (sn.type != AmPmSection && (v < minimumDateTime || v > maximumDateTime)) {
+    if (sn.type != AmPmSection && !(sn.type & DayOfWeekSectionMask)
+        && (v < minimumDateTime || v > maximumDateTime)) {
         const int localmin = getDigit(minimumDateTime, sectionIndex);
         const int localmax = getDigit(maximumDateTime, sectionIndex);
 
diff --git a/tests/auto/widgets/widgets/qdatetimeedit/tst_qdatetimeedit.cpp b/tests/auto/widgets/widgets/qdatetimeedit/tst_qdatetimeedit.cpp
index 5f003928c0..ad16fcc79f 100644
--- a/tests/auto/widgets/widgets/qdatetimeedit/tst_qdatetimeedit.cpp
+++ b/tests/auto/widgets/widgets/qdatetimeedit/tst_qdatetimeedit.cpp
@@ -4009,20 +4009,23 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     KeyPairList shortAndLongNameIssueKeypresses;
     shortAndLongNameIssueKeypresses << key(Qt::Key_Tab) << key(Qt::Key_3) << key(Qt::Key_1) << key(Qt::Key_Up);
 
+    // When day-of-week is specified, rather than day-of-month, changing month
+    // cares more about preserving day-of-week than day-of-month, so Jan/31 ->
+    // Feb picks 28th even in a leap year, as that's exactly four weeks later.
     QTest::newRow("no fixday, leap, yy/M/dddd")
         << ozzy << y2kStart << QString::fromLatin1("yy/M/dddd")
         << threeDigitDayIssueKeypresses_DayName
-        << QDate(2000, 2, 29) << QString::fromLatin1("00/2/Tuesday");
+        << QDate(2000, 2, 28) << QString::fromLatin1("00/2/Monday");
 
     QTest::newRow("no fixday, leap, yy/M/ddd")
         << ozzy << y2kStart << QString::fromLatin1("yy/M/ddd")
         << threeDigitDayIssueKeypresses_DayName
-        << QDate(2000, 2, 29) << QString::fromLatin1("00/2/Tue");
+        << QDate(2000, 2, 28) << QString::fromLatin1("00/2/Mon");
 
     QTest::newRow("no fixday, leap, yy/MM/dddd")
         << ozzy << y2kStart << QString::fromLatin1("yy/MM/dddd")
         << threeDigitDayIssueKeypresses_DayName
-        << QDate(2000, 2, 29) << QString::fromLatin1("00/02/Tuesday");
+        << QDate(2000, 2, 28) << QString::fromLatin1("00/02/Monday");
 
     QTest::newRow("fixday, leap, yy/MM/dd")
         << ozzy << y2kStart << QString::fromLatin1("yy/MM/dd")
@@ -4072,12 +4075,12 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("no fixday, leap, yyyy/M/dddd")
         << ozzy << y2kStart << QString::fromLatin1("yyyy/M/dddd")
         << threeDigitDayIssueKeypresses_DayName
-        << QDate(2000, 2, 29) << QString::fromLatin1("2000/2/Tuesday");
+        << QDate(2000, 2, 28) << QString::fromLatin1("2000/2/Monday");
 
     QTest::newRow("no fixday, leap, yyyy/MM/dddd")
         << ozzy << y2kStart << QString::fromLatin1("yyyy/MM/dddd")
         << threeDigitDayIssueKeypresses_DayName
-        << QDate(2000, 2, 29) << QString::fromLatin1("2000/02/Tuesday");
+        << QDate(2000, 2, 28) << QString::fromLatin1("2000/02/Monday");
 
     QTest::newRow("fixday, leap, yyyy/dd/MM")
         << ozzy << y2kStart << QString::fromLatin1("yyyy/dd/MM")
@@ -4112,12 +4115,12 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("fixday, leap, yyyy/dddd/M")
         << ozzy << y2kStart << QString::fromLatin1("yyyy/dddd/M")
         << threeDigitDayIssueKeypresses_DayName_YearDayMonth
-        << QDate(2000, 2, 29) << QString::fromLatin1("2000/Tuesday/2");
+        << QDate(2000, 2, 28) << QString::fromLatin1("2000/Monday/2");
 
     QTest::newRow("fixday, leap, yyyy/dddd/MM")
         << ozzy << y2kStart << QString::fromLatin1("yyyy/dddd/MM")
         << threeDigitDayIssueKeypresses_DayName_YearDayMonth
-        << QDate(2000, 2, 29) << QString::fromLatin1("2000/Tuesday/02");
+        << QDate(2000, 2, 28) << QString::fromLatin1("2000/Monday/02");
 
     QTest::newRow("fixday, leap, d/M/yyyy")
         << ozzy << y2kStart << QString::fromLatin1("d/M/yyyy")
@@ -4137,7 +4140,7 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("fixday, leap, dddd/MM/yyyy")
         << ozzy << y2kStart << QString::fromLatin1("dddd/MM/yyyy")
         << threeDigitDayIssueKeypresses_DayName_DayMonthYear
-        << QDate(2000, 2, 29) << QString::fromLatin1("Tuesday/02/2000");
+        << QDate(2000, 2, 28) << QString::fromLatin1("Monday/02/2000");
 
     QTest::newRow("fixday, leap, d/yy/M")
         << ozzy << y2kStart << QString::fromLatin1("d/yy/M")
@@ -4172,12 +4175,12 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("fixday, leap, dddd/yy/M")
         << ozzy << y2kStart << QString::fromLatin1("dddd/yy/M")
         << threeDigitDayIssueKeypresses_DayName_DayYearMonth
-        << QDate(2000, 2, 29) << QString::fromLatin1("Tuesday/00/2");
+        << QDate(2000, 2, 28) << QString::fromLatin1("Monday/00/2");
 
     QTest::newRow("fixday, leap, dddd/yy/MM")
         << ozzy << y2kStart << QString::fromLatin1("dddd/yy/MM")
         << threeDigitDayIssueKeypresses_DayName_DayYearMonth
-        << QDate(2000, 2, 29) << QString::fromLatin1("Tuesday/00/02");
+        << QDate(2000, 2, 28) << QString::fromLatin1("Monday/00/02");
 
     QTest::newRow("fixday, leap, M/d/yy")
         << ozzy << y2kStart << QString::fromLatin1("M/d/yy")
@@ -4197,7 +4200,7 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("fixday, leap, M/dddd/yyyy")
         << ozzy << y2kStart << QString::fromLatin1("M/dddd/yyyy")
         << threeDigitDayIssueKeypresses_DayName_MonthDayYear
-        << QDate(2000, 2, 29) << QString::fromLatin1("2/Tuesday/2000");
+        << QDate(2000, 2, 28) << QString::fromLatin1("2/Monday/2000");
 
     QTest::newRow("fixday, leap, MM/dd/yyyy")
         << ozzy << y2kStart << QString::fromLatin1("MM/dd/yyyy")
@@ -4207,7 +4210,7 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize_data()
     QTest::newRow("fixday, leap, MM/dddd/yyyy")
         << ozzy << y2kStart << QString::fromLatin1("MM/dddd/yyyy")
         << threeDigitDayIssueKeypresses_DayName_MonthDayYear
-        << QDate(2000, 2, 29) << QString::fromLatin1("02/Tuesday/2000");
+        << QDate(2000, 2, 28) << QString::fromLatin1("02/Monday/2000");
 
     QTest::newRow("fixday, leap, M/yyyy/dd")
         << ozzy << y2kStart << QString::fromLatin1("M/yyyy/dd")
@@ -4275,6 +4278,9 @@ void tst_QDateTimeEdit::dateEditCorrectSectionSize()
     edit.setDisplayFormat(displayFormat);
     edit.show();
     edit.setFocus();
+    // Day-of-week tests rely on advance through week advancing the
+    // day-of-month, so not stopping at the locale's first day of the week:
+    edit.setWrapping(true);
     // For some reason, we need to set the selected section for the dd/MM/yyyy tests,
     // otherwise the 3 is inserted at the front of 01/01/2000 (301/01/2000), instead of the
     // selected text being replaced. This is not an issue for the yyyy/MM/dd format though...