jeirmeister/Cockatrice
1
0
Fork 0
mirror of https://github.com/Cockatrice/Cockatrice.git synced 2026-11-10 20:13:24 -08:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

Address review findings

Browse source
This commit is contained in:
Lukas Brübach 2026-11-08 14:50:35 +01:00
parent 181bd9d02c
commit d2af5c95f4
20 changed files with 154 additions and 97 deletions
Download patch file Download diff file Expand all files Collapse all files

13
cockatrice/src/interface/theme_config.cpp
View file

@ -458,17 +458,18 @@ QColor defaultAppColor(AppColor::Role role, bool dark)
switch (role) {
case AppColor::AccentStrong:
case AppColor::AccentSoft:
// The neutral icon family is palette-derived like the accent, so a
// theme without an authored icon family keeps following the platform
// instead of pinning one scheme's ink.
// The neutral icon family is palette-derived like the accent: a theme
// that leaves it unauthored (System, and every theme written before the
// roles existed) keeps the live platform palette's WindowText instead
// of pinning one scheme's ink.
case AppColor::Icon:
case AppColor::IconStrong:
case AppColor::IconSoft:
return {};
// The semantic family follows the same chroma ladder as the authored
// themes. A theme without stored values (System) has no per-surface
// palette to measure against, so these are authored against the
// representative platform surfaces the System theme runs on.
// themes. A theme that authors none of it has no per-surface palette to
// measure against, so these are authored against the representative
// platform surfaces the System theme runs on.
case AppColor::Positive:
return QColor(dark ? QStringLiteral("#6dd58c") : QStringLiteral("#1a7d3a"));
case AppColor::PositiveStrong:

10
cockatrice/src/interface/theme_config.h
View file

@ -63,11 +63,13 @@ Q_ENUM_NS(Role)
*
* Accent roles and the neutral Icon family return an invalid QColor because
* ThemeManager derives them from the palette: the accent from the highlight,
* the icon ink from the neutral text color plus the highlight. Semantic roles
* the icon ink from the live WindowText held to the label floor. Semantic roles
* return an opaque default that suits the scheme, so themes written before the
* role existed still render it. A theme with no authored icon family keeps
* deriving its icon ink from the platform palette instead of pinning one
* scheme's colors.
* role existed still render it. The System theme deliberately leaves the
* neutral family unauthored: the palette file it picks cannot know the platform
* scheme, so a fixed ladder would be invisible on the other one. It derives
* neutral ink from the live platform palette instead of pinning one scheme's
* colors.
*/
QColor defaultAppColor(AppColor::Role role, bool dark);

11
cockatrice/src/interface/theme_icon.cpp
View file

@ -64,10 +64,19 @@ AppColor::Role emphasisRole(AppColor::Role role, IconEmphasis emphasis)
*
* The fill needs the surface scheme to choose the badge glyph ink, and it cannot
* read it from the tone: an authored quiet level is a dark color on the dark
* scheme too.
* scheme too. The theme's resolved scheme is the same source the palette file
* and the default app colors are chosen from, so the fill follows it instead of
* re-deriving the scheme from the live Window lightness, which can disagree
* when a theme pins a scheme the platform palette does not match.
*/
ThemeIconFill::Scheme activeScheme()
{
if (themeManager != nullptr) {
return themeManager->isDarkModeActive() ? ThemeIconFill::Scheme::Dark : ThemeIconFill::Scheme::Light;
}
// No manager (test binaries link the icon engine without the app): fall
// back to the live palette, which is the only scheme signal left.
const QPalette &palette = qApp->palette();
return palette.color(QPalette::Active, QPalette::Window).lightness() < 128 ? ThemeIconFill::Scheme::Dark
: ThemeIconFill::Scheme::Light;

10
cockatrice/src/interface/theme_icon.h
View file

@ -104,11 +104,11 @@ public:
* hue and saturation stay the author's.
*
* Neutral label roles (WindowText, Text) resolve to icon ink rather than the
* raw palette color: ThemeIconInk::glyphInk derives it from the accent and
* holds the glyph to a contrast floor, because a 2px glyph in body-text ink
* reads thinner than the text beside it. A theme that authors the neutral
* icon roles overrides that derivation. Semantic roles keep the hue the call
* site asked for.
* raw palette color: an unauthored neutral family derives it from the live
* WindowText and holds the glyph to a contrast floor, because a 2px glyph in
* body-text ink reads thinner than the text beside it. A theme that authors
* the neutral icon roles overrides that derivation. Semantic roles keep the
* hue the call site asked for.
*
* Emphasis selects the family's authored variant, so the palette editor's
* rows are the whole source of truth. A caller that names a variant

44
cockatrice/src/interface/theme_icon_ink.cpp
View file

@ -177,26 +177,50 @@ double targetLuminance(const QColor &surface, double ratio)
*
* Luminance is monotone in HSV value at a fixed hue and saturation, so a
* bisection finds the boundary: the closest tone to the surface that still
* clears the floor, on either scheme. Returns an invalid color when even full
* value cannot reach the floor at that saturation.
* clears the floor, on either scheme. The direction depends on the surface:
* toward black (a light surface) the floor is a luminance ceiling, so the
* search keeps the highest passing value; toward white (a dark surface) it is a
* floor, so the search keeps the lowest passing value. Returns an invalid color
* when the tone cannot reach the floor at that saturation at all.
*/
QColor solveValue(int hue, int saturation, const QColor &surface, double floor)
{
const bool towardBlack = darkerPoleWins(surface);
const double target = targetLuminance(surface, floor + CONTRAST_ROUNDING_MARGIN);
if (ThemeIconInk::relativeLuminance(QColor::fromHsv(hue, saturation, 255)) < target) {
// Toward black a tone is legible at or below the target luminance, toward
// white at or above it. Testing the wrong side is what let a saturated
// violet at full value look "too dark" on a light surface and fall back to
// the unchanged secondary.
const auto passes = [hue, saturation, target, towardBlack](int value) {
const double luminance = ThemeIconInk::relativeLuminance(QColor::fromHsv(hue, saturation, value));
return towardBlack ? luminance <= target : luminance >= target;
};
if (passes(255)) {
// Toward black, full value is the palest tone: if even that clears the
// floor, the whole ramp does and the surface wants the authored tone.
// Toward white it is the palest tone but also the most legible one, so
// the search still starts from the failing end.
if (towardBlack) {
return QColor::fromHsv(hue, saturation, 255).toRgb();
}
} else if (!towardBlack) {
// Toward white the floor needs the palest tone; no darker value can
// reach it at this saturation.
return {};
}
int low = 0;
int high = 255;
int passing = towardBlack ? 0 : 255;
int failing = towardBlack ? 255 : 0;
for (int search = 0; search < CONTRAST_SEARCH_STEPS; ++search) {
const int middle = (low + high) / 2;
if (ThemeIconInk::relativeLuminance(QColor::fromHsv(hue, saturation, middle)) >= target) {
high = middle;
const int middle = (passing + failing) / 2;
if (passes(middle)) {
passing = middle;
} else {
low = middle;
failing = middle;
}
}
return QColor::fromHsv(hue, saturation, high).toRgb();
return QColor::fromHsv(hue, saturation, passing).toRgb();
}
/**

8
cockatrice/src/interface/theme_icon_ink.h
View file

@ -24,7 +24,9 @@
* on. The chroma is what makes it read as an icon rather than as leftover text;
* the floor keeps a theme with a weak text color from producing a glyph that is
* not legible. Neither step invents a color: both are derived from the palette
* the theme already ships.
* the theme already ships. An unauthored neutral family stops at the floor: the
* live platform palette is the only source that knows the actual scheme, so a
* chroma pinned in a palette file could belong to the other one.
*
* The rule only applies to the neutral label roles. A semantic tint
* (`AppColor::Positive`, `QPalette::Highlight`, `QPalette::Link`) is a hue the
@ -104,7 +106,9 @@ double defaultAccentMix();
* @param accent The theme accent, normally `QPalette::Highlight`.
* @param accentMix How much of the accent color to borrow. Defaults to
* defaultAccentMix(); previews and tests sweep it to compare
* strengths, production callers do not pass it.
* strengths. Production passes 0 for an unauthored neutral
* family, where the live platform palette is the only source
* that knows the actual scheme.
* @return The ink to resolve `currentColor` to, as an RGB color.
*/
QColor glyphInk(const QColor &text, const QColor &surface, const QColor &accent, double accentMix = defaultAccentMix());

11
cockatrice/src/interface/theme_manager.cpp
View file

@ -656,14 +656,17 @@ QColor ThemeManager::appColor(AppColor::Role role) const
case AppColor::IconStrong:
case AppColor::IconSoft: {
// Legacy fallback for a theme written before the icon roles
// existed, or one with no authored icon family. The three levels
// are the ink rule this series shipped, applied once here so
// authored roles never get recomputed at paint time.
// existed, or one that deliberately leaves the neutral family
// unauthored (System). No accent chroma is borrowed here: the
// palette file that would carry it cannot know the platform scheme,
// so neutral ink is the live WindowText held to the label floor.
// The three levels are the ink rule this series shipped, applied
// once here so authored roles never get recomputed at paint time.
const QPalette &palette = qApp->palette();
const QColor text = palette.color(QPalette::Active, QPalette::WindowText);
const QColor surface = ThemeIconInk::harderSurface(palette.color(QPalette::Active, QPalette::Window),
palette.color(QPalette::Active, QPalette::Button), text);
const QColor ink = ThemeIconInk::glyphInk(text, surface, accent);
const QColor ink = ThemeIconInk::glyphInk(text, surface, accent, 0.0);
switch (role) {
case AppColor::IconStrong:
return ThemeIconInk::forEmphasis(ink, surface, IconEmphasis::Primary);

2
cockatrice/themes/Fabric/palette-default-dark.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #30b758
PositiveSoft = #84a98f
Negative = #ed8077
NegativeStrong = #ff7367
NegativeSoft = #be9794
NegativeSoft = #be9794

2
cockatrice/themes/Fabric/palette-default-light.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #006b22
PositiveSoft = #40634b
Negative = #b0251d
NegativeStrong = #ba0a00
NegativeSoft = #7c504d
NegativeSoft = #7c504d

2
cockatrice/themes/Fusion/palette-default-dark.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #32bf5c
PositiveSoft = #8ab196
Negative = #f8867c
NegativeStrong = #ff8075
NegativeSoft = #c79e9b
NegativeSoft = #c79e9b

2
cockatrice/themes/Fusion/palette-default-light.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #007e28
PositiveSoft = #4c765a
Negative = #cf2c23
NegativeStrong = #da0c00
NegativeSoft = #935f5c
NegativeSoft = #935f5c

2
cockatrice/themes/Leather/palette-default-dark.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #2dab52
PositiveSoft = #7c9f86
Negative = #df7970
NegativeStrong = #ff5e50
NegativeSoft = #b38e8b
NegativeSoft = #b38e8b

2
cockatrice/themes/Leather/palette-default-light.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #006621
PositiveSoft = #3d5f48
Negative = #a7231c
NegativeStrong = #b10a00
NegativeSoft = #764c4a
NegativeSoft = #764c4a

2
cockatrice/themes/Plasma/palette-default-dark.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #2ba44f
PositiveSoft = #779881
Negative = #d5736b
NegativeStrong = #ff4f40
NegativeSoft = #ab8885
NegativeSoft = #ab8885

2
cockatrice/themes/Plasma/palette-default-light.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #006420
PositiveSoft = #3c5d47
Negative = #a4231b
NegativeStrong = #ae0900
NegativeSoft = #744b48
NegativeSoft = #744b48

20
cockatrice/themes/System/palette-default-dark.toml
View file

@ -1,21 +1,19 @@
# The System theme derives all of its colors from the platform palette
# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette
# overrides. Only the identity, neutral-ink and semantic app colors are
# provided here, so buttons keep their classic greens while the actual palette
# stays native to the OS.
# The neutral icon family is authored instead of derived: deriving it from the
# platform palette picked up the scheme's accent chroma, which painted neutral
# chrome blue. Neutral ink stays neutral here, black on the light scheme and
# white on the dark one, regardless of what the platform's accent carries.
# overrides. Only the identity and semantic app colors are provided here, so
# buttons keep their classic greens while the actual palette stays native to
# the OS.
# The neutral icon family is deliberately not authored: the palette file is
# picked from the theme's scheme, which on Qt < 6.5 is always light and on newer
# Qt can be unknown, so a fixed black-or-white ladder would be invisible on a
# platform palette of the other scheme. Neutral ink derives from the live
# WindowText instead.
[AppColors]
AccentStrong = #ff148c3c
AccentSoft = #ff78c850
Icon = #ffe0e0e0
IconStrong = #ffffffff
IconSoft = #ffb0b0b0
Positive = #ff6dd58c
PositiveStrong = #ff32c05c
PositiveSoft = #ff8ab196
Negative = #ffff8a80
NegativeStrong = #ffff8378
NegativeSoft = #ffc79e9b
NegativeSoft = #ffc79e9b

20
cockatrice/themes/System/palette-default-light.toml
View file

@ -1,21 +1,19 @@
# The System theme derives all of its colors from the platform palette
# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette
# overrides. Only the identity, neutral-ink and semantic app colors are
# provided here, so buttons keep their classic greens while the actual palette
# stays native to the OS.
# The neutral icon family is authored instead of derived: deriving it from the
# platform palette picked up the scheme's accent chroma, which painted neutral
# chrome blue. Neutral ink stays neutral here, black on the light scheme and
# white on the dark one, regardless of what the platform's accent carries.
# overrides. Only the identity and semantic app colors are provided here, so
# buttons keep their classic greens while the actual palette stays native to
# the OS.
# The neutral icon family is deliberately not authored: the palette file is
# picked from the theme's scheme, which on Qt < 6.5 is always light and on newer
# Qt can be unknown, so a fixed black-or-white ladder would be invisible on a
# platform palette of the other scheme. Neutral ink derives from the live
# WindowText instead.
[AppColors]
AccentStrong = #ff148c3c
AccentSoft = #ff78c850
Icon = #ff1a1a1a
IconStrong = #ff000000
IconSoft = #ff5f5f5f
Positive = #ff1a7d3a
PositiveStrong = #ff007e28
PositiveSoft = #ff4c7659
Negative = #ffb3261e
NegativeStrong = #ffda0b00
NegativeSoft = #ff935f5c
NegativeSoft = #ff935f5c

2
cockatrice/themes/VelvetMarble/palette-default-dark.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #2ba650
PositiveSoft = #789a82
Negative = #d8756c
NegativeStrong = #ff5344
NegativeSoft = #ad8a86
NegativeSoft = #ad8a86

2
cockatrice/themes/VelvetMarble/palette-default-light.toml
View file

@ -79,4 +79,4 @@ PositiveStrong = #006721
PositiveSoft = #3e6049
Negative = #aa241c
NegativeStrong = #b40a00
NegativeSoft = #784d4b
NegativeSoft = #784d4b

84
tests/theme_migration/theme_icon_render_test.cpp
View file

@ -80,16 +80,18 @@ int countOpaquePixels(const QPixmap &pixmap, const QColor &expected)
/**
* @brief The ink the engine resolves a neutral glyph to, for @p palette.
*
* Neutral glyphs are not painted with WindowText verbatim: theme_icon_ink
* borrows accent chroma and holds the result to a contrast floor. Tests that
* pin the rendered color ask the rule instead of restating it.
* Neutral glyphs are not painted with WindowText verbatim: theme_icon_ink holds
* the platform text color to a contrast floor. Tests that pin the rendered color
* ask the rule instead of restating it. No accent chroma is borrowed: this is
* the fallback an unauthored neutral family takes, so it works whatever scheme
* the platform palette actually is.
*/
QColor expectedNeutralInk(const QPalette &palette)
{
const QColor text = palette.color(QPalette::Active, QPalette::WindowText);
const QColor surface = ThemeIconInk::harderSurface(palette.color(QPalette::Active, QPalette::Window),
palette.color(QPalette::Active, QPalette::Button), text);
return ThemeIconInk::glyphInk(text, surface, palette.color(QPalette::Active, QPalette::Highlight));
return ThemeIconInk::glyphInk(text, surface, palette.color(QPalette::Active, QPalette::Highlight), 0.0);
}
/**
@ -327,10 +329,16 @@ TEST_F(ThemeIconRenderTest, SelectedPaletteTintUsesHighlightedText)
qApp->setPalette(palette);
const IconTint tint(QPalette::WindowText);
// The normal ink is derived from the active text color, but it is icon ink
// rather than the text color itself.
EXPECT_NE(tint.resolve(QIcon::Normal), activeText);
EXPECT_EQ(tint.resolve(QIcon::Normal), expectedNeutralInk(qApp->palette()));
// The normal ink is the accent-free neutral fallback: the platform text
// color held to the label floor. A text color that already clears the floor
// is the ink verbatim, so this pins the rule and the floor, not a shift.
const QColor normal = tint.resolve(QIcon::Normal);
EXPECT_EQ(normal, expectedNeutralInk(qApp->palette()));
const QPalette resolved = qApp->palette();
const QColor surface = ThemeIconInk::harderSurface(resolved.color(QPalette::Active, QPalette::Window),
resolved.color(QPalette::Active, QPalette::Button),
resolved.color(QPalette::Active, QPalette::WindowText));
EXPECT_GE(ThemeIconInk::contrastRatio(normal, surface), ThemeIconInk::labelContrastFloor());
// A selected row paints on the highlight, so the ink is the palette's
// highlighted text rather than the icon ink.
EXPECT_EQ(tint.resolve(QIcon::Selected), selectedText);
@ -1194,9 +1202,9 @@ InkPalette shippedPalette(const QString &theme, bool dark)
// A theme may author only [AppColors] (System does), so appColors alone do
// not mean it ships a palette: the keys have to be present. The authored
// neutral-family assertions therefore cover only themes with a palette;
// System's authored neutral family is checked for authorship and ordering
// by ShippedPalettesAuthorTheEmphasisLadder, and its semantic fallbacks
// against representative platform surfaces by
// System deliberately leaves its neutral family unauthored, which
// ShippedPalettesAuthorTheEmphasisLadder pins, and its semantic fallbacks
// are measured against representative platform surfaces by
// DefaultSemanticFamilyMeetsTheLadderOnPlatformSurfaces.
if (config.colors.isEmpty()) {
return {};
@ -1211,9 +1219,9 @@ InkPalette shippedPalette(const QString &theme, bool dark)
/**
* @brief The [AppColors] a shipped theme authors, even when it ships no palette.
*
* System deliberately has no palette of its own, but it still authors every
* app-color role, including the neutral icon family that used to be derived
* from the platform palette.
* System deliberately has no palette of its own, but it still authors its
* semantic roles. The neutral icon family stays unauthored there so it follows
* the live platform palette; see ShippedPalettesAuthorTheEmphasisLadder.
*/
QMap<AppColor::Role, QColor> shippedAppColors(const QString &theme, bool dark)
{
@ -1257,6 +1265,29 @@ TEST(IconInk, ContrastFloorPicksThePoleItCanReach)
EXPECT_GE(ThemeIconInk::contrastRatio(solved, background), 4.5);
}
/**
* @brief Primary stays louder than secondary on a light surface.
*
* A saturated blue or violet at full value is already dark enough to clear the
* label floor on a light surface. When solveValue() read the target as a
* minimum in both directions it rejected that tone, walked the whole saturation
* ladder and fell back to the unchanged secondary, so primary was never
* distinguishable. #5a3ee2 on #efeff4 is the shipped Plasma accent's shape.
*/
TEST(IconInk, PrimaryRaisesAChromaticToneOnALightSurface)
{
const QColor surface(QStringLiteral("#efeff4"));
const QColor violet(QStringLiteral("#5a3ee2"));
ASSERT_GE(ThemeIconInk::contrastRatio(violet, surface), ThemeIconInk::labelContrastFloor())
<< "the fixture must already be legible, so the fallback would be a no-op";
const QColor primary = ThemeIconInk::forEmphasis(violet, surface, IconEmphasis::Primary);
ASSERT_TRUE(primary.isValid());
EXPECT_GE(ThemeIconInk::contrastRatio(primary, surface), ThemeIconInk::labelContrastFloor());
EXPECT_GT(primary.hsvSaturation(), violet.hsvSaturation()) << "primary fell back to the unchanged secondary";
EXPECT_NEAR(primary.hue(), violet.hue(), 2);
}
/**
* @brief A green or red accent must not tint neutral chrome.
*
@ -1675,30 +1706,17 @@ TEST(IconInk, ShippedPalettesAuthorTheEmphasisLadder)
EXPECT_GT(checked, 0) << "no shipped palette was loadable";
// The System theme ships no palette of its own, so its families have no
// authored surfaces to measure against. It still authors the neutral family
// instead of deriving it: deriving picked up the platform accent's chroma,
// which painted neutral chrome blue on a default Linux scheme.
// authored surfaces to measure against. It nevertheless authors the
// semantic family, while the neutral family stays unauthored on purpose:
// the palette file is chosen from the theme's scheme, which is always light
// on Qt < 6.5 and can be unknown on newer Qt, so one fixed ladder would be
// invisible against a platform palette of the other scheme.
for (const bool dark : {false, true}) {
const QMap<AppColor::Role, QColor> system = shippedAppColors(QStringLiteral("System"), dark);
EXPECT_TRUE(system.contains(AppColor::PositiveStrong));
EXPECT_TRUE(system.contains(AppColor::NegativeSoft));
for (const AppColor::Role role : {AppColor::Icon, AppColor::IconStrong, AppColor::IconSoft}) {
ASSERT_TRUE(system.contains(role)) << (dark ? "dark" : "light") << static_cast<int>(role);
EXPECT_EQ(system.value(role).hsvSaturation(), 0) << (dark ? "dark" : "light") << static_cast<int>(role);
}
// Strong is the loudest level a neutral family can reach: the darkest
// tone on the light scheme, the palest on the dark one. Quiet recedes
// toward the surface on both.
const double secondary = ThemeIconInk::relativeLuminance(system.value(AppColor::Icon));
const double primary = ThemeIconInk::relativeLuminance(system.value(AppColor::IconStrong));
const double quiet = ThemeIconInk::relativeLuminance(system.value(AppColor::IconSoft));
if (dark) {
EXPECT_GT(primary, secondary) << "dark";
EXPECT_GT(secondary, quiet) << "dark";
} else {
EXPECT_LT(primary, secondary) << "light";
EXPECT_LT(secondary, quiet) << "light";
EXPECT_FALSE(system.contains(role)) << (dark ? "dark" : "light") << static_cast<int>(role);
}
}
}