From d093ec8d03fe2f6ce4b93075229951689e7e3ebb Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Fri, 28 Aug 2020 09:23:13 +0200 Subject: [PATCH] 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 Reviewed-by: Thiago Macieira Reviewed-by: Andrei Golubev Reviewed-by: Volker Hilsheimer --- src/corelib/time/qdatetimeparser.cpp | 103 ++++++++++++++---- src/widgets/widgets/qdatetimeedit.cpp | 45 ++++++-- .../qdatetimeedit/tst_qdatetimeedit.cpp | 30 +++-- 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; @@ -107,6 +107,34 @@ int QDateTimeParser::getDigit(const QDateTime &t, int index) const return -1; } +/*! + \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 &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 ¤tValue, 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...