Skip to content

Read the accent from themes that annotate their colours - #6

Open
Zedster07 wants to merge 1 commit into
omacom:masterfrom
Zedster07:theme-accent-comments
Open

Zedster07 wants to merge 1 commit into
omacom:masterfrom
Zedster07:theme-accent-comments

Conversation

@Zedster07

Copy link
Copy Markdown

A colors.toml is written by hand, and people annotate it:

accent = "#4ECDC4"          # secondary-400 — the paired accent

accentFromColorsFile only stripped the quotes when the value both opened and closed with one. A trailing comment means it doesn't, so the quotes stayed on, QColor::fromString rejected the whole string, and the theme silently fell back to the built-in #FFD60A. The accent omacut followed was the one colour the theme had explicitly ruled out.

Nothing reports this — the fallback is a valid colour, so the app just looks like it isn't following the theme.

The fix

A quoted value is taken up to its closing quote. An unquoted one is cut at the first # that isn't the colour's own leading hash, so bare hex (accent = #4ECDC4 # note) and named colours (accent = teal # note) also survive a comment after them.

Verified against a real theme

Built both ways and read the same file — a theme whose accent line carries a comment:

build reads
master #FFD60A (the fallback)
this branch #4ECDC4 (the theme's accent)

./bin/test — 27 passing. themeAccentReadsOmarchyColors gains four cases: a commented double-quoted value, a commented single-quoted one, a commented bare hex, and a commented named colour. The existing cases are untouched and still pass, so uncommented themes read exactly as before.

Unrelated to #5 and based on master, so the two can go in either order.

A colors.toml is a file people write by hand, and they comment it:

    accent = "#4ECDC4"          # secondary-400 -- the paired accent

The quotes were only stripped when the value both opened and closed with
one, so a trailing comment left them in place, QColor rejected the whole
string, and the theme silently fell back to the built-in yellow. The
accent omacut followed was the one colour the theme had ruled out.

Take a quoted value up to its closing quote, and cut an unquoted one at
the first '#' that isn't the colour's own leading hash -- so bare hex and
named colours survive a note after them too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016rcJ8LUydunVeEgaT9hHDr

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes silent fallback to yellow when themes annotate accents. small and correct.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed, and the fix holds. Nothing to change here; the two things worth naming are both about malformed input rather than about themes.

Built and ran on a disposable VM rather than on the review machine, so none of this branch's code executed where credentials live. ./bin/test needs QT_QPA_PLATFORM=offscreen on a headless box or the binary aborts before the first test — that is the existing script, not this change. With that set: 27 passed, 0 failed, including the four cases added to themeAccentReadsOmarchyColors.

The before/after was measured rather than taken on trust. The same probe was compiled twice on that VM, once against 0948c46 and once against this head, over the annotated shapes: on master every one of them returned #FFD60A, and on this branch they return the theme's colour. The unquoted branch traces as described — for accent = #4ECDC4 # secondary-400 the value is #4ECDC4 # secondary-400, startsWith('#') is true so the search begins at index 1, the comment # is found at index 8, and left(8).trimmed() is #4ECDC4, so the colour's own hash survives.

Cases the added tests do not cover were run too, and all of them answer correctly: a comment with no space before it (accent = "#4ECDC4"#note), an = inside the comment on the accent line, an = inside the comment on a decoy line above it (mode = "dark" # accent = "#ff0000" followed by the real accent), an indented key, CRLF endings, a # inside a quoted named colour, and the annotated four-key palette shape. Splitting on the first = before stripping the comment is safe here, because on a key-value line a # can never precede the =, so the key is never misread. Empty and degenerate values (accent =, accent = ", accent = "", accent = '') all return the fallback without reading out of range, despite the value.size() >= 2 guard going away — front() is now reached only behind startsWith.

Two low findings, neither of which affects a well-formed theme:

  • An unterminated quote is now accepted. accent = "#aabbcc falls through to value.mid(1) and yields #aabbcc, where master returned the fallback. closing > 0 and closing >= 0 are equivalent, since indexOf(quote, 1) cannot return 0.
  • Trailing junk after the closing quote is ignored. accent = "#4ECDC4"junk returns #4ECDC4, and accent = "#4ECDC4" "#ff0000" returns the first. Both returned the fallback before.

Both are strictly more lenient than master on input that was never valid TOML, QColor::fromString still gates the result so nothing invalid can reach the binding, and no input that master read correctly changed. That is why nothing was pushed to the branch: requiring a balanced quote and only whitespace or a comment after it is a decision about how strict a theme reader should be, and it is the maintainer's to make rather than something to fold into a bug fix.

One thing the maintainer should see, since it is not visible from this repository alone. This is the same defect in three apps that each carry a copy of the parser, and all three now have a fix open, written by three different people to three different designs: omacom/omacalc#14 for omacalc#12 adds a line-level stripTomlComment() that walks the whole line tracking quote state and handles a backslash escape; this PR handles it per-value, splitting on = first; omacom/omawrite#34 for omawrite#32 adds a tomlValue() that only handles the quoted case. None dominates. This one is the only one of the three that reads an unquoted value with a comment after it — accent = teal # note and accent = #4ECDC4 # note both work here and fall back in the other two — and it is the only one that keeps master's existing support for bare #rrggbb, which the omacalc helper would strip to nothing. The omacalc helper is the only one that parses an escaped quote correctly (accent = "a\"b" closes on the escaped quote here), though that is unreachable for a colour, and its line-level shape is the one that generalises to more than one key, which matters in omacalc and omawrite where four are read and not here where only accent is. Choosing one shape for all three is a call for the maintainer, not for a review.

Reviewed by Claude Opus 5 in Claude Code and independently by Codex at xhigh reasoning. Codex reached the same conclusions on the traces and the empty-value bounds that had already been worked out here, so that is agreement rather than confirmation, and its independence is not currently guaranteed. The trailing-junk case above is its own: it was not part of the first pass, and it was verified on the VM before being written down.

Waiting on the maintainer for the merge and for the cross-repository shape decision. Nothing is needed from the author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants