Skip to content

Make group hit badges readable when they contain selected entries - #16700

Merged
koppor merged 4 commits into
mainfrom
group-badge-contrast
Aug 28, 2026
Merged

Make group hit badges readable when they contain selected entries#16700
koppor merged 4 commits into
mainfrom
group-badge-contrast

Conversation

@koppor

@koppor koppor commented Aug 26, 2026

Copy link
Copy Markdown
Member

Summary

🤖 The hit count on a group badge becomes unreadable as soon as the group contains selected entries: the badge turns green but keeps the light badge text (≈2.6:1 contrast in the dark themes). Each theme now names a foreground for that green badge, so the count stays legible in the built-in themes and in custom ones.

Analogies: like honey, the fix is small and sticks exactly where it is needed; like chocolate, it is dark on green where that reads best and white on dark green where that does; like the moon, it is only noticed when it is missing.

jabref-contrib-policy:4.2:reviewed​:ok

Steps to test

  1. Open the Chocolate demonstration library, switch to the dark color scheme (Preferences > General > Appearance).
  2. Select an entry that belongs to several groups.
  3. The green badges in the group tree show their count in a readable dark color.

Before / after (JabRef dark theme):

group badges before and after

Related issues and pull requests

No issue; found while porting themes.jabref.org to #15798.

AI usage

Claude Code (model claude-fable-5), AIL4: implementation, screenshots and this description generated by the AI, reviewed and tested by me.

AI CHECKLIST.md walkthrough

1. Code self-review

Nullability and control flow

  • [/] No == null / != null checks — JSpecify annotations used instead. (not applicable: no nullability in the change)
  • [/] No Objects.requireNonNull(...).
  • [/] New classes annotated with @NullMarked.
  • [/] Optional consumed with ifPresent / ifPresentOrElse / map / orElseThrow.
  • [/] StringUtil.isBlank(...) used instead of s == null || s.isBlank().

Exceptions

  • [/] No catch (Exception e). (not applicable: no exception handling in the change)
  • [/] No throw new RuntimeException(...) / IllegalStateException(...).
  • [/] Logged exceptions are passed as the last logger argument.

Style and idioms

  • [/] New BibEntry objects built with withers. (not applicable, except: List.of() used for the loop)
  • Modern Java used: List.of() / Map.of() / Set.of(), Path.of(), text blocks.
  • [/] Regexes use a precompiled Pattern.compile(...) constant.
  • [/] Background work uses BackgroundTask.
  • No commented-out code, no trivial comments restating the code, no AI-disclosure comments in source.
  • [/] Markdown Javadoc (///) uses Markdown syntax.

User-facing text

  • [/] All user-facing text localized. (not applicable: no text changes)
  • [/] Sentence case; no trailing !; labels do not end with :.
  • [/] Variance expressed with placeholders.

Security

  • [/] User-controlled data is HTML-escaped before being written into any text/html response. (not applicable)

Tests

  • [/] Behavior changes in org.jabref.model / org.jabref.logic have added or updated tests. (not applicable: GUI/CSS only; ThemeTokenContractTest covers the new token)
  • [/] Tests assert object contents, use plain JUnit asserts, no @DisplayName, do not catch exceptions, use @TempDir.
  • [/] Fetcher tests hit the live endpoints.

2. Verification commands

  • [/] ./gradlew :jablib:check (not applicable: jabgui-only change; :jabgui:compileJava and ThemeTokenContractTest pass)
  • ./gradlew checkstyleMain checkstyleTest checkstyleJmh — via CI
  • ./gradlew modernizer — via CI
  • ./gradlew --no-configuration-cache :rewriteDryRun reports no changes (rewriteRun executed before commit)
  • ./gradlew javadoc — via CI
  • [/] npx markdownlint-cli2 (only if Markdown changed)
  • IntelliJ formatting applied to the changed Java file

3. Documentation

  • CHANGELOG.md entry added.
  • Searched jabref/issues and jabref-koppor/issues for a related issue; none found, PR link used.
  • [/] Requirement added to docs/requirements/<area>.md (minor fix)
  • [/] Developer documentation under docs/ updated (no architecture change)

4. Pull request

  • PR body built from .github/PULL_REQUEST_TEMPLATE.md, every section filled.
  • All checklist items kept and marked.
  • All HTML comments removed from the PR body.
  • PR created with gh pr create --body-file <file>.
  • CHANGELOG.md TODO placeholder replaced with the real PR-number link after PR creation.

Checklist

  • I own the copyright of the code submitted and I license it under the MIT license
  • If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
  • I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (if applicable)
  • I added screenshots in the PR description (if change is visible to the user)
  • I added one sentence (max 20 words) to CHANGELOG.md describing the change from the user's point of view (if the change is visible to the user)
  • [/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

🤖 Generated with Claude Code

koppor and others added 3 commits August 27, 2026 01:32
The badge text kept the normal badge foreground (white-ish in dark themes) on the
green 'contains selected entries' background, which is unreadable. A dedicated
-color-badge-selected-fg token lets each theme pick a foreground for that green.
The text node carries the pseudo-classes itself because JavaFX does not restyle
a descendant when a custom pseudo-class flips on its ancestor, and function
values (ladder/derive) are dropped for -fx-fill on this node.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NexrEs9itF7UUEbMRRu7p3
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Keep selected group hit counts readable across themes

🐞 Bug fix ⚙️ Configuration changes 📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Applies selection state to badge text so JavaFX refreshes its foreground.
• Adds theme-specific selected-badge colors for readable contrast across built-in schemes.
• Documents the group badge contrast fix in the changelog.
Diagram

graph TD
  A["Entry selection"] --> B["Group match state"] --> C{"Match status"}
  C -->|Any selected| D["Badge pseudo-classes"] --> E["Selected text rule"] --> F["Theme foreground"]
  C -->|All selected| D
Loading
High-Level Assessment

The tokenized foreground plus mirrored text pseudo-classes is the appropriate approach. Ancestor-only pseudo-classes do not trigger JavaFX descendant restyling, while computed ladder or derive fills are unreliable on this text node; direct hard-coded styling would also undermine theme customization.

Files changed (5) +23 / -5

Bug fix (2) +14 / -5
GroupTreeView.javaPropagate selection pseudo-classes to badge text +9/-5

Propagate selection pseudo-classes to badge text

• Creates the hit-count text before configuring selection state and applies 'any-selected' and 'all-selected' pseudo-classes to both the container and text. This ensures JavaFX restyles the foreground when group match state changes.

jabgui/src/main/java/org/jabref/gui/groups/GroupTreeView.java

jabref-base.cssStyle selected badge text with a dedicated token +5/-0

Style selected badge text with a dedicated token

• Adds selectors for both selected-match pseudo-classes and resolves their text fill through '-color-badge-selected-fg'.

jabgui/src/main/resources/org/jabref/gui/theme/internal/jabref-base.css

Documentation (1) +1 / -0
CHANGELOG.mdDocument readable selected group badge counts +1/-0

Document readable selected group badge counts

• Adds a user-facing fixed entry for the group hit-count contrast issue and links PR #16700.

CHANGELOG.md

Other (2) +8 / -0
jabref-theme.cssDefine selected badge foregrounds for JabRef schemes +4/-0

Define selected badge foregrounds for JabRef schemes

• Declares a black selected-badge foreground in both light and dark JabRef color schemes for contrast against their green badge backgrounds.

jabgui/src/main/resources/org/jabref/gui/theme/jabref-theme.css

primer-theme.cssDefine selected badge foregrounds for Primer schemes +4/-0

Define selected badge foregrounds for Primer schemes

• Declares white text for the Primer light scheme's darker green badge and black text for the dark scheme's lighter green badge.

jabgui/src/main/resources/org/jabref/gui/theme/primer-theme.css

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Selected badge behavior untested 📘 Rule violation ☼ Reliability
Description
The new pseudo-class propagation and selected badge foreground have no behavioral test; existing
theme tests only validate token declaration and CSS parsing. A regression could therefore restore
unreadable counts while the token contract still passes.
Code

jabgui/src/main/java/org/jabref/gui/groups/GroupTreeView.java[R371-373]

+            for (Node styled : List.of(node, text)) {
+                BindingsHelper.includePseudoClassWhen(styled, PSEUDOCLASS_ANYSELECTED,
+                        group.anySelectedEntriesMatchedProperty());
Evidence
Compliance rule 24 requires relevant deterministic coverage for observable behavior changes. The
changed Java lines propagate selected-state pseudo-classes to the text and the new CSS consumes
them, but ThemeTokenContractTest only compares token declarations and uses, so it cannot detect
failure of the propagation or computed foreground behavior.

AGENTS.md: Add or Update Deterministic Tests for Behavior Changes
jabgui/src/main/java/org/jabref/gui/groups/GroupTreeView.java[364-377]
jabgui/src/main/resources/org/jabref/gui/theme/internal/jabref-base.css[1610-1621]
jabgui/src/test/java/org/jabref/gui/theme/ThemeTokenContractTest.java[156-173]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Add deterministic JavaFX coverage for the selected group badge behavior introduced by this PR.

## Issue Context
Verify that selection-property changes apply `any-selected` and `all-selected` to the badge `Text`, and that the selected foreground rule is applied. The existing token contract test checks declarations and parsing only.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/groups/GroupTreeView.java[371-375]
- jabgui/src/main/resources/org/jabref/gui/theme/internal/jabref-base.css[1618-1620]
- jabgui/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java[34-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +371 to +373
for (Node styled : List.of(node, text)) {
BindingsHelper.includePseudoClassWhen(styled, PSEUDOCLASS_ANYSELECTED,
group.anySelectedEntriesMatchedProperty());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. Selected badge behavior untested 📘 Rule violation ☼ Reliability

The new pseudo-class propagation and selected badge foreground have no behavioral test; existing
theme tests only validate token declaration and CSS parsing. A regression could therefore restore
unreadable counts while the token contract still passes.
Agent Prompt
## Issue description
Add deterministic JavaFX coverage for the selected group badge behavior introduced by this PR.

## Issue Context
Verify that selection-property changes apply `any-selected` and `all-selected` to the badge `Text`, and that the selected foreground rule is applied. The existing token contract test checks declarations and parsing only.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/groups/GroupTreeView.java[371-375]
- jabgui/src/main/resources/org/jabref/gui/theme/internal/jabref-base.css[1618-1620]
- jabgui/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java[34-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

subhramit
subhramit previously approved these changes Aug 26, 2026
koppor added a commit to JabRef/themes.jabref.org that referenced this pull request Aug 26, 2026
@koppor koppor added the automerge PR is tagged with that label will be merged if workflows are green label Aug 28, 2026
@koppor
koppor enabled auto-merge August 28, 2026 03:08
@koppor

koppor commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

"automerge", because I only resoled conflicts after the approval.

@koppor
koppor added this pull request to the merge queue Aug 28, 2026
@github-actions github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Aug 28, 2026
Merged via the queue into main with commit 9e141e0 Aug 28, 2026
72 of 82 checks passed
@koppor
koppor deleted the group-badge-contrast branch August 28, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge PR is tagged with that label will be merged if workflows are green component: css component: groups component: theming component: ui status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants