From 1a842d8d140b7c857e668e30396f391c071a3058 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 3 Sep 2026 14:20:50 -0700 Subject: [PATCH] Build the two rule descriptions as sentences, not fragments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both describeRule functions assembled their output by concatenation, which no catalogue could fix. A translator handed " and " or " on " in isolation cannot move it: German puts the verb last, Japanese does not separate list items with a word at all, and the fragments arrive in an order the English sentence chose. Reported by a native speaker reviewing the German catalogue (#247), whose "the summaries" item is the Sieve one. Every branch is now one whole sentence with placeholders, so a translator rewrites the sentence including its word order. Joining is Intl.ListFormat, which gives "A, B und C" for an allof rule and the language's own disjunction for anyof, rather than a hardcoded " and " that would be wrong twice over. The recurrence tail no longer appends: ", 5 times" and ", until 2026-05-03" wrap the sentence they qualify, so a language that puts the limit first can. Ordinals become words. The old suffix table -- st, nd, rd, th, picked by arithmetic -- is English spelling rules in code, and no catalogue can reach a suffix chosen that way. German writes "1.", Japanese "第1". nthOfPeriod is 1-5 or -1 in practice, so five words and "last" cover it. WEEKDAYS is gone. Its long names could have been catalogue entries but its short ones never could: "T" is Tuesday and Thursday, "S" is Saturday and Sunday, and a catalogue cannot hold two translations under one key. That was bad data rather than missing translation, and Intl has every name in every locale in three widths. lib/datetime.ts gains weekdayName, weekdayNames and formatList; recurrence.ts keeps WEEKDAY_KEYS for the ordering, which is not a language question. Adds the first tests either function has had. Neither had any, and no test would have caught what was wrong with them, since the English output was correct -- so these pin the two properties that actually matter: fragments go through the catalogue, and the joining is Intl's. 32 strings and 9 plural forms are new and land with each language. Verified: typecheck clean, 1009 tests pass. --- web/src/lib/__tests__/describeRules.test.ts | 93 ++++++++++++++ web/src/lib/datetime.ts | 45 +++++++ web/src/lib/recurrence.ts | 132 +++++++++++++++----- web/src/lib/sieve.ts | 102 ++++++++------- web/src/views/calendar/EventEditor.tsx | 6 +- 5 files changed, 300 insertions(+), 78 deletions(-) create mode 100644 web/src/lib/__tests__/describeRules.test.ts diff --git a/web/src/lib/__tests__/describeRules.test.ts b/web/src/lib/__tests__/describeRules.test.ts new file mode 100644 index 0000000..1d2649b --- /dev/null +++ b/web/src/lib/__tests__/describeRules.test.ts @@ -0,0 +1,93 @@ +/** + * The two sentence builders, which had no tests while they were building + * English by concatenation -- and no test would have caught the thing wrong + * with them, since the English output was correct. These pin the two + * properties that matter now: every fragment goes through the catalogue, and + * the joining is Intl's rather than a hardcoded " and ". + */ +import { describe, expect, it } from "vitest"; +import { describeRule as describeSieve } from "../sieve"; +import { describeRule as describeRecurrence, weekdayOptions } from "../recurrence"; +import { setUiLanguageForFormatting } from "../datetime"; +import { setCatalog } from "../i18n"; + +describe("sieve describeRule", () => { + it("names the header and operator through the catalogue", () => { + const s = describeSieve({ + id: "1", name: "r", join: "allof", enabled: true, + tests: [{ type: "header", header: "subject", op: "contains", value: "invoice" }], + actions: [{ type: "fileinto", mailbox: "Work" }], + } as never); + expect(s).toContain("Subject"); + expect(s).toContain("contains"); + expect(s).toContain("invoice"); + expect(s).toContain("Work"); + }); + + it("joins an allof rule as a conjunction and anyof as a disjunction", () => { + const base = { + id: "1", name: "r", enabled: true, + tests: [ + { type: "header", header: "from", op: "is", value: "a@b" }, + { type: "header", header: "to", op: "is", value: "c@d" }, + ], + actions: [{ type: "keep" }], + }; + expect(describeSieve({ ...base, join: "allof" } as never)).toContain(" and "); + expect(describeSieve({ ...base, join: "anyof" } as never)).toContain(" or "); + }); + + it("says 'always' when a rule has no tests", () => { + const s = describeSieve({ id: "1", name: "r", join: "allof", enabled: true, tests: [], actions: [{ type: "stop" }] } as never); + expect(s).toContain("always"); + }); +}); + +describe("recurrence describeRule", () => { + it("describes the simple frequencies", () => { + expect(describeRecurrence(undefined)).toBe("Does not repeat"); + expect(describeRecurrence({ "@type": "RecurrenceRule", frequency: "daily" } as never)).toBe("Daily"); + expect(describeRecurrence({ "@type": "RecurrenceRule", frequency: "daily", interval: 3 } as never)).toBe("Every 3 days"); + }); + + it("recognises Monday to Friday as every weekday", () => { + const rule = { + "@type": "RecurrenceRule", frequency: "weekly", + byDay: ["mo", "tu", "we", "th", "fr"].map((day) => ({ "@type": "NDay", day })), + }; + expect(describeRecurrence(rule as never)).toBe("Every weekday"); + }); + + it("uses a word, not a suffix, for the nth weekday of a month", () => { + const s = describeRecurrence({ + "@type": "RecurrenceRule", frequency: "monthly", + byDay: [{ "@type": "NDay", day: "tu", nthOfPeriod: 2 }], + } as never); + expect(s).toContain("second"); + expect(s).not.toContain("2nd"); + }); + + it("wraps the sentence for count and until rather than appending to it", () => { + const s = describeRecurrence({ "@type": "RecurrenceRule", frequency: "daily", count: 5 } as never); + expect(s).toBe("Daily, 5 times"); + const u = describeRecurrence({ "@type": "RecurrenceRule", frequency: "daily", until: "2026-05-03T00:00:00" } as never); + expect(u).toBe("Daily, until 2026-05-03"); + }); + + it("takes its weekday names from the locale, not a table of English", () => { + setUiLanguageForFormatting("de-DE"); + const names = weekdayOptions().map((w) => w.label); + expect(names[0]).toBe("Montag"); + expect(names).toHaveLength(7); + // The narrow forms collide in English ("T" for both Tuesday and Thursday), + // which is why they cannot be catalogue keys and come from Intl instead. + expect(weekdayOptions().map((w) => w.short)).toHaveLength(7); + setUiLanguageForFormatting(null); + }); + + it("renders a translated rule through the catalogue", () => { + setCatalog("de", { strings: { Daily: "Täglich" }, plurals: {} }); + expect(describeRecurrence({ "@type": "RecurrenceRule", frequency: "daily" } as never)).toBe("Täglich"); + setCatalog("en", { strings: {}, plurals: {} }); + }); +}); diff --git a/web/src/lib/datetime.ts b/web/src/lib/datetime.ts index 7d6e15b..8c1a380 100644 --- a/web/src/lib/datetime.ts +++ b/web/src/lib/datetime.ts @@ -557,3 +557,48 @@ export function localeOptions(): LocaleOption[] { optionsExtras = extras; return list; } + +/** + * Weekday names in the reader's locale, indexed by JSCalendar's two-letter day. + * + * These used to be a table of English strings with a `short` of "M", "T", "W"… + * which could not become catalogue entries at all: "T" is both Tuesday and + * Thursday and "S" is both Saturday and Sunday, so the key collides with + * itself. A catalogue cannot hold two translations under one key, and no + * amount of translating fixes that — the data was wrong, not the wiring. + * + * Intl has the names already, in every locale, in three widths, and gets the + * plural and capitalisation conventions right without anybody maintaining a + * list. 2026-06-01 is a Monday; the rest follow from it. + */ +export type WeekdayKey = "mo" | "tu" | "we" | "th" | "fr" | "sa" | "su"; + +const WEEKDAY_ORDER: WeekdayKey[] = ["mo", "tu", "we", "th", "fr", "sa", "su"]; +const WEEKDAY_BASE = Date.UTC(2026, 5, 1); // a Monday + +export function weekdayName(day: WeekdayKey, width: "long" | "short" | "narrow" = "long"): string { + const i = WEEKDAY_ORDER.indexOf(day); + if (i < 0) return day; + return intl({ weekday: width, timeZone: "UTC" }).format(new Date(WEEKDAY_BASE + i * 86_400_000)); +} + +/** Every weekday, Monday first, for pickers that show all seven. */ +export function weekdayNames(width: "long" | "short" | "narrow" = "long"): Array<{ key: WeekdayKey; name: string }> { + return WEEKDAY_ORDER.map((key) => ({ key, name: weekdayName(key, width) })); +} + +/** + * "A, B and C" — or "A, B oder C", or the comma the locale actually uses. + * + * Joining with a translated " and " does not work: Japanese does not separate + * list items with a word, and the last separator differs from the others in + * English. Intl.ListFormat knows all of that. + */ +export function formatList(items: string[], type: "conjunction" | "disjunction" = "conjunction"): string { + if (items.length < 2) return items[0] ?? ""; + try { + return new Intl.ListFormat(resolvedLocale(), { style: "long", type }).format(items); + } catch { + return items.join(", "); + } +} diff --git a/web/src/lib/recurrence.ts b/web/src/lib/recurrence.ts index 65a7c4b..f3c4de2 100644 --- a/web/src/lib/recurrence.ts +++ b/web/src/lib/recurrence.ts @@ -1,14 +1,25 @@ import type { JSCalendarRecurrenceRule, JSCalendarNDay } from "@/jmap/types"; +import { formatList, weekdayName, weekdayNames } from "./datetime"; +import { plural, t } from "@/lib/i18n"; -export const WEEKDAYS: Array<{ key: JSCalendarNDay["day"]; label: string; short: string }> = [ - { key: "mo", label: "Monday", short: "M" }, - { key: "tu", label: "Tuesday", short: "T" }, - { key: "we", label: "Wednesday", short: "W" }, - { key: "th", label: "Thursday", short: "T" }, - { key: "fr", label: "Friday", short: "F" }, - { key: "sa", label: "Saturday", short: "S" }, - { key: "su", label: "Sunday", short: "S" }, -]; +/** + * The seven days, Monday first, named in the reader's locale. + * + * This was a table of English strings carrying `label: "Monday"` and + * `short: "M"`, rendered straight into the picker. The long names could have + * become catalogue entries; the short ones could not, because "T" is both + * Tuesday and Thursday and "S" is both Saturday and Sunday, and a catalogue + * cannot hold two translations under one key. Intl knows all of them. + */ +export const WEEKDAY_KEYS: Array = ["mo", "tu", "we", "th", "fr", "sa", "su"]; + +export function weekdayOptions(): Array<{ key: JSCalendarNDay["day"]; label: string; short: string }> { + return weekdayNames("long").map(({ key, name }) => ({ + key: key as JSCalendarNDay["day"], + label: name, + short: weekdayName(key, "narrow"), + })); +} export type RecurrencePreset = "none" | "daily" | "weekly" | "weekdays" | "monthly" | "yearly" | "custom"; @@ -28,7 +39,7 @@ export function presetFor(rule: JSCalendarRecurrenceRule | undefined): Recurrenc } export function ruleFromPreset(preset: RecurrencePreset, start: Date): JSCalendarRecurrenceRule | undefined { - const dow = WEEKDAYS[(start.getDay() + 6) % 7]!.key; + const dow = WEEKDAY_KEYS[(start.getDay() + 6) % 7]!; switch (preset) { case "daily": return { "@type": "RecurrenceRule", frequency: "daily" }; @@ -45,48 +56,107 @@ export function ruleFromPreset(preset: RecurrencePreset, start: Date): JSCalenda } } +/** + * A recurrence rule as a sentence. + * + * Built as whole sentences with placeholders rather than by concatenation. + * The old version appended fragments -- `base += " on " + names` -- which is + * untranslatable however complete the catalogue is: German puts the weekday + * list somewhere else in the clause, and a translator handed " on " alone + * cannot move it. Every branch below is one key a translator can rewrite in + * full, including the word order. + */ export function describeRule(rule: JSCalendarRecurrenceRule | undefined): string { - if (!rule) return "Does not repeat"; + if (!rule) return t("Does not repeat"); const n = rule.interval ?? 1; + const every = n !== 1; let base: string; + switch (rule.frequency) { case "daily": - base = n === 1 ? "Daily" : `Every ${n} days`; + base = every ? plural(n, { one: "Every {n} day", other: "Every {n} days" }) : t("Daily"); break; + case "weekly": { - base = n === 1 ? "Weekly" : `Every ${n} weeks`; - if (rule.byDay?.length) { - const names = rule.byDay.map((d) => WEEKDAYS.find((w) => w.key === d.day)?.label ?? d.day); - const set = rule.byDay.map((d) => d.day).sort().join(","); - if (set === ["mo", "tu", "we", "th", "fr"].sort().join(",") && n === 1) base = "Every weekday"; - else base += ` on ${names.join(", ")}`; + const days = rule.byDay?.length ? rule.byDay.map((d) => d.day) : []; + const weekdaysOnly = + days.length === 5 && ["mo", "tu", "we", "th", "fr"].every((d) => days.includes(d as JSCalendarNDay["day"])); + if (weekdaysOnly && !every) { + base = t("Every weekday"); + } else if (days.length) { + const list = formatList(days.map((d) => weekdayName(d as never))); + base = every + ? plural(n, { one: "Every {n} week on {days}", other: "Every {n} weeks on {days}" }, { days: list }) + : t("Weekly on {days}", { days: list }); + } else { + base = every ? plural(n, { one: "Every {n} week", other: "Every {n} weeks" }) : t("Weekly"); } break; } + case "monthly": { - base = n === 1 ? "Monthly" : `Every ${n} months`; - if (rule.byMonthDay?.length) base += ` on day ${rule.byMonthDay.join(", ")}`; - else if (rule.byDay?.length) { + if (rule.byMonthDay?.length) { + const list = formatList(rule.byMonthDay.map(String)); + base = every + ? plural(n, { one: "Every {n} month on day {days}", other: "Every {n} months on day {days}" }, { days: list }) + : t("Monthly on day {days}", { days: list }); + } else if (rule.byDay?.length) { const d = rule.byDay[0]!; - const ord = d.nthOfPeriod ? ordinal(d.nthOfPeriod) + " " : ""; - base += ` on the ${ord}${WEEKDAYS.find((w) => w.key === d.day)?.label ?? d.day}`; + const weekday = weekdayName(d.day as never); + if (d.nthOfPeriod) { + const ord = ordinal(d.nthOfPeriod); + base = every + ? plural(n, { one: "Every {n} month on the {ordinal} {weekday}", other: "Every {n} months on the {ordinal} {weekday}" }, { ordinal: ord, weekday }) + : t("Monthly on the {ordinal} {weekday}", { ordinal: ord, weekday }); + } else { + base = every + ? plural(n, { one: "Every {n} month on {weekday}", other: "Every {n} months on {weekday}" }, { weekday }) + : t("Monthly on {weekday}", { weekday }); + } + } else { + base = every ? plural(n, { one: "Every {n} month", other: "Every {n} months" }) : t("Monthly"); } break; } + case "yearly": - base = n === 1 ? "Yearly" : `Every ${n} years`; + base = every ? plural(n, { one: "Every {n} year", other: "Every {n} years" }) : t("Yearly"); break; + default: - base = `Every ${n} ${rule.frequency}`; + // An RFC frequency this build has no sentence for. The frequency word + // itself stays as the server sent it rather than being invented. + base = t("Every {n} {frequency}", { n, frequency: rule.frequency }); + } + + // The tail wraps the sentence rather than being glued to its end, so a + // translator can put "until 3 May" first if that is what the language does. + if (rule.count) { + base = plural(rule.count, { one: "{rule}, {n} time", other: "{rule}, {n} times" }, { rule: base }); + } + if (rule.until) { + base = t("{rule}, until {date}", { rule: base, date: rule.until.slice(0, 10) }); } - if (rule.count) base += `, ${rule.count} times`; - if (rule.until) base += `, until ${rule.until.slice(0, 10)}`; return base; } +/** + * "first", "second", "last" -- words, not "1st". + * + * The suffix table this replaced ("st", "nd", "rd", "th") is English spelling + * rules in code: German writes "1.", Japanese "第1", and no catalogue can + * reach a suffix chosen by arithmetic. JSCalendar's nthOfPeriod is 1-5 or -1 + * in practice, so five words and "last" cover it; anything else falls back to + * the bare number, which is wrong in no language. + */ function ordinal(n: number): string { - if (n === -1) return "last"; - const s = ["th", "st", "nd", "rd"]; - const v = n % 100; - return n + (s[(v - 20) % 10] ?? s[v] ?? s[0]!); + switch (n) { + case -1: return t("last"); + case 1: return t("first"); + case 2: return t("second"); + case 3: return t("third"); + case 4: return t("fourth"); + case 5: return t("fifth"); + default: return String(n); + } } diff --git a/web/src/lib/sieve.ts b/web/src/lib/sieve.ts index f70176c..c39085a 100644 --- a/web/src/lib/sieve.ts +++ b/web/src/lib/sieve.ts @@ -6,6 +6,9 @@ * Sieve below each comment is what the server actually runs. */ +import { formatList } from "./datetime"; +import { t } from "@/lib/i18n"; + export type HeaderOp = "contains" | "notcontains" | "is" | "notis" | "matches" | "notmatches" | "regex" | "notregex" | "exists" | "notexists"; export type SieveTest = @@ -318,49 +321,60 @@ export function reorderRules(rules: SieveRule[], fromId: string, toId: string, b return [...rest.slice(0, at), moved, ...rest.slice(at)]; } +/** + * A filter rule as a sentence, for the rule list. + * + * Rebuilt as whole sentences with placeholders. The old version concatenated + * fragments -- a header name, an operator, a quoted value, joined by " and " + * -- which no catalogue could fix: German puts the verb last, Japanese does + * not separate list items with a word at all, and a translator handed " and " + * on its own cannot move anything. Reported by a native speaker reviewing the + * German catalogue (#247). + * + * Intl.ListFormat does the joining, so "A, B and C" becomes "A, B und C" and, + * for an anyof rule, the disjunction the language actually uses. + */ export function describeRule(r: SieveRule): string { - const tests = r.tests - .map((t) => { - switch (t.type) { - case "header": - return `${t.header} ${HEADER_OPS.find((o) => o.value === t.op)?.label ?? t.op} "${t.value}"`; - case "address": - return `${t.header} address ${HEADER_OPS.find((o) => o.value === t.op)?.label ?? t.op} "${t.value}"`; - case "size": - return `size ${t.op} ${Math.round(t.value / 1024)} KB`; - case "body": - return `body ${t.op === "contains" ? "contains" : "does not contain"} "${t.value}"`; - case "true": - return "always"; - } - }) - .join(r.join === "allof" ? " and " : " or "); - const actions = r.actions - .map((a) => { - switch (a.type) { - case "fileinto": - return `move to ${a.mailbox}`; - case "redirect": - return `forward to ${a.address}`; - case "discard": - return "delete"; - case "keep": - return "keep"; - case "reject": - return "reject"; - case "markread": - return "mark read"; - case "flag": - return "star"; - case "addflag": - case "setflag": - return `add ${a.flag}`; - case "removeflag": - return `remove ${a.flag}`; - case "stop": - return "stop"; - } - }) - .join(", "); - return `${tests || "always"} → ${actions}`; + const headerLabel = (h: string): string => t(HEADER_CHOICES.find((c) => c.value === h)?.label ?? h); + const opLabel = (op: string): string => t(HEADER_OPS.find((o) => o.value === op)?.label ?? op); + + const tests = r.tests.map((test) => { + switch (test.type) { + case "header": + return t('{header} {op} "{value}"', { header: headerLabel(test.header), op: opLabel(test.op), value: test.value }); + case "address": + return t('{header} address {op} "{value}"', { header: headerLabel(test.header), op: opLabel(test.op), value: test.value }); + case "size": + return test.op === "over" + ? t("size is over {n} KB", { n: Math.round(test.value / 1024) }) + : t("size is under {n} KB", { n: Math.round(test.value / 1024) }); + case "body": + return test.op === "contains" + ? t('body contains "{value}"', { value: test.value }) + : t('body does not contain "{value}"', { value: test.value }); + case "true": + return t("always"); + } + }); + + const actions = r.actions.map((a) => { + switch (a.type) { + case "fileinto": return t("move to {folder}", { folder: a.mailbox }); + case "redirect": return t("forward to {address}", { address: a.address }); + case "discard": return t("delete it"); + case "keep": return t("keep it"); + case "reject": return t("reject it"); + case "markread": return t("mark it read"); + case "flag": return t("star it"); + case "addflag": + case "setflag": return t("add {flag}", { flag: a.flag }); + case "removeflag": return t("remove {flag}", { flag: a.flag }); + case "stop": return t("stop"); + } + }); + + return t("{tests} → {actions}", { + tests: tests.length ? formatList(tests, r.join === "allof" ? "conjunction" : "disjunction") : t("always"), + actions: formatList(actions, "conjunction"), + }); } diff --git a/web/src/views/calendar/EventEditor.tsx b/web/src/views/calendar/EventEditor.tsx index 8f5ee96..aaf0a7d 100644 --- a/web/src/views/calendar/EventEditor.tsx +++ b/web/src/views/calendar/EventEditor.tsx @@ -13,7 +13,7 @@ import { RecipientInput } from "../compose/RecipientInput"; import { DateField, DateTimeField } from "@/ui/datefield"; import { browserTimeZone, dateToZonedLocal, formatDuration, fromInputDateTime, listTimeZones, parseDuration, toInputDateTime, toLocalDateOnly, zonedToDate, DAY_MS, humanDuration } from "@/lib/dates"; import { formatClock, formatNumericDate, formatWeekday, formatWeekdayDate } from "@/lib/datetime"; -import { WEEKDAYS, describeRule, presetFor, ruleFromPreset, type RecurrencePreset } from "@/lib/recurrence"; +import { WEEKDAY_KEYS, weekdayOptions, describeRule, presetFor, ruleFromPreset, type RecurrencePreset } from "@/lib/recurrence"; import { newKey } from "@/lib/contacts"; import { availabilityWindow } from "@/lib/availabilityWindow"; import { askEditScope, droppedMessage, runScoped } from "./scope"; @@ -359,7 +359,7 @@ function EventForm({ init, base, scope, editing, onClose, settingsTz, defaultAle )} {!oneDate && ( - { const p = e.target.value as RecurrencePreset; setPreset(p); if (p === "custom") setRule(rule ?? { "@type": "RecurrenceRule", frequency: "weekly", byDay: [{ "@type": "NDay", day: WEEKDAY_KEYS[(start.getDay() + 6) % 7]! }] }); else setRule(ruleFromPreset(p, start)); }}> @@ -381,7 +381,7 @@ function EventForm({ init, base, scope, editing, onClose, settingsTz, defaultAle {customRule.frequency === "weekly" && (
- {WEEKDAYS.map((w) => { + {weekdayOptions().map((w) => { const on = customRule.byDay?.some((d) => d.day === w.key); return ; })}