Fix CPS strategy not updating grid until restart (#11379)#11387
Merged
Conversation
Changing the CPS line-length strategy in Options had no visible effect: the new value was stored in Se.Settings and synced to Configuration.Settings by ApplySettings/SetLibSeSettings, but each row in the grid binds to computed properties (CharactersPerSecond, *BackgroundBrush) that never received PropertyChanged when the settings changed, so cells kept the previously- computed numbers. Raise notifications on every row at the end of ApplySettings. Also: - Add CalcIgnoreArabicDiacritics and CalcIgnoreArabicDiacriticsNoSpace to CalcFactory.Calculators - the settings dropdown offered them but the factory's name lookup fell through to CalcAll. - Per-line length labels under the textbox and the grid row text-too-long colouring now use line.CountCharacters(false) instead of raw line.Length, matching libse's other per-line length checks (SplitLongLinesHelper, FixLongLines, MoveWordUpDown) and making the per-line counts agree with the strategy-aware total. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request addresses CPS/line-length calculation consistency and UI refresh behavior in the subtitle grid and text info panel, ensuring that settings changes (notably CPS strategy) are reflected immediately without requiring edits or restart.
Changes:
- Refreshes each subtitle row’s computed/bound properties after applying settings so CPS/WPM values and error highlighting repaint immediately.
- Fixes missing CPS strategy implementations by registering the Arabic-diacritics calculators in
CalcFactory. - Aligns per-line length display and “text too long” detection with the configured strategy-aware character counting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/Features/Main/SubtitleLineViewModel.cs | Uses strategy-aware per-line length checks for highlighting and adds a helper to raise property-change notifications after settings updates. |
| src/ui/Features/Main/MainViewModel.cs | Triggers per-row refresh after ApplySettings() and updates per-line length display to use strategy-aware counting. |
| src/libse/Common/TextLengthCalculator/CalcFactory.cs | Registers missing calculators so strategy selections don’t silently fall back to CalcAll. |
Comment on lines
261
to
265
|
|
||
| foreach (var line in stripped.SplitToLines()) | ||
| { | ||
| if (line.Length > Se.Settings.General.SubtitleLineMaximumLength) | ||
| if (line.CountCharacters(false) > Se.Settings.General.SubtitleLineMaximumLength) | ||
| { |
Comment on lines
+518
to
+525
| OnPropertyChanged(nameof(CharactersPerSecond)); | ||
| OnPropertyChanged(nameof(WordsPerMinute)); | ||
| OnPropertyChanged(nameof(TextBackgroundBrush)); | ||
| OnPropertyChanged(nameof(DurationBackgroundBrush)); | ||
| OnPropertyChanged(nameof(CpsBackgroundBrush)); | ||
| OnPropertyChanged(nameof(WpmBackgroundBrush)); | ||
| OnPropertyChanged(nameof(GapBackgroundBrush)); | ||
| } |
Address Copilot review on PR #11387: PixelWidth depends on ShowColumnPixelWidth, ColorTextTooWide, and the font name/size used by CalculatePixelWidth, so it must also be re-notified when settings change. Otherwise the Pixel Width column stays stale until the row text changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #11379. Three related issues in the CPS/line-length pipeline:
CPS strategy change didn't repaint the grid. Changing the CPS strategy (or any other setting that feeds into CPS/length/colour calculations) and clicking OK updated
Se.Settingsand was synced intoConfiguration.SettingsbyApplySettings/SetLibSeSettings, but the rows themselves never receivedPropertyChangedfor the computed properties (CharactersPerSecond, the five*BackgroundBrushes,WordsPerMinute). Cells kept the previously-computed numbers until the user edited a row or restarted.ApplySettingsnow walksSubtitlesand calls a smallRefreshAfterSettingsChanged()helper on each row.Two strategies in the dropdown silently fell back to
CalcAll.CpsLineLengthStrategyDisplay.List()offeredCalcIgnoreArabicDiacriticsandCalcIgnoreArabicDiacriticsNoSpace, butCalcFactory.Calculatorsdidn't include them, so the name lookup inMakeCalculatorreturnednulland fell through to the?? new CalcAll()default. Added both to the list.Per-line length didn't agree with the total. The textbox info panel showed per-line lengths as raw
line.Lengthand the total ascleanText.CountCharacters(false)(strategy-aware), so with any "no spaces" / "no punctuation" strategy the per-line sum didn't equal the displayed total. Same raw-.Lengthmismatch in the grid'sTextBackgroundBrushfor the row text-too-long check. Both now useline.CountCharacters(false), matching libse's other per-line length checks (SplitLongLinesHelper,FixLongLines,MoveWordUpDown,Helper.csin FixCommonErrors).Test plan
SubtitleLineMaximumLengthin raw chars but under it after stripping spaces no longer gets a red row.🤖 Generated with Claude Code