diff --git a/BACKLOG.md b/BACKLOG.md index 77fada6..8140ea1 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -222,6 +222,8 @@ Two items jump the queue by owner decision. Full reasoning in `decisions.md`; th - [ ] **N39 The spectrum the colour picker does not have (filed 2026-09-06; the font half closed 2026-09-07, the alpha half the same day).** The font button is in, over a font enumeration that reads the `name` table out of every installed font file, and over a planner that writes several tags as one step, which is what this entry said both halves wanted. The transparency went in on that same planner: the picker takes a number from 0 to 255 and writes `\1a` to `\4a` beside the colour, as one undo step, and an empty field writes none at all, which is this product's answer to the reference writing it only when it changed. What is left is the spectrum, where the picker offers sixteen colours and a field that takes `#RRGGBB` and the reference offers a saturation square, a hue slider, the three number notations and a screen eyedropper. What it said when it was filed: Row three names nine buttons before Next line: four style flags, a font, and four colours. The flags and the colours are built; the font is not, and it is the one that needs something this repository does not have, a list of the fonts installed on the machine. Beside it two halves of the colour picker are missing. The alpha: a colour is written with its transparency beside it, `\1a` to `\4a`, and only when that transparency changed, which is two tags in one undo step and `Edit::SetOverrideTag` writes one. The spectrum: the picker offers sixteen colours and a field that takes `#RRGGBB`, where a saturation square, a hue slider, the three number notations and a screen eyedropper belong. The alpha and the font want the same thing first, a planner that writes a list of tags as one step. +- [ ] **N42 The style editor holds ten of a style's twenty-three columns, because the parser reads ten (filed 2026-09-07).** Edit beside the Style dropdown opens on the font, the size, the four colours and the four flags, and writes each as its own undo step over the span the parser recorded. What it does not hold is everything the parser never read: the outline and shadow widths, the border style, the three margins, the alignment, the rotation and scaling, the spacing and the encoding. They are not drawn as empty boxes, because a box that cannot be written is worse than an absence; adding them is parser work in `ass.rs` and `AssStyle` before it is dialog work, and the dialog then grows by a line each. The style's **name** is a separate matter and not this entry's: renaming one means rewriting every event that names it, which is a different operation with its own undo step. + - [ ] **N40 (the same defect as N36, filed twice by mistake on 2026-09-06; keep this entry, which carries what was learned) One check in `video-aspect.spec.js` fails on the CI runner about half the time, and its own evidence does not name the cause (filed 2026-09-06).** "says a media with no picture has none, and is as quiet about it as about no audio" opens a media with no video track and asserts that nothing alarming is on the status bar. On the CI runner it twice found "Open a video first." there: on the merge run of #99 at 16:29 and on #101 at 18:39, both on 2026-09-06, and it has never failed on this repository's own runner. What the check already collects rules out two of the three ways that sentence can appear: the app logged no `was refused as` line, so no command was refused through `refused()`, and the run counted zero `video://error` events. That leaves a rejected `invoke` inside `useVideoPlayer`, whose five commands each set `errorCode` from a rejection, and `VideoErrorCode::NotLoaded` is what `from_mpv` answers for `mpv_error::PropertyUnavailable`, which is what a video property is on a media that carries no video. Which of the five it is has not been proven and must not be guessed: the check has to name the node and the code it found before anything is changed, because a fix aimed at the wrong one of them would look like it worked. **A flaky check is a check that says nothing**, so this is a defect in the suite as much as in the app, and the first change is to the check. - **2026-09-06, found by reading and fixed, not proven to be the cause.** Every one of those five setters wrote its answer unconditionally, so a command sent against the file that was open could still be in flight when the next file opened and then set the error state about a document nobody had asked about. `useVideoPlayer` now stamps each command with the open it belongs to and drops an answer carrying an older one. It is a real race and the fix is right on its own terms; whether it is what this check keeps catching is only knowable from CI, because it has never reproduced here. Two attempts to reproduce it by taking the audio output away, which is the one difference this machine and the runner are known to have, did not: mpv found an output both times and the spec passed. - **2026-09-07, and this changes what the entry is about.** `video-aspect.spec.js`'s "fires about as often as the shape changes" failed once in a full run **on this machine**, with extra picture payloads, and passed on its own straight afterwards. Until then every one of these had been runner-only, which is why the entry above reads as a property of the runner. It is not: it is a property of the checks that count events, wherever they run, and a slower or busier machine only makes it likelier. The retry on CI still earns its place, and it is now covering a defect that lives here too rather than one that lives there. diff --git a/crates/sublore-edit/src/plan.rs b/crates/sublore-edit/src/plan.rs index bd86121..895e5aa 100644 --- a/crates/sublore-edit/src/plan.rs +++ b/crates/sublore-edit/src/plan.rs @@ -43,6 +43,14 @@ pub enum Edit { cue: usize, keep_tags: bool, }, + /// One field of one declared style. The name is not among them: renaming a style means + /// rewriting every event that names it, which is a different operation and a different undo + /// step. See edit-bar-tasks.md and the style editor's own slice. + SetStyleField { + style: usize, + field: AssStyleField, + value: String, + }, /// Several override tags written at one caret, as one undo step. A font picker names two, the /// family and the size, and a colour with its transparency names two more: neither is two /// things a translator did. Pairs are `(tag, value)` in the order they are written. @@ -163,6 +171,11 @@ pub fn plan(document: &SubtitleDocument, edit: &Edit) -> Result plan_toggle_style(document, *cue, *flag, *from, *to), Edit::ClearText { cue, keep_tags } => plan_clear_text(document, *cue, *keep_tags), + Edit::SetStyleField { + style, + field, + value, + } => plan_set_style_field(document, *style, *field, value), Edit::SetOverrideTags { cue, tags, at } => { plan_set_override_tags(document, *cue, tags, *at) } @@ -1152,6 +1165,165 @@ fn check_tag(tag: &str, value: &str) -> Result<(), EditError> { Ok(()) } +/// Which column of a `Style:` line a write names. Closed on purpose, and the name is not on it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum AssStyleField { + Fontname, + Fontsize, + Primary, + Secondary, + Outline, + Back, + Bold, + Italic, + Underline, + Strikeout, +} + +impl AssStyleField { + pub fn as_str(self) -> &'static str { + match self { + AssStyleField::Fontname => "fontname", + AssStyleField::Fontsize => "fontsize", + AssStyleField::Primary => "primary colour", + AssStyleField::Secondary => "secondary colour", + AssStyleField::Outline => "outline colour", + AssStyleField::Back => "shadow colour", + AssStyleField::Bold => "bold", + AssStyleField::Italic => "italic", + AssStyleField::Underline => "underline", + AssStyleField::Strikeout => "strikeout", + } + } + + /// Where the field sits in the style the document read, or an empty span where the section's + /// own `Format:` line declares no such column. + fn span(self, style: &sublore_formats::AssStyle) -> Span { + match self { + AssStyleField::Fontname => style.fontname, + AssStyleField::Fontsize => style.fontsize, + AssStyleField::Primary => style.primary, + AssStyleField::Secondary => style.secondary, + AssStyleField::Outline => style.outline, + AssStyleField::Back => style.back, + AssStyleField::Bold => style.bold_field, + AssStyleField::Italic => style.italic_field, + AssStyleField::Underline => style.underline_field, + AssStyleField::Strikeout => style.strikeout_field, + } + } +} + +/// Write one field of one declared style. +/// +/// The same shape a cue's field write has, over a different line: the span the parser recorded is +/// replaced and nothing else moves. A field the section's `Format:` line does not declare is +/// refused rather than added, for the reason a cue's is: declaring one means rewriting every line +/// under that header, including the ones nobody edited. +fn plan_set_style_field( + document: &SubtitleDocument, + index: usize, + field: AssStyleField, + value: &str, +) -> Result { + let Some(style) = document.ass_styles().get(index) else { + return Err(EditError::new( + EditErrorKind::NotApplicable, + format!("no style {index} in this document"), + )); + }; + let span = field.span(style); + if span.start == span.end && span.start == 0 { + return Err(EditError::new( + EditErrorKind::NotApplicable, + format!( + "the styles section's Format line declares no {}", + field.as_str() + ), + )); + } + validate_style_value(field, value)?; + + let Some(segment_index) = document + .segments() + .iter() + .position(|segment| segment.span.start <= span.start && span.end <= segment.span.end) + else { + return Err(EditError::new( + EditErrorKind::NotApplicable, + "the style line is not inside any segment of this document", + )); + }; + + let core = field_core(document, span); + Ok(Planned { + splice: Splice::new( + core.start, + document.slice(core).to_owned(), + value.to_owned(), + ), + label: EditLabel { + kind: EditKind::SetStyleField(field), + // A style is not a cue, and the history keys a run on the pair: a style write and a + // cue write must never coalesce, so this names a row no cue can have. + cue: usize::MAX, + }, + expect: Expectation { + // No cue changes, which is the whole of what this asserts: every one of them is read + // back and compared, because a style line that swallowed a comma would move them all. + from: 0, + removed: 0, + cues: Vec::new(), + segments_from: segment_index, + segments_removed: 1, + segments_inserted: 1, + }, + }) +} + +/// What a style's field may hold. The comma is the dangerous one, for the reason it is in an event. +fn validate_style_value(field: AssStyleField, value: &str) -> Result<(), EditError> { + let unwritable = |detail: &str| EditError::new(EditErrorKind::UnwritableText, detail); + if value.contains(',') { + return Err(unwritable( + "a comma separates the fields of a style line, so a value may not hold one", + )); + } + if value.contains(['\n', '\r']) { + return Err(unwritable( + "a style line is one line, so a value may not break it", + )); + } + if value + .chars() + .any(|character| matches!(character, '\u{0}'..='\u{1f}' | '\u{7f}')) + { + return Err(unwritable( + "a control character cannot be written into a style field", + )); + } + if value.trim() != value { + return Err(unwritable( + "a style field's padding belongs to the file, so a value may not carry its own", + )); + } + // The four flags are what a renderer reads as on or off, and nothing else belongs there. + if matches!( + field, + AssStyleField::Bold + | AssStyleField::Italic + | AssStyleField::Underline + | AssStyleField::Strikeout + ) && value != "0" + && value != "-1" + { + return Err(unwritable( + "a style's flag is written -1 for on and 0 for off", + )); + } + Ok(()) +} + /// Write several override tags at one caret, as one step. /// /// Every name and every value is checked before anything is written, so a list with one bad entry diff --git a/crates/sublore-edit/src/splice.rs b/crates/sublore-edit/src/splice.rs index 7c133e0..02889e8 100644 --- a/crates/sublore-edit/src/splice.rs +++ b/crates/sublore-edit/src/splice.rs @@ -72,6 +72,9 @@ pub enum EditKind { /// A tag written with a value the caller chose. One kind rather than one per tag, unlike the /// flag above: the picker commits on the pick, so every pick opens its own step. SetOverrideTag, + /// One field of one declared style. Its own kind and its own row, so a style write and a cue + /// write are never one undo step whatever else they have in common. + SetStyleField(crate::plan::AssStyleField), /// One cue emptied, with or without its braced runs kept. Its own kind so a Clear and the /// typing around it are never one undo step. See edit-bar-tasks.md B13. ClearText, diff --git a/crates/sublore-edit/tests/session.rs b/crates/sublore-edit/tests/session.rs index cc08781..778fd2c 100644 --- a/crates/sublore-edit/tests/session.rs +++ b/crates/sublore-edit/tests/session.rs @@ -12,7 +12,7 @@ use std::time::{Duration, Instant}; use sublore_edit::diff::CuePatch; use sublore_edit::error::EditErrorKind; use sublore_edit::history::Run; -use sublore_edit::plan::Edit; +use sublore_edit::plan::{AssStyleField, Edit}; use sublore_edit::session::EditSession; use sublore_formats::override_tags::StyleFlag; use sublore_formats::{AssField, SubtitleDocument, SubtitleFormat}; @@ -1139,6 +1139,78 @@ fn two_tags_written_where_a_note_stands_stay_in_one_block() { assert_eq!(raw_text(&session, 0), "{\\fnGentium\\fs48}{note}word"); } +#[test] +fn a_style_field_is_written_where_the_parser_found_it_and_no_cue_moves() { + let mut session = session("ass/clean/basic.ass"); + let before = session.to_bytes(); + let texts_before = texts(&session); + + session + .apply( + &Edit::SetStyleField { + style: 0, + field: AssStyleField::Fontname, + value: "Gentium Book".to_owned(), + }, + Run::New, + Instant::now(), + ) + .expect("a declared style takes a font"); + let after = session.to_bytes(); + assert_ne!(after, before, "the style line changed"); + assert!( + String::from_utf8_lossy(&after).contains("Gentium Book"), + "the font is in the file" + ); + // Not one cue moved: that is what the plan asserts and it is what a style write must never do. + assert_eq!(texts(&session), texts_before); + + session.undo().expect("a step to undo").expect("a patch"); + assert_eq!(session.to_bytes(), before, "undo restores the bytes"); +} + +#[test] +fn a_style_field_refuses_a_comma_a_break_and_a_flag_that_is_neither_on_nor_off() { + let mut session = session("ass/clean/basic.ass"); + let before = session.to_bytes(); + for (field, value) in [ + (AssStyleField::Fontname, "Gentium, Book"), + (AssStyleField::Fontname, "Gentium\nBook"), + (AssStyleField::Fontname, " Gentium"), + (AssStyleField::Bold, "1"), + (AssStyleField::Bold, "yes"), + ] { + session + .apply( + &Edit::SetStyleField { + style: 0, + field, + value: value.to_owned(), + }, + Run::New, + Instant::now(), + ) + .expect_err("the value cannot be written into a style line"); + } + assert_eq!(session.to_bytes(), before, "a refusal writes nothing"); +} + +#[test] +fn a_style_the_document_does_not_declare_is_refused() { + let mut session = session("ass/clean/basic.ass"); + session + .apply( + &Edit::SetStyleField { + style: 99, + field: AssStyleField::Fontname, + value: "Gentium".to_owned(), + }, + Run::New, + Instant::now(), + ) + .expect_err("there is no style 99"); +} + #[test] fn a_list_with_one_bad_tag_in_it_writes_none_of_them() { let mut session = session("ass/clean/basic.ass"); diff --git a/crates/sublore-formats/src/ass.rs b/crates/sublore-formats/src/ass.rs index 202acdc..f68243d 100644 --- a/crates/sublore-formats/src/ass.rs +++ b/crates/sublore-formats/src/ass.rs @@ -226,6 +226,10 @@ fn style_record( italic: style_flag(body, style_field(body, remainder, format.italic)), underline: style_flag(body, style_field(body, remainder, format.underline)), strikeout: style_flag(body, style_field(body, remainder, format.strikeout)), + bold_field: style_field(body, remainder, format.bold), + italic_field: style_field(body, remainder, format.italic), + underline_field: style_field(body, remainder, format.underline), + strikeout_field: style_field(body, remainder, format.strikeout), }) } diff --git a/crates/sublore-formats/src/document.rs b/crates/sublore-formats/src/document.rs index ccfc0cf..3f66514 100644 --- a/crates/sublore-formats/src/document.rs +++ b/crates/sublore-formats/src/document.rs @@ -119,6 +119,12 @@ pub struct AssStyle { pub italic: bool, pub underline: bool, pub strikeout: bool, + /// Where each of those four sits, so an editor can write one back. Read apart from the + /// booleans above because a reader wants the meaning and a writer wants the bytes. + pub bold_field: Span, + pub italic_field: Span, + pub underline_field: Span, + pub strikeout_field: Span, } /// A parsed file: the bytes it came from, and the ordered segments that tile them. diff --git a/e2e/specs/current-line-bands.spec.js b/e2e/specs/current-line-bands.spec.js index 1e37ac1..d0fbecb 100644 --- a/e2e/specs/current-line-bands.spec.js +++ b/e2e/specs/current-line-bands.spec.js @@ -78,7 +78,11 @@ const CONTROLS = [ * the block to its pixels across a size change. So the panel's contents scale and its box does not, * and at 150 per cent that is one control's worth. See BACKLOG N38. */ -const BARE_SHORTFALL = { 90: [], 110: [], 150: [".currentline__text"] }; +const BARE_SHORTFALL = { + 90: [], + 110: [], + 150: [".currentline__text", ".currentline__colour-primary", ".currentline__subtitle-next-line"], +}; /** * The most the panel may fail to show at once, pinned so it cannot grow in silence. A ceiling and @@ -98,9 +102,23 @@ const BARE_SHORTFALL = { 90: [], 110: [], 150: [".currentline__text"] }; * The colour beside them at 110 per cent is that same row and not a new shortfall: the entries on * either side of it are the first and the last control of the button row, so the row was already * behind the scroll there before the colours were drawn into it. B12. + * + * **2026-09-07, and this is a real growth rather than a reading of the same one.** Edit beside the + * Style dropdown made the first band wide enough to wrap at the narrow window, which pushed the + * button row down by a line at 90 per cent with a waveform and at 150 per cent without one. It is + * paid here rather than fixed for the reason the paragraph above gives: what fixes it is N38, and + * N38 needs the owner. Every control is still drawn and every one is still reachable by scrolling + * the panel, which is what these two checks actually guard. */ const SHORTFALL = { - 90: { floor: [".currentline__text"], wide: [] }, + 90: { + floor: [ + ".currentline__text", + ".currentline__colour-primary", + ".currentline__subtitle-next-line", + ], + wide: [], + }, 110: { floor: [ ".currentline__text", @@ -112,6 +130,7 @@ const SHORTFALL = { }, 150: { floor: [ + ".currentline__start", ".currentline__end", ".currentline__text", ".currentline__edit-style-bold", diff --git a/e2e/specs/style-editor.spec.js b/e2e/specs/style-editor.spec.js new file mode 100644 index 0000000..58da83b --- /dev/null +++ b/e2e/specs/style-editor.spec.js @@ -0,0 +1,221 @@ +/* global describe, it, before, document, window */ +/** + * B10: the style editor, which Edit beside the Style dropdown opens. + * + * The claim worth checking is not that a field takes a value: it is that writing a style moves the + * style line and nothing else. Every event line in the saved file is compared byte for byte. + */ +import { copyFileSync, existsSync, mkdirSync, readFileSync, rmSync } from "node:fs"; +import path from "node:path"; +import process from "node:process"; + +import { browser, expect } from "@wdio/globals"; + +import { answerChooser, waitForChooser } from "../lib/chooser.js"; +import { clickAt, focusWindow, pressKey, typeText } from "../lib/input.js"; +import { repoRoot, windowHeight, windowWidth } from "../lib/paths.js"; +import { waitFor } from "../lib/proc.js"; +import { findToplevel } from "../lib/x11.js"; + +/** Three cues and one declared style, which the first row names. */ +const FIXTURE = ["ass", "clean", "basic.ass"]; +/** Typed into the editor's font field. It has a space in it, which a style line may hold. */ +const FONT = "Gentium Book"; + +function dataHome() { + const home = process.env.SUBLORE_E2E_DATA_HOME; + if (typeof home !== "string" || home === "") { + throw new Error("SUBLORE_E2E_DATA_HOME is not set; e2e/wdio.conf.js sets it for every run."); + } + return home; +} + +/** Writes go to the harness temp dir. The committed fixture is copied, never opened directly. */ +function workingCopy() { + const from = path.join(repoRoot, "fixtures", "subtitles", ...FIXTURE); + if (!existsSync(from)) { + throw new Error( + `E2E prerequisite missing: ${from} does not exist. It is committed; restore it with ` + + "`git checkout fixtures/subtitles`.", + ); + } + const directory = path.join(dataHome(), "style-editor"); + rmSync(directory, { recursive: true, force: true }); + mkdirSync(directory, { recursive: true }); + const copy = path.join(directory, "basic.ass"); + copyFileSync(from, copy); + return copy; +} + +function centreOf(selector) { + return browser.execute((css) => { + const element = document.querySelector(css); + if (element === null) { + return null; + } + const rect = element.getBoundingClientRect(); + const dpr = window.devicePixelRatio; + return { x: (rect.x + rect.width / 2) * dpr, y: (rect.y + rect.height / 2) * dpr }; + }, selector); +} + +async function clickElement(toplevel, selector) { + const centre = await centreOf(selector); + if (centre === null) { + throw new Error(`${selector} is missing from the DOM, so there is nothing to click`); + } + // No window manager under Xvfb, so the toplevel origin is also the viewport origin. + clickAt(toplevel.absX + centre.x, toplevel.absY + centre.y); +} + +function present(selector) { + return browser.execute((css) => document.querySelector(css) !== null, selector); +} + +function textOf(selector) { + return browser.execute((css) => document.querySelector(css)?.textContent ?? null, selector); +} + +function valueOf(selector) { + return browser.execute((css) => document.querySelector(css)?.value ?? null, selector); +} + +/** Every `Dialogue:` line of a file, which a style write may never touch. */ +function events(file) { + return readFileSync(file, "utf8") + .split(/\r?\n/) + .filter((line) => line.startsWith("Dialogue:") || line.startsWith("Comment:")); +} + +/** The one `Style:` line of the fixture. */ +function styleLine(file) { + return ( + readFileSync(file, "utf8") + .split(/\r?\n/) + .find((line) => line.startsWith("Style:")) ?? null + ); +} + +describe("the style editor", () => { + let toplevel = null; + let copy = null; + let eventsBefore = null; + + before(async () => { + copy = workingCopy(); + eventsBefore = events(copy); + toplevel = await waitFor(findToplevel, { + timeout: 30000, + message: `the ${windowWidth}x${windowHeight} "Sublore" toplevel to appear`, + }); + focusWindow(toplevel.id); + await waitFor(() => present(".toolbar__file-open-subtitle"), { + timeout: 30000, + message: "the app UI to render", + }); + }); + + it("opens on the style the line names, with the values the file holds", async () => { + await clickElement(toplevel, ".toolbar__file-open-subtitle"); + const chooser = await waitForChooser("Choose a subtitle"); + await answerChooser(chooser, copy, "subtitle"); + focusWindow(toplevel.id); + await waitFor( + async () => ((await textOf(".statusbar__document"))?.includes("3 cues") === true ? 1 : null), + { timeout: 20000, message: "the fixture to open" }, + ); + + await clickElement(toplevel, ".currentline__style-edit"); + await waitFor(() => present(".styleeditor__panel"), { + timeout: 15000, + message: "the style editor to open", + }); + + // The title is the style's own name, and the fields are what the file spells, not a guess. + expect(await textOf(".styleeditor__title")).toBe("Default"); + const line = styleLine(copy); + expect(line).not.toBe(null); + const columns = line.replace(/^Style:\s*/, "").split(","); + expect(await valueOf(".styleeditor__fontname")).toBe(columns[1].trim()); + expect(await valueOf(".styleeditor__fontsize")).toBe(columns[2].trim()); + expect(await valueOf(".styleeditor__primary")).toBe(columns[3].trim()); + }); + + it("writes the font into the style line and moves no event at all", async () => { + await clickElement(toplevel, ".styleeditor__fontname"); + pressKey("ctrl+a"); + typeText(FONT); + await waitFor(async () => ((await valueOf(".styleeditor__fontname")) === FONT ? 1 : null), { + timeout: 15000, + message: `the font field to hold exactly ${FONT}`, + }); + pressKey("Return"); + + // Nothing is on disk yet: the write is an edit like any other and waits for a save. + await waitFor(() => present(".statusbar__dirty"), { + timeout: 15000, + message: "the document to be marked unsaved", + }); + await clickElement(toplevel, ".styleeditor__close"); + await waitFor(async () => ((await present(".styleeditor__panel")) ? null : 1), { + timeout: 15000, + message: "the editor to close", + }); + + await clickElement(toplevel, ".toolbar__file-save"); + await waitFor( + async () => ((await textOf(".statusbar__message"))?.includes("aved") === true ? 1 : null), + { timeout: 20000, message: "the document to be saved" }, + ); + + expect(styleLine(copy)).toContain(FONT); + // The whole of what this check is for: a style line that swallowed a comma would move every + // column of every event under it, and this is the only place that would be seen. + expect(events(copy)).toEqual(eventsBefore); + }); + + it("turns a flag on in the style line, and one undo takes it back", async () => { + await clickElement(toplevel, ".currentline__style-edit"); + await waitFor(() => present(".styleeditor__panel"), { + timeout: 15000, + message: "the style editor to open again", + }); + const flag = () => + browser.execute(() => document.querySelector(".styleeditor__bold")?.checked ?? null); + expect(await flag()).toBe(false); + + await clickElement(toplevel, ".styleeditor__bold"); + await waitFor(async () => ((await flag()) === true ? 1 : null), { + timeout: 15000, + message: "the style's bold flag to come on", + }); + await clickElement(toplevel, ".styleeditor__close"); + await waitFor(async () => ((await present(".styleeditor__panel")) ? null : 1), { + timeout: 15000, + message: "the editor to close", + }); + + // ASS writes -1 for on, and the font written a moment ago is still there beside it. + await clickElement(toplevel, ".toolbar__file-save"); + await waitFor(async () => (styleLine(copy)?.includes("-1") === true ? 1 : null), { + timeout: 20000, + message: "the flag to reach the file", + }); + expect(styleLine(copy)).toContain(FONT); + expect(events(copy)).toEqual(eventsBefore); + + await clickElement(toplevel, ".toolbar__edit-undo"); + await waitFor(async () => ((await present(".statusbar__dirty")) ? 1 : null), { + timeout: 15000, + message: "the undo to leave the document unsaved again", + }); + await clickElement(toplevel, ".currentline__style-edit"); + await waitFor(() => present(".styleeditor__panel"), { + timeout: 15000, + message: "the style editor to open once more", + }); + expect(await flag()).toBe(false); + expect(await valueOf(".styleeditor__fontname")).toBe(FONT); + await clickElement(toplevel, ".styleeditor__close"); + }); +}); diff --git a/e2e/wdio.conf.js b/e2e/wdio.conf.js index 59d8ca7..f962425 100644 --- a/e2e/wdio.conf.js +++ b/e2e/wdio.conf.js @@ -13,7 +13,7 @@ import { passedTests, recordPassedTest, resetTally } from "./lib/tally.js"; * Every spec that exists must run. WebdriverIO does not reliably fail a run that executed nothing, * so the count is asserted here. Bump it when you add a test; see e2e/README.md. */ -const EXPECTED_TESTS = 292; +const EXPECTED_TESTS = 295; // Keeps a run out of the real data dir. Created once in the launcher; workers inherit the value. const inherited = process.env.SUBLORE_E2E_DATA_HOME; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index c545b56..1fb7846 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -215,6 +215,7 @@ pub fn run() -> tauri::Result<()> { subtitle::subtitle_toggle_style, subtitle::subtitle_set_override_tags, subtitle::subtitle_clear_text, + subtitle::subtitle_set_style_field, fonts::fonts_installed, subtitle::subtitle_open_source, subtitle::subtitle_close_source, diff --git a/src-tauri/src/subtitle/mod.rs b/src-tauri/src/subtitle/mod.rs index 27b8eed..ec926c9 100644 --- a/src-tauri/src/subtitle/mod.rs +++ b/src-tauri/src/subtitle/mod.rs @@ -14,7 +14,7 @@ use std::time::Instant; use serde::{Deserialize, Serialize}; use sublore_edit::diff::{CuePatch, CueView}; use sublore_edit::history::Run; -use sublore_edit::plan::{self, Edit}; +use sublore_edit::plan::{self, AssStyleField, Edit}; use sublore_edit::session::EditSession; use sublore_formats::override_tags::StyleFlag; use sublore_formats::{parse, AssField, Newline, SubtitleDocument, SubtitleFormat}; @@ -179,6 +179,9 @@ pub struct CuePatchDto { pub can_redo: bool, pub dirty: bool, pub truncated: bool, + /// The styles as they stand. On every patch because a style write changes no cue, so nothing + /// else in this shape would tell the interface that one moved. + pub styles: Vec, } #[derive(Clone, Debug, Serialize)] @@ -446,6 +449,64 @@ pub async fn subtitle_toggle_style( .await } +/// Which column of a `Style:` line a write names, on the wire. The name is not on it: renaming a +/// style means rewriting every event that names it, which is a different operation. See B10. +#[derive(Clone, Copy, Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum AssStyleFieldDto { + Fontname, + Fontsize, + Primary, + Secondary, + Outline, + Back, + Bold, + Italic, + Underline, + Strikeout, +} + +impl From for AssStyleField { + fn from(field: AssStyleFieldDto) -> Self { + match field { + AssStyleFieldDto::Fontname => AssStyleField::Fontname, + AssStyleFieldDto::Fontsize => AssStyleField::Fontsize, + AssStyleFieldDto::Primary => AssStyleField::Primary, + AssStyleFieldDto::Secondary => AssStyleField::Secondary, + AssStyleFieldDto::Outline => AssStyleField::Outline, + AssStyleFieldDto::Back => AssStyleField::Back, + AssStyleFieldDto::Bold => AssStyleField::Bold, + AssStyleFieldDto::Italic => AssStyleField::Italic, + AssStyleFieldDto::Underline => AssStyleField::Underline, + AssStyleFieldDto::Strikeout => AssStyleField::Strikeout, + } + } +} + +/// One field of one declared style. Answers with a patch like every other edit, and the patch +/// carries the styles, which is how the interface learns that one moved. See B10. +#[tauri::command] +pub async fn subtitle_set_style_field( + app: AppHandle, + state: State<'_, SubtitleState>, + revision: u64, + style: usize, + field: AssStyleFieldDto, + value: String, +) -> Result { + edited( + &app, + state.slot(), + revision, + Edit::SetStyleField { + style, + field: field.into(), + value, + }, + ) + .await +} + /// Empty one cue's text. `keep_tags` is the reference's Clear Text: the braced runs stay where /// they are and only the words go. See edit-bar-tasks.md B13. #[tauri::command] @@ -1170,27 +1231,7 @@ pub fn summarize(path: Option<&str>, document: &SubtitleDocument) -> SubtitleSum has_bom: source.has_bom(), newline: newline_str(source.newline()).to_owned(), byte_length: source.byte_len() as u64, - styles: document - .ass_styles() - .iter() - .map(|style| { - let [name, fontname, fontsize, primary, secondary, outline, back] = - document.ass_style_text(style); - AssStyleDto { - name: name.to_owned(), - fontname: fontname.to_owned(), - fontsize: fontsize.to_owned(), - primary: primary.to_owned(), - secondary: secondary.to_owned(), - outline: outline.to_owned(), - back: back.to_owned(), - bold: style.bold, - italic: style.italic, - underline: style.underline, - strikeout: style.strikeout, - } - }) - .collect(), + styles: ass_styles(document), } } @@ -1322,6 +1363,33 @@ fn opened_payload(session: &EditSession, summary: SubtitleSummary) -> SubtitleOp } } +/// The declared styles as an editor reads them. Carried on every patch as well as on the +/// summary, because a style write changes no cue and the interface has to be told some other +/// way that one moved. +fn ass_styles(document: &SubtitleDocument) -> Vec { + document + .ass_styles() + .iter() + .map(|style| { + let [name, fontname, fontsize, primary, secondary, outline, back] = + document.ass_style_text(style); + AssStyleDto { + name: name.to_owned(), + fontname: fontname.to_owned(), + fontsize: fontsize.to_owned(), + primary: primary.to_owned(), + secondary: secondary.to_owned(), + outline: outline.to_owned(), + back: back.to_owned(), + bold: style.bold, + italic: style.italic, + underline: style.underline, + strikeout: style.strikeout, + } + }) + .collect() +} + pub(crate) fn describe(session: &EditSession, patch: CuePatch) -> CuePatchDto { CuePatchDto { revision: session.revision(), @@ -1333,6 +1401,7 @@ pub(crate) fn describe(session: &EditSession, patch: CuePatch) -> CuePatchDto { can_redo: session.can_redo(), dirty: session.dirty(), truncated: session.truncated(), + styles: ass_styles(session.document()), } } diff --git a/src/App.tsx b/src/App.tsx index 78482c3..8654590 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from "react import { choosePath, type ChooseKind } from "./chooser"; import AboutDialog from "./components/AboutDialog"; +import StyleEditor from "./components/StyleEditor"; import CueList from "./components/CueList"; import CurrentLine from "./components/CurrentLine"; import FindBar, { type FindMode } from "./components/FindBar"; @@ -486,6 +487,8 @@ export default function App() { // them all. const [choosing, setChoosing] = useState(false); const [aboutOpen, setAboutOpen] = useState(false); + /** Which declared style the editor is open over, or null while it is closed. See B10. */ + const [editingStyle, setEditingStyle] = useState(null); // Absent until the menu asks for it, and gone again on Close: T4 takes the band off the screen. const [transcribeOpen, setTranscribeOpen] = useState(false); // The find band, and what it is looking for. The query outlives a close so reopening the band @@ -1670,6 +1673,14 @@ export default function App() { styles={subtitle.summary?.styles.map((style) => style.name) ?? []} canComment={subtitle.summary?.format === "ass"} onCommitComment={(cue, comment) => subtitle.setComment(cue, comment)} + onEditStyle={() => { + const at = (subtitle.summary?.styles ?? []).findIndex( + (style) => style.name === activeCue?.style, + ); + if (at >= 0) { + setEditingStyle(at); + } + }} canWriteTag={writesAtCaret} caretAt={writesAtCaret && caret !== null ? caret.offset : null} fonts={fonts.families} @@ -1806,6 +1817,18 @@ export default function App() { moduleRefusals={modules.refused.map((refused) => refusalLine(refused, en.modules))} /> {aboutOpen && setAboutOpen(false)} />} + {/* The style the editor was opened over may go with an undo or a reopen, so the panel is + drawn only while the document still declares one at that place. */} + {editingStyle !== null && subtitle.summary?.styles[editingStyle] !== undefined && ( + subtitle.setStyleField(index, field, value)} + onClose={() => setEditingStyle(null)} + /> + )} ); diff --git a/src/components/CurrentLine.tsx b/src/components/CurrentLine.tsx index 722c059..2f37531 100644 --- a/src/components/CurrentLine.tsx +++ b/src/components/CurrentLine.tsx @@ -47,6 +47,8 @@ type CurrentLineProps = { commands: CommandRegistry; /** The names the document's styles section declares, in its own order. See edit-bar-tasks C5. */ styles: string[]; + /** Open the editor over the style the line names. Greyed on a row that names none (B10). */ + onEditStyle: () => void; /** Whether the format has a descriptor at all: only ASS has one, so only ASS can be commented. */ canComment: boolean; onCommitComment: (cue: number, comment: boolean) => Promise; @@ -258,6 +260,7 @@ export default function CurrentLine({ fontsLoading, onLoadFonts, styles, + onEditStyle, canComment, onCommitComment, }: CurrentLineProps) { @@ -1044,6 +1047,15 @@ export default function CurrentLine({
{commentField()} {styleField()} + {/* Row one of the reference puts Edit right after the dropdown, not at the end (B10). */} + {comboField("actor", en.subtitle.currentLine.actor, en.subtitle.currentLine.actorNames)} {comboField("effect", en.subtitle.currentLine.effect, en.subtitle.currentLine.effectValues)} diff --git a/src/components/StyleEditor.tsx b/src/components/StyleEditor.tsx new file mode 100644 index 0000000..a1c8004 --- /dev/null +++ b/src/components/StyleEditor.tsx @@ -0,0 +1,167 @@ +import { useLayoutEffect, useRef, useState } from "react"; + +import { useLayer } from "../hooks/useLayers"; +import { en } from "../i18n/en"; +import { type AssStyle, type AssStyleField } from "../types/subtitle"; + +type StyleEditorProps = { + /** Which declared style is being edited, by its place in the document's own order. */ + index: number; + style: AssStyle; + /** The families installed on this machine, empty until the editor asks for them. */ + fonts: string[]; + onLoadFonts: () => void; + /** Write one field. Each is its own undo step, which is what the reference's own dialog does. */ + onCommit: (index: number, field: AssStyleField, value: string) => Promise; + onClose: () => void; +}; + +/** The fields this editor writes, in the order the reference's own dialog puts them. */ +const TEXT_FIELDS: { field: AssStyleField; label: string }[] = [ + { field: "fontname", label: en.subtitle.styleEditor.fontname }, + { field: "fontsize", label: en.subtitle.styleEditor.fontsize }, + { field: "primary", label: en.subtitle.styleEditor.primary }, + { field: "secondary", label: en.subtitle.styleEditor.secondary }, + { field: "outline", label: en.subtitle.styleEditor.outline }, + { field: "back", label: en.subtitle.styleEditor.back }, +]; + +/** The four flags, which ASS writes as -1 for on and 0 for off. */ +const FLAG_FIELDS: { field: AssStyleField; label: string; of: (style: AssStyle) => boolean }[] = [ + { field: "bold", label: en.subtitle.styleEditor.bold, of: (style) => style.bold }, + { field: "italic", label: en.subtitle.styleEditor.italic, of: (style) => style.italic }, + { field: "underline", label: en.subtitle.styleEditor.underline, of: (style) => style.underline }, + { field: "strikeout", label: en.subtitle.styleEditor.strikeout, of: (style) => style.strikeout }, +]; + +function held(style: AssStyle, field: AssStyleField): string { + switch (field) { + case "fontname": + return style.fontname; + case "fontsize": + return style.fontsize; + case "primary": + return style.primary; + case "secondary": + return style.secondary; + case "outline": + return style.outline; + case "back": + return style.back; + default: + return ""; + } +} + +/** + * What Edit beside the Style dropdown opens: one declared style, field by field. + * + * Each field is committed on its own, when it is left, and each is its own undo step. The name is + * not among them: renaming a style means rewriting every event that names it, which is a different + * operation. See edit-bar-tasks.md B10. + */ +export default function StyleEditor({ + index, + style, + fonts, + onLoadFonts, + onCommit, + onClose, +}: StyleEditorProps) { + const panelRef = useRef(null); + /** What each field holds while it is being typed into, before it is sent. */ + const [draft, setDraft] = useState>>({}); + // Mounted only while the panel is open (decision 1, T8). + useLayer(true); + + useLayoutEffect(() => { + panelRef.current?.focus(); + onLoadFonts(); + }, [onLoadFonts]); + + const value = (field: AssStyleField) => draft[field] ?? held(style, field); + + async function commit(field: AssStyleField) { + const typed = draft[field]; + setDraft((current) => ({ ...current, [field]: undefined })); + if (typed === undefined || typed === held(style, field)) { + return; + } + await onCommit(index, field, typed); + } + + return ( +
{ + if (event.target === event.currentTarget) { + onClose(); + } + }} + > +
{ + if (event.key === "Escape") { + event.preventDefault(); + onClose(); + } + }} + > +

{style.name}

+ {TEXT_FIELDS.map(({ field, label }) => ( + + ))} + {/* The families this machine has, offered to the font field and typed into anywhere else. */} + + {fonts.map((name) => ( + +
+ {FLAG_FIELDS.map(({ field, label, of }) => ( + + ))} +
+ +
+
+ ); +} diff --git a/src/hooks/useSubtitleFile.ts b/src/hooks/useSubtitleFile.ts index f89ff10..8fe8aaa 100644 --- a/src/hooks/useSubtitleFile.ts +++ b/src/hooks/useSubtitleFile.ts @@ -8,6 +8,7 @@ import { type RowRef } from "../types/chrome"; import { isSubtitleError, type AssFieldName, + type AssStyleField, type StyleFlagName, type CuePatch, type CueRow, @@ -107,6 +108,8 @@ export type SubtitleFile = { toggleStyle: (cue: number, flag: StyleFlagName, from: number, to: number) => Promise; /** Several override tags at one caret, as one undo step: a font is a family and a size. B12. */ setOverrideTags: (cue: number, tags: [string, string][], at: number) => Promise; + /** One field of one declared style. Each field is its own undo step. See edit-bar-tasks B10. */ + setStyleField: (style: number, field: AssStyleField, value: string) => Promise; /** Empty one line. `keepTags` leaves the braced runs and drops only the words. See B13. */ clearText: (cue: number, keepTags: boolean) => Promise; /** Begin a translation from the source: same cues, same timings, nothing written yet. See S2. */ @@ -202,7 +205,11 @@ export function useSubtitleFile(onRowsMoved: RowsMoved, onPanels: PanelSink): Su ...patch.cues, ...current.slice(patch.from + patch.removed), ]); - setSummary((current) => (current === null ? null : { ...current, cueCount: patch.cueCount })); + // The styles ride on every patch because a style write changes no cue: nothing else in the + // patch would tell the panel and the dropdown that one moved. See edit-bar-tasks.md B10. + setSummary((current) => + current === null ? null : { ...current, cueCount: patch.cueCount, styles: patch.styles }, + ); setCanUndo(patch.canUndo); setCanRedo(patch.canRedo); setDirty(patch.dirty); @@ -418,6 +425,12 @@ export function useSubtitleFile(onRowsMoved: RowsMoved, onPanels: PanelSink): Su [command], ); + const setStyleField = useCallback( + (style: number, field: AssStyleField, value: string) => + command("subtitle_set_style_field", { style, field, value }), + [command], + ); + const clearText = useCallback( (cue: number, keepTags: boolean) => command("subtitle_clear_text", { cue, keepTags }), [command], @@ -521,6 +534,7 @@ export function useSubtitleFile(onRowsMoved: RowsMoved, onPanels: PanelSink): Su setOverrideTags, newTranslation, clearText, + setStyleField, insertCue, deleteCue, splitCue, diff --git a/src/i18n/en.ts b/src/i18n/en.ts index e06a4b9..0b6e006 100644 --- a/src/i18n/en.ts +++ b/src/i18n/en.ts @@ -385,6 +385,26 @@ export const en = { "Those cues are gone: another transcription has started since. Run it again.", commandFailed: "Sublore could not finish that action. Restart Sublore if it happens again.", }, + /** + * What Edit beside the Style dropdown opens. Every field is one write and one undo step, and + * the name is not among them: renaming a style rewrites every line that names it. See B10. + */ + styleEditor: { + /** The button beside the Style dropdown, which is where the reference's own row puts it. */ + edit: "Edit", + title: "Style", + fontname: "Font", + fontsize: "Size", + primary: "Primary colour", + secondary: "Secondary colour", + outline: "Outline colour", + back: "Shadow colour", + bold: "Bold", + italic: "Italic", + underline: "Underline", + strikeout: "Strikeout", + close: "Close", + }, /** Said while a list the interface asked for has not come back yet. */ reading: "Reading…", /** Said on the status bar while a second document is open to read from. See S1. */ diff --git a/src/styles/chrome.css b/src/styles/chrome.css index 60c7a57..43b037f 100644 --- a/src/styles/chrome.css +++ b/src/styles/chrome.css @@ -136,6 +136,81 @@ /* Help > About. It covers the window rather than dimming it: every colour is a token and the palette has no scrim in it. */ +/* The style editor, over the shell like About and closed the same three ways. See B10. */ +.styleeditor { + position: fixed; + z-index: 40; + display: flex; + align-items: center; + justify-content: center; + inset: 0; + background: rgb(0 0 0 / 55%); +} + +.styleeditor__panel { + display: flex; + flex-direction: column; + gap: 0.4rem; + width: min(26rem, 90vw); + padding: 1rem; + border: 1px solid var(--line-strong); + background: var(--bg-panel); +} + +.styleeditor__title { + margin: 0 0 0.3rem; + font-size: 1rem; +} + +.styleeditor__field { + display: flex; + align-items: baseline; + gap: 0.5rem; +} + +.styleeditor__label { + flex: none; + width: 9rem; + color: var(--ink-muted); +} + +.styleeditor__value { + flex: 1; + min-width: 0; + padding: 0.1rem 0.3rem; + border: 1px solid var(--line); + background: var(--bg-sunken); + color: inherit; + font: inherit; +} + +.styleeditor__value:disabled { + color: var(--ink-faint); + background: var(--bg-panel); +} + +.styleeditor__flags { + display: flex; + gap: 0.9rem; + margin-top: 0.3rem; +} + +.styleeditor__flag { + display: flex; + align-items: center; + gap: 0.3rem; +} + +.styleeditor__close { + align-self: flex-end; + margin-top: 0.5rem; + padding: 0.15rem 0.8rem; + border: 1px solid var(--line); + background: var(--bg-sunken); + color: inherit; + font: inherit; +} + .about { position: fixed; z-index: 20; diff --git a/src/styles/tools.css b/src/styles/tools.css index 7a9d5f1..ff25d35 100644 --- a/src/styles/tools.css +++ b/src/styles/tools.css @@ -136,6 +136,11 @@ background: var(--bg-panel); } +/* Edit, beside the Style dropdown. Greyed on a row whose style the document does not declare. */ +.currentline__style-edit { + flex: none; +} + /* The four colours a line can override. They sit after the style buttons, which is where row three of the reference puts them, and they grey with no caret to write at. Each is a letter over a bar, the way the reference draws them: the bar names which colour the button sets and is fixed, never diff --git a/src/types/subtitle.ts b/src/types/subtitle.ts index 3a87dd1..2dc1162 100644 --- a/src/types/subtitle.ts +++ b/src/types/subtitle.ts @@ -8,6 +8,22 @@ export type SubtitleNewline = "lf" | "crlf" | "mixed" | "none"; * One declared style as an editor reads it: every value the file's own spelling, and the four flags * as booleans because that is what a line's own override tags start from. */ +/** + * Which column of a `Style:` line a write names. The name is not on it: renaming a style means + * rewriting every event that names it, which is a different operation. See edit-bar-tasks.md B10. + */ +export type AssStyleField = + | "fontname" + | "fontsize" + | "primary" + | "secondary" + | "outline" + | "back" + | "bold" + | "italic" + | "underline" + | "strikeout"; + export type AssStyle = { name: string; fontname: string; @@ -107,6 +123,8 @@ export type CuePatch = { canRedo: boolean; dirty: boolean; truncated: boolean; + /** The styles as they stand: a style write changes no cue, so nothing else here would say it. */ + styles: AssStyle[]; }; export type SubtitleSaved = {