Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions BACKLOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,12 @@ Two items jump the queue by owner decision. Full reasoning in `decisions.md`; th

- [ ] **N38 The current line's box keeps its pixels while its contents scale, so a large interface loses a control off the bottom (filed 2026-09-06).** The top block opens at a pixel count and an interface size change deliberately does not move it: `interface-scale.spec.js`'s "leaves the three panels at the proportions the sashes were left at when the size changes" asserts `bigger.block === left.block`, and its reason is the waveform, whose measurements are in device pixels because a peak bucket is one millisecond. Everything inside the panel is in rem and does scale. So at 150 per cent in the narrowest window the panel is 1.5 times fuller in a box that has not moved, and the text box needs the panel's scroll to be reached. It has been paid for three times by raising the block's opening height, from 13.5rem to 21rem as the panel gained the effect, the drawing order, the three margins, Next line and the style dropdown, and raising it cannot fix the 150 per cent case because the shortfall there grows with the size. The two candidates are scaling the stored heights with the interface size, which contradicts that criterion and needs the owner, and the reference's own answer, which is merging and splitting the panel's rows by width (edit-bar-tasks.md question 1).

- [ ] **N39 The font button of the current line's row three, and the two halves of the colour picker that are not built (filed 2026-09-06).** 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.

- [ ] **N40 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.
- **A second check joined it on 2026-09-06:** `editor.spec.js`'s "scrolls a viewport at a time without falling behind" failed on the runner and has never failed here. Two timing-sensitive checks failing only on CI is a property of the runner as much as of either check, and it blocks every pull request, so it is the owner's to weigh: the suite cannot be trusted to be green there today.

- [ ] **N37 The scaled surface check misses the doubling by eight pixels, about one run in several (filed 2026-09-06).** `e2e/scripts/scaled-surface-check.js` asserts the video surface doubles when `GDK_SCALE` doubles, within three pixels. On CI job 101433553363 it read 346x166 at ratio 1 and 700x342 at ratio 2, which is eight and ten pixels over twice, and the same script printed `5/5 checks` on the run before it with nothing changed between them. The failure message reads as though the surface had not been resolved to native pixels, which is not what these numbers say: the scale is applied, and the layout under it settles a few pixels away from where it settles at ratio 1. The candidates are the ruler band, whose height is a rounded number of device pixels and therefore not exactly half at ratio 2, and the surface being measured before the last layout pass at a size where that pass takes longer. **Second sighting, 2026-09-06, and it says the difference is fixed rather than random.** Job 101441553639 on a build whose top block is taller read 346x230 at ratio 1 and 700x470 at ratio 2. The first sighting read 346x166 and 700x342. The heights differ between the two runs and **the shortfall does not**: eight pixels of width and ten of height over twice, both times. That is the shape of a fixed inset in device pixels that is applied once at each ratio instead of scaling with it, not of a layout that settles late, and it rules out the ruler band, whose height does change between those runs. The script still prints only the surface. The way to name it is to print the stage rectangle the page sends and the rectangle the backend applies, at both ratios, and see which of the two carries the eight and the ten.

- [ ] **N36 A media with no picture sometimes says "Open a video first." to the translator, on the runner and not here (filed 2026-09-06).** `video-aspect.spec.js`'s "says a media with no picture has none, and is as quiet about it as about no audio" failed on CI job 101422670879 with `["Open a video first."]` where it expects no alert at all, at `video-aspect.spec.js:281`. That string is `video.errors.notLoaded`, which the status bar draws from `useVideoPlayer`'s error code, and the backend answers `NotLoaded` for mpv's `PropertyUnavailable` as well as for a genuinely closed player (`src-tauri/src/video/error.rs:81`). So a property read that lands in the window around an open is shown to a person as an instruction to do the thing they just did. This is the twin of the audio fix of 2026-09-05: a media with no video track is not an error and must be as quiet as a machine with no sound. The fix is to find which call answers it and stop that answer reaching the status bar, and the way to prove it is to force the answer and watch the check go red.
Expand Down
95 changes: 95 additions & 0 deletions crates/sublore-edit/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ pub enum Edit {
from: usize,
to: usize,
},
/// Write one override tag with a value the caller chose, over the same stretch a style toggle
/// works on. The pickers use this where the four flags use `ToggleStyle`: a colour is picked
/// rather than flipped, so there is no state to read first. See edit-bar-tasks.md B12.
SetOverrideTag {
cue: usize,
tag: String,
value: String,
from: usize,
to: usize,
},
/// Turn an ASS event into a `Comment:` or back into a `Dialogue:`. The descriptor is not one
/// of the fields `AssField` can name, and this changes how many cues a player would draw, so it
/// is its own edit. See edit-bar-tasks.md B8.
Expand Down Expand Up @@ -148,6 +158,13 @@ pub fn plan(document: &SubtitleDocument, edit: &Edit) -> Result<Planned, EditErr
from,
to,
} => plan_toggle_style(document, *cue, *flag, *from, *to),
Edit::SetOverrideTag {
cue,
tag,
value,
from,
to,
} => plan_set_override_tag(document, *cue, tag, value, *from, *to),
Edit::Insert {
before,
start_ms,
Expand Down Expand Up @@ -1105,6 +1122,84 @@ fn validate_field_value(field: AssField, value: &str) -> Result<(), EditError> {
/// The flag's state at the caret, then the opposite of it written there, and the state it had put
/// back at the far end of the selection shifted by whatever the first write inserted. That is the
/// whole of it, and it is why the writer returns a shift.
/// The same write a style toggle makes, with the value given rather than worked out. A tag name
/// that is not a backslash and letters is refused: everything downstream reads a name that way, and
/// a value carrying a brace would close the block it was written into.
fn plan_set_override_tag(
document: &SubtitleDocument,
index: usize,
tag: &str,
value: &str,
from: usize,
to: usize,
) -> Result<Planned, EditError> {
let named = tag.strip_prefix('\\').unwrap_or("");
// One digit may lead, because the numbered colours and alphas are spelt `\\2c` and `\\1a`, and
// after it the name is letters to the end: whatever follows those is the value.
let letters = named
.strip_prefix(|first: char| first.is_ascii_digit())
.unwrap_or(named);
if letters.is_empty() || !letters.bytes().all(|byte| byte.is_ascii_alphabetic()) {
return Err(EditError::new(
EditErrorKind::NotApplicable,
format!(
"{tag} is not a tag name: a name is a backslash, one digit at most, then letters"
),
));
}
if value.contains(['{', '}', '\\']) {
return Err(EditError::new(
EditErrorKind::NotApplicable,
"a tag value may not carry a brace or a backslash",
));
}
let located = locate(document, index)?;
if !matches!(&located.cue.detail, CueDetail::Ass(_)) {
return Err(EditError::new(
EditErrorKind::NotApplicable,
"only an ASS event carries override tags",
));
}
let text = document.slice(located.cue.text);
if from > text.len()
|| to > text.len()
|| !text.is_char_boundary(from)
|| !text.is_char_boundary(to)
{
return Err(EditError::new(
EditErrorKind::NotApplicable,
format!("the range {from}..{to} is outside the cue's text or cuts a character"),
));
}
let (start, _) = if from <= to { (from, to) } else { (to, from) };
let (written, _) = override_tags::set_tag(text, start, tag, value);

let write = plan_text_write(document, &located, &written)?;
Ok(Planned {
splice: Splice::new(
write.range.start,
document.slice(write.range).to_owned(),
write.inserted,
),
label: EditLabel {
kind: EditKind::SetOverrideTag,
cue: index,
},
expect: Expectation {
from: index,
removed: 1,
cues: vec![ExpectedCue {
text_raw: write.written,
start_ms: located.cue.start.millis(),
end_ms: located.cue.end.millis(),
}],
segments_from: located.segment_index,
segments_removed: 1,
segments_inserted: 1,
},
})
}

fn plan_toggle_style(
document: &SubtitleDocument,
index: usize,
Expand Down
3 changes: 3 additions & 0 deletions crates/sublore-edit/src/splice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ pub enum EditKind {
/// Which flag is on the label, for the reason `SetField` carries its field: bold and italic on
/// one line must never merge into one undo step.
ToggleStyle(sublore_formats::override_tags::StyleFlag),
/// 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,
Insert,
Delete,
Split,
Expand Down
61 changes: 61 additions & 0 deletions crates/sublore-edit/tests/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,67 @@ fn a_field_committed_as_whitespace_writes_nothing_however_often_it_is_committed(
);
}

fn set_override_tag(cue: usize, tag: &str, value: &str, from: usize, to: usize) -> Edit {
Edit::SetOverrideTag {
cue,
tag: tag.to_owned(),
value: value.to_owned(),
from,
to,
}
}

#[test]
fn a_tag_with_a_chosen_value_is_written_where_the_caret_is() {
// B12: a colour is picked rather than flipped, so the value comes from the caller.
let mut session = session("ass/clean/basic.ass");
let text = raw_text(&session, 0);
session
.apply(
&set_override_tag(0, "\\c", "&H0000FF&", 0, 0),
Run::New,
Instant::now(),
)
.expect("an ASS event takes an override tag");
assert_eq!(raw_text(&session, 0), format!("{{\\c&H0000FF&}}{text}"));

session.undo().expect("a step to undo").expect("a patch");
assert_eq!(raw_text(&session, 0), text);
}

#[test]
fn a_numbered_colour_replaces_the_one_already_in_the_block_rather_than_joining_it() {
// B12: `\\2c` is one name, so a second pick of the same colour is not a second tag.
let mut session = session("ass/clean/basic.ass");
let text = raw_text(&session, 0);
for value in ["&H0000FF&", "&H00FF00&"] {
session
.apply(
&set_override_tag(0, "\\2c", value, 0, 0),
Run::New,
Instant::now(),
)
.expect("an ASS event takes a numbered colour");
}
assert_eq!(raw_text(&session, 0), format!("{{\\2c&H00FF00&}}{text}"));
}

#[test]
fn a_tag_name_that_is_not_a_name_and_a_value_that_could_close_a_block_are_both_refused() {
let mut session = session("ass/clean/basic.ass");
let before = session.to_bytes();
for (tag, value) in [("c", "&H0&"), ("\\1c1", "&H0&"), ("\\c", "&H0&}x{\\b1")] {
session
.apply(
&set_override_tag(0, tag, value, 0, 0),
Run::New,
Instant::now(),
)
.expect_err("neither a bare name nor a value carrying a brace is written");
}
assert_eq!(session.to_bytes(), before, "a refusal writes nothing");
}

fn toggle_style(cue: usize, flag: StyleFlag, from: usize, to: usize) -> Edit {
Edit::ToggleStyle {
cue,
Expand Down
37 changes: 32 additions & 5 deletions crates/sublore-formats/src/override_tags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,21 @@ pub struct Block {
pub span: Span,
}

/// Whether a braced run holds a tag. A tag is a backslash followed by a letter, and a run with none
/// is a comment: `{note}` is a note and `{\b1}` is styling.
/// Whether a braced run holds a tag. A tag is a backslash and a name, and a run with none is a
/// comment: `{note}` is a note and `{\b1}` is styling.
fn holds_a_tag(inside: &str) -> bool {
let bytes = inside.as_bytes();
bytes
.iter()
.enumerate()
.any(|(at, byte)| *byte == b'\\' && bytes.get(at + 1).is_some_and(u8::is_ascii_alphabetic))
.any(|(at, byte)| *byte == b'\\' && names_a_tag(bytes, at))
}

/// Whether the backslash at `at` opens a name: one digit at most, then at least one letter. The
/// digit is there because the numbered colours and alphas are spelt `\2c` and `\1a`.
fn names_a_tag(bytes: &[u8], at: usize) -> bool {
let letters = at + 1 + usize::from(bytes.get(at + 1).is_some_and(u8::is_ascii_digit));
bytes.get(letters).is_some_and(u8::is_ascii_alphabetic)
}

/// The drawing scale a braced run leaves behind it: the last `\p<digits>` in it, or `None` when it
Expand Down Expand Up @@ -185,13 +192,14 @@ pub fn tags_in(text: &str, block: Block) -> Vec<Tag> {
at += 1;
continue;
}
let mut after = at + 1;
let letters = at + 1 + usize::from(bytes.get(at + 1).is_some_and(u8::is_ascii_digit));
let mut after = letters;
while after < end && bytes[after].is_ascii_alphabetic() {
after += 1;
}
// A backslash with no letter after it is not a tag: `\\N` is a line break and its letter is
// taken by the name, which is right, and a trailing backslash names nothing.
if after == at + 1 {
if after == letters {
at += 1;
continue;
}
Expand Down Expand Up @@ -550,4 +558,23 @@ mod tests {
]
);
}

#[test]
fn a_numbered_colour_is_one_name_and_not_a_digit_before_a_value() {
let text = "{\\2c&H0000FF&}word";
let parsed = blocks(text);
assert_eq!(parsed[0].kind, BlockKind::Override);
let found = tags_in(text, parsed[0]);
assert_eq!(found.len(), 1);
assert_eq!(&text[found[0].name.range()], "\\2c");
assert_eq!(&text[found[0].value.range()], "&H0000FF&");
}

#[test]
fn a_backslash_and_a_digit_with_no_letter_after_it_names_nothing() {
let text = "{\\3}word";
let parsed = blocks(text);
assert_eq!(parsed[0].kind, BlockKind::Comment);
assert!(tags_in(text, parsed[0]).is_empty());
}
}
12 changes: 11 additions & 1 deletion e2e/specs/command-registry.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import { findToplevel } from "../lib/x11.js";
*/
const DECLARED = [
"file-open-subtitle",
"file-open-source",
"file-close-source",
"video-open",
"file-save",
"file-save-copy",
Expand Down Expand Up @@ -103,9 +105,14 @@ const TITLES = [
{ id: "help", label: "Help", disabled: false },
];

/** File with nothing open: Save, Save a copy and Discard are drawn, and all three are greyed. */
/**
* File with nothing open: Save, Save a copy and Discard are drawn, and all three are greyed, and
* so are the two that read a second document beside one that is not there (M2.6 S1).
*/
const FILE_ITEMS = [
{ id: "file-open-subtitle", disabled: false },
{ id: "file-open-source", disabled: true },
{ id: "file-close-source", disabled: true },
{ id: "video-open", disabled: false },
{ id: "file-save", disabled: true },
{ id: "file-save-copy", disabled: true },
Expand Down Expand Up @@ -501,6 +508,9 @@ describe("the command registry", () => {
// seed the cursor onto row 0 (decision 5): insert, delete and merge all only need that, so they
// ungrey too. Split stays gated behind a caret nothing has placed yet.
expect(flips(empty, open)).toEqual([
// A source is read beside a target, so the item that opens one wakes with the target (S1).
// Close source is not beside it: no source is open, so there is still nothing to close.
{ route: "menu", id: "file-open-source", disabled: false },
{ route: "menu", id: "file-save-copy", disabled: false },
// Find and Replace need a document and nothing else, so both ungrey with the file (F2, F3).
// Find next is absent from this list on purpose: it also needs a pattern, and nothing here
Expand Down
Loading