fix: drop orphaned overrides when a series stops, plus review minors

10 files changed, 162 insertions, 2 deletions

diff --git a/src/calendarstore.cpp b/src/calendarstore.cpp
index ef67eba..619ffb9 100644
--- a/src/calendarstore.cpp
+++ b/src/calendarstore.cpp
@@ -363,8 +363,25 @@ void writeFields(icalcomponent *root, icalcomponent *c, const EventEdit &edit, b
     if (!master || edit.repeat.custom)
         return;  // a custom RRULE is never rewritten (spec, repeat control)
     removeAll(c, ICAL_RRULE_PROPERTY);
-    if (edit.repeat.freq == RepeatRule::Freq::None)
+    if (edit.repeat.freq == RepeatRule::Freq::None) {
+        // Stopping a series orphans its RECURRENCE-ID overrides: occurrences()
+        // skips the override loop for a non-repeating event, so they would
+        // vanish from every view while staying in the synced file, ready to
+        // spring back if a repeat is re-enabled. EXDATEs are inert once the
+        // series does not repeat, and removing them would drop data, so they
+        // stay. Collect first: removing a component mid-walk invalidates it.
+        QList<icalcomponent *> doomed;
+        for (icalcomponent *v = icalcomponent_get_first_component(root, ICAL_VEVENT_COMPONENT);
+             v; v = icalcomponent_get_next_component(root, ICAL_VEVENT_COMPONENT)) {
+            if (v != c && icalcomponent_get_first_property(v, ICAL_RECURRENCEID_PROPERTY))
+                doomed.append(v);
+        }
+        for (icalcomponent *v : doomed) {
+            icalcomponent_remove_component(root, v);
+            icalcomponent_free(v);
+        }
         return;
+    }
     // UNTIL follows DTSTART's type: a DATE for all-day, else UTC (RFC 5545).
     QString until;
     if (edit.repeat.end == RepeatRule::End::Until) {
@@ -667,8 +684,13 @@ bool sameMeaning(const QByteArray &a, const QByteArray &b)
             || p.end != q.end || p.summary != q.summary || p.cancelled != q.cancelled)
             return false;
     }
+    // EXDATE properties are a SET: a server may reorder them, so compare
+    // sorted copies rather than the lists as read.
+    QList<QDateTime> xd = x.exdates, yd = y.exdates;
+    std::sort(xd.begin(), xd.end());
+    std::sort(yd.begin(), yd.end());
     return x.summary == y.summary && x.start == y.start && x.end == y.end
-        && x.allDay == y.allDay && x.repeat == y.repeat && x.exdates == y.exdates;
+        && x.allDay == y.allDay && x.repeat == y.repeat && xd == yd;
 }
 
 }  // namespace CalendarStore
diff --git a/src/calendarsync.cpp b/src/calendarsync.cpp
index 38d0c07..fa85c1c 100644
--- a/src/calendarsync.cpp
+++ b/src/calendarsync.cpp
@@ -29,6 +29,7 @@ CalendarSync::CalendarSync(const QString &command, int delayMs, QObject *parent)
             [this]() { m_output += m_process.readAll(); });
     connect(&m_process, &QProcess::finished, this,
             [this](int code, QProcess::ExitStatus status) {
+        m_output += m_process.readAll();  // drain the last chunk, delivered with finished
         emit finished(status == QProcess::NormalExit && code == 0,
                       QString::fromLocal8Bit(m_output));
         if (m_again) {
diff --git a/src/calendarwindow.cpp b/src/calendarwindow.cpp
index 7079c62..d9c1da2 100644
--- a/src/calendarwindow.cpp
+++ b/src/calendarwindow.cpp
@@ -145,6 +145,8 @@ CalendarWindow::CalendarWindow(const Config &config, const QStringList &ownAddre
     if (index >= 0)
         m_collectionBox->setCurrentIndex(index);
     (agenda ? m_agendaButton : m_monthButton)->click();
+    if (agenda)
+        m_agendaView->scrollToDay(QDate::currentDate());
 }
 
 CalendarWindow::~CalendarWindow() = default;
@@ -679,6 +681,9 @@ bool CalendarWindow::applyChanges(const QList<Change> &changes, bool reverse)
             for (int j = done.size() - 1; j >= 0; --j) {
                 QString ignored;
                 CalendarWriter::replace(done[j].path, done[j].after, done[j].before, &ignored);
+                // The write was reverted, so it must not be reported as a
+                // difference at the next sync.
+                m_written.remove(done[j].path);
             }
             status(r == CalendarWriter::Result::Stale
                        ? tr("The event changed on disk, probably from a sync. Check it and try again.")
diff --git a/src/calendarwriter.cpp b/src/calendarwriter.cpp
index 4c81a37..b39dced 100644
--- a/src/calendarwriter.cpp
+++ b/src/calendarwriter.cpp
@@ -49,6 +49,11 @@ Result replace(const QString &path, const std::optional<QByteArray> &expected,
         return Result::Ok;
     }
 
+    // ponytail: the stale read above and QSaveFile::commit() are not one
+    // atomic operation; a cron sync landing between them is lost. The writer
+    // holds no lock, because the sync command owns the flock. Upgrade path:
+    // take the same lock here if that window ever costs real data.
+    //
     // QSaveFile is the platform's atomic write: a temporary in the same
     // directory, renamed on commit(). Its temporary is named "<name>.XXXXXX",
     // which does not end in .ics, so vdirsyncer never lists it.
diff --git a/src/config.cpp b/src/config.cpp
index 23c08f7..14d7775 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -303,6 +303,9 @@ void Config::load(const QString &path)
         const int value = calDelay.toString().toInt(&ok);
         if (ok && value >= 0)
             m_calendarSyncDelayMs = value;
+        else if (ok)
+            addProblem(tr("Calendar sync delay %1 is out of range; using the default.")
+                           .arg(calDelay.toString()));
         else
             addProblem(tr("Calendar sync delay '%1' is not a number; using the default.")
                            .arg(calDelay.toString()));
diff --git a/tests/test_calendarstore.cpp b/tests/test_calendarstore.cpp
index 524304e..8aa526e 100644
--- a/tests/test_calendarstore.cpp
+++ b/tests/test_calendarstore.cpp
@@ -152,10 +152,12 @@ private slots:
     void aNewEventIsCompleteAndEmbedsItsZone();
     void sameMeaningIgnoresFormatting();
     void sameMeaningMatchesOverridesRegardlessOfOrder();
+    void sameMeaningTreatsExdatesAsASet();
     void sameMeaningRejectsDifferentUids();
 
     void editingOneOccurrenceWritesAnOverride();
     void editingTheSameOccurrenceAgainReplacesItsOverride();
+    void stoppingASeriesDropsItsOverrides();
     void deletingOneOccurrenceAddsAnExdateAndDropsItsOverride();
     void deletingOneOccurrenceKeepsExistingExdates();
 
@@ -595,6 +597,25 @@ void TestCalendarStore::sameMeaningMatchesOverridesRegardlessOfOrder()
     QVERIFY(!CalendarStore::sameMeaning(a, ics(master + second + changed)));
 }
 
+void TestCalendarStore::sameMeaningTreatsExdatesAsASet()
+{
+    // EXDATE properties are a set; a server may reorder them on the round
+    // trip, and a list comparison would then warn about a version it kept.
+    const QByteArray a = ics(vevent(QStringLiteral(
+        "UID:ex@example.org\r\nDTSTART:20260922T080000Z\r\n"
+        "EXDATE:20260923T080000Z\r\nEXDATE:20260924T080000Z\r\n")));
+    const QByteArray b = ics(vevent(QStringLiteral(
+        "UID:ex@example.org\r\nDTSTART:20260922T080000Z\r\n"
+        "EXDATE:20260924T080000Z\r\nEXDATE:20260923T080000Z\r\n")));
+    QVERIFY(CalendarStore::sameMeaning(a, b));
+
+    // Only the order is ignored, not the contents.
+    const QByteArray changed = ics(vevent(QStringLiteral(
+        "UID:ex@example.org\r\nDTSTART:20260922T080000Z\r\n"
+        "EXDATE:20260923T080000Z\r\nEXDATE:20260925T080000Z\r\n")));
+    QVERIFY(!CalendarStore::sameMeaning(a, changed));
+}
+
 void TestCalendarStore::sameMeaningRejectsDifferentUids()
 {
     // Identical fields, different events: the UID is the identity.
@@ -647,6 +668,34 @@ void TestCalendarStore::editingTheSameOccurrenceAgainReplacesItsOverride()
     QCOMPARE(e.overrides[0].start, rome(22, 17));
 }
 
+void TestCalendarStore::stoppingASeriesDropsItsOverrides()
+{
+    // A series with one overridden occurrence, then the repeat is stopped
+    // (Scope::All with RepeatRule() = Freq::None). The override is not a real
+    // occurrence any more: occurrences() never reads it for a non-repeating
+    // event, so leaving it in the file is invalid iCalendar that other clients
+    // may render as a phantom, and it would spring back if a repeat were
+    // re-enabled. EXDATEs are left inert.
+    const CalEvent series = parse(vevent(kDailySeries));
+    EventEdit edit = editOf(series);
+    edit.summary = QStringLiteral("Just this one");
+    edit.start = rome(22, 15);
+    edit.end = rome(22, 16);
+    const QByteArray withOverride = CalendarStore::applyEdit(
+        series.rawText, edit, CalendarStore::Scope::ThisOccurrence, rome(22, 10));
+    QCOMPARE(reparse(withOverride).overrides.size(), 1);
+
+    EventEdit stop = editOf(reparse(withOverride));
+    stop.repeat = RepeatRule();  // does not repeat
+    const QByteArray after = CalendarStore::applyEdit(
+        withOverride, stop, CalendarStore::Scope::All, {});
+
+    const CalEvent e = reparse(after);
+    QVERIFY(e.overrides.isEmpty());
+    QVERIFY2(!after.contains("RECURRENCE-ID"), after.constData());
+    QVERIFY2(!after.contains("RRULE"), after.constData());
+}
+
 void TestCalendarStore::deletingOneOccurrenceAddsAnExdateAndDropsItsOverride()
 {
     const CalEvent series = parse(vevent(kDailySeries));
diff --git a/tests/test_calendarwindow.cpp b/tests/test_calendarwindow.cpp
index c52f245..5591b2b 100644
--- a/tests/test_calendarwindow.cpp
+++ b/tests/test_calendarwindow.cpp
@@ -27,6 +27,7 @@
 #include <QMessageBox>
 #include <QPushButton>
 #include <QSpinBox>
+#include <QStatusBar>
 #include <QTemporaryDir>
 #include <QTimer>
 #include <QToolBar>
@@ -126,6 +127,7 @@ private slots:
     void theToolbarCarriesTheNewEventAction();
     void aFreshWindowShowsTheCurrentMonth();
     void aStaleSaveCanBeRetried();
+    void aRolledBackWriteIsNotReportedLostAfterSync();
 };
 
 void TestCalendarWindow::loadsAndSelects()
@@ -267,5 +269,56 @@ void TestCalendarWindow::aStaleSaveCanBeRetried()
     QVERIFY(!w->isEditing());
 }
 
+void TestCalendarWindow::aRolledBackWriteIsNotReportedLostAfterSync()
+{
+    // applyChanges records every successful write in m_written for the next
+    // sync to verify. A later change in the same call fails, rolling the
+    // earlier one back, so that write never reached disk and must be
+    // forgotten. Keeping it makes the next sync compare the file against
+    // bytes that were reverted and warn that the server kept a different
+    // version, a difference that never happened.
+    QTemporaryDir dir;
+    const QString cal = dir.filePath(QStringLiteral("cal"));
+    const QString one = cal + QStringLiteral("/a/one.ics");
+    const QString two = cal + QStringLiteral("/a/two.ics");
+    const QByteArray original =
+        eventText(QStringLiteral("one@example.org"), QStringLiteral("One"), 22);
+    writeFile(one, original);
+    writeFile(two, eventText(QStringLiteral("two@example.org"), QStringLiteral("Two"), 23));
+
+    // A real sync command, so the post-sync check actually runs.
+    const QString ini = dir.filePath(QStringLiteral("qtmaildir.conf"));
+    writeFile(ini, QStringLiteral("[general]\ncalendars_dir = %1\n"
+                                  "calendar_sync_command = /bin/true\n"
+                                  "calendar_sync_delay_ms = 0\n").arg(cal).toUtf8());
+    Config config;
+    config.load(ini);
+    std::unique_ptr<CalendarWindow> w(
+        new CalendarWindow(config, { QStringLiteral("me@example.org") },
+                           dir.filePath(QStringLiteral("uistate.conf"))));
+
+    // Change 1 succeeds; change 2 is stale (its expected bytes do not match a
+    // missing file), so change 1 is rolled back.
+    const QByteArray rolled =
+        eventText(QStringLiteral("one@example.org"), QStringLiteral("Rolled"), 22);
+    const QList<CalendarWindow::Change> changes = {
+        { one, original, rolled },
+        { cal + QStringLiteral("/a/ghost.ics"), QByteArray("gone"), QByteArray("x") },
+    };
+    QVERIFY(!w->applyChanges(changes, false));
+    QCOMPARE(read(one), original);  // the first change was reverted
+
+    // A later successful write triggers the sync. The rolled-back path must
+    // not reappear in the post-sync check: with the fix the sync reports
+    // clean, without it the stale entry produces a false "different version".
+    const QByteArray twoAfter =
+        eventText(QStringLiteral("two@example.org"), QStringLiteral("Two, edited"), 23);
+    QVERIFY(w->applyChanges({ { two, read(two), twoAfter } }, false));
+
+    QTRY_VERIFY_WITH_TIMEOUT(
+        w->statusBar()->currentMessage().contains(QStringLiteral("Calendars synced.")),
+        5000);
+}
+
 QTEST_MAIN(TestCalendarWindow)
 #include "test_calendarwindow.moc"
diff --git a/tests/test_config.cpp b/tests/test_config.cpp
index be343d1..b0597de 100644
--- a/tests/test_config.cpp
+++ b/tests/test_config.cpp
@@ -154,6 +154,7 @@ private slots:
     void calendarSyncCommandSurvivesACommaAndDefaults();
     void calendarsDirThatDoesNotExistIsReported();
     void calendarSyncDelayRejectsGarbage();
+    void aNegativeCalendarSyncDelayIsOutOfRange();
 };
 
 static QString writeIni(const QTemporaryDir &dir, const QString &body)
@@ -2963,5 +2964,19 @@ void TestConfig::calendarSyncDelayRejectsGarbage()
     QVERIFY(!config.problems().isEmpty());
 }
 
+void TestConfig::aNegativeCalendarSyncDelayIsOutOfRange()
+{
+    // "-5" parses, so "is not a number" would be a lie; the value is simply
+    // outside what the key accepts. It falls back to the default and says so.
+    QTemporaryDir dir;
+    Config config;
+    config.load(writeIni(dir, QStringLiteral("[general]\n"
+                                             "calendar_sync_delay_ms=-5\n")));
+    QCOMPARE(config.calendarSyncDelayMs(), 2000);
+    const QString joined = config.problems().join(QLatin1Char('\n'));
+    QVERIFY2(joined.contains(QStringLiteral("out of range")), qPrintable(joined));
+    QVERIFY2(!joined.contains(QStringLiteral("not a number")), qPrintable(joined));
+}
+
 QTEST_MAIN(TestConfig)
 #include "test_config.moc"
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 5446780..f6f75aa 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -8833,6 +8833,9 @@ void TestMainWindow::theCalendarActionWithNoCalendarOpensNothing()
     MainWindow window(config);
     auto *action = window.findChild<QAction *>(QStringLiteral("calendar"));
     QVERIFY2(action, "no action named calendar");
+    // Enabled, not disabled: a future edit that greys it out would otherwise
+    // make the trigger below a no-op and leave this test passing vacuously.
+    QVERIFY2(action->isEnabled(), "the calendar action is disabled");
 
     action->trigger();
 
diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts
index 6012a90..35702ae 100644
--- a/translations/qtmaildir_it_IT.ts
+++ b/translations/qtmaildir_it_IT.ts
@@ -469,6 +469,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation>
         <translation>Il ritardo di sincronizzazione del calendario &apos;%1&apos; non è un numero; uso il valore predefinito.</translation>
     </message>
     <message>
+        <source>Calendar sync delay %1 is out of range; using the default.</source>
+        <translation>Il ritardo di sincronizzazione del calendario %1 è fuori intervallo; uso il valore predefinito.</translation>
+    </message>
+    <message>
         <source>Language &apos;%1&apos; is not a locale name; using the system language. Expected something like &apos;it&apos; or &apos;it_IT&apos;.</source>
         <translation>&apos;%1&apos; non è un nome di locale; verrà usata la lingua di sistema. Atteso qualcosa come &apos;it&apos; o &apos;it_IT&apos;.</translation>
     </message>