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
2 changes: 2 additions & 0 deletions BACKLOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
172 changes: 172 additions & 0 deletions crates/sublore-edit/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -163,6 +171,11 @@ pub fn plan(document: &SubtitleDocument, edit: &Edit) -> Result<Planned, EditErr
to,
} => 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)
}
Expand Down Expand Up @@ -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<Planned, EditError> {
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
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 @@ -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,
Expand Down
74 changes: 73 additions & 1 deletion crates/sublore-edit/tests/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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");
Expand Down
4 changes: 4 additions & 0 deletions crates/sublore-formats/src/ass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
})
}

Expand Down
6 changes: 6 additions & 0 deletions crates/sublore-formats/src/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 21 additions & 2 deletions e2e/specs/current-line-bands.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
Expand All @@ -112,6 +130,7 @@ const SHORTFALL = {
},
150: {
floor: [
".currentline__start",
".currentline__end",
".currentline__text",
".currentline__edit-style-bold",
Expand Down
Loading