Skip to content

Move custom-tab fields off the Main tab - #16792

Open
koppor wants to merge 3 commits into
mainfrom
custom-tabs-no-duplicates
Open

Move custom-tab fields off the Main tab#16792
koppor wants to merge 3 commits into
mainfrom
custom-tabs-no-duplicates

Conversation

@koppor

@koppor koppor commented Aug 31, 2026

Copy link
Copy Markdown
Member

Summary

🤖 A field placed on a user-defined entry editor tab was still displayed on the Main tab as well. The Main tab now leaves such fields to their custom tab — it shows neither an editor row nor an add button for them — so no field appears twice.

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

Analogies: Like honey, this change flows every field into exactly one cell of the comb; like chocolate, it is best not split into two identical halves; and like the moon, each field now shows only one face at a time.

Steps to test

  1. In File → Preferences → Entry editor → Editor tabs, add a custom tab "My tab" with the fields author and doi.
  2. Open an entry that has author and DOI set — the Main tab no longer shows Author or DOI, and offers no add button for them.
  3. "My tab" shows the Author and DOI editors.

Main tab (Author/DOI moved away):

Main tab without author/doi

Custom tab:

Custom tab with author/doi

Related issues and pull requests

Follow-up to #16598; user documentation updated in JabRef/user-documentation#659.

AI usage

Claude Code (model claude-fable-5), AIL4 — task specified and result reviewed by the contributor.

AI CHECKLIST.md walkthrough

Nullability and control flow

  • No == null / != null checks — JSpecify annotations (@NullMarked, @Nullable, @NonNull) used instead.
  • No Objects.requireNonNull(...) — nullability expressed via JSpecify annotations.
  • [/] New classes annotated with @NullMarked (org.jspecify.annotations.NullMarked).
  • Optional consumed with ifPresent / ifPresentOrElse / map / orElseThrow — never orElse(unusedValue) nor an isPresent() + get() block.
  • StringUtil.isBlank(...) used instead of s == null || s.isBlank().

Exceptions

  • No catch (Exception e) — only specific exceptions are caught.
  • No throw new RuntimeException(...) / IllegalStateException(...) — these tear down the whole application.
  • [/] Logged exceptions are passed as the last logger argument (LOGGER.info("...", e)), not concatenated into the message string.

Style and idioms

  • New BibEntry objects built with withers (withField, not setField).
  • Modern Java used: List.of() / Map.of() / Set.of(), Path.of(), SequencedCollection / SequencedSet, text blocks.
  • [/] Regexes use a precompiled Pattern.compile(...) constant, not String.matches(...).
  • [/] Background work uses org.jabref.logic.util.BackgroundTask, not new Thread().
  • No commented-out code, no trivial comments restating the code, no AI-disclosure comments in source.
  • Markdown Javadoc (///) uses Markdown syntax, not JavaDoc inline tags: `code` instead of {@code}, [ClassName] instead of {@link}.

User-facing text

  • [/] All user-facing text localized (Localization.lang in Java, % prefix in FXML).
  • [/] Sentence case (not Title Case); no trailing !; labels do not end with :.
  • [/] Variance expressed with placeholders ("...: %0"), not string concatenation.

Security

  • [/] User-controlled data (request params, entry fields, file contents) is HTML-escaped before being written into any text/html response — including exception/error messages, not just the success body (XSS).

Tests

  • Behavior changes in org.jabref.model / org.jabref.logic have added or updated tests.
  • Tests assert object contents (assertEquals), use plain JUnit asserts (not AssertJ), have no @DisplayName, do not catch exceptions (let them propagate so JUnit reports setup/teardown failures directly), and use @TempDir instead of manual temp directories.
  • [/] Fetcher tests hit the live endpoints — the remote API is not mocked or stubbed (automated-review suggestions to mock it are rejected on purpose).

Verification commands

  • ./gradlew :jabgui:test --tests EntryEditorTabModelTest (change is jabgui-only; jablib untouched).
  • ./gradlew :jabgui:checkstyleMain :jabgui:checkstyleTest.
  • [/] ./gradlew modernizer.
  • ./gradlew rewriteRun run before committing.
  • [/] ./gradlew javadoc.
  • npx markdownlint-cli2 on the changed Markdown file.
  • Touched files formatted with the project IntelliJ code style.

Documentation

  • [/] CHANGELOG.md entry — not applicable: custom tabs are unreleased (introduced after the last release in Reintroduce custom entry editor tabs with two-column preferences UI #16598).
  • Searched for a related issue; none exists — the request came from review feedback on the documentation PR.
  • Requirement req~entry-editor.custom-tabs~1 updated in docs/requirements/entry-editor.md.
  • [/] Developer documentation under docs/ updated if behavior or architecture changed.

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>.
  • [/] TODO placeholder replacement — no CHANGELOG entry.

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 (feature is unreleased, no entry per policy)
  • I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

🤖 Generated with Claude Code

A field shown on a user-defined entry editor tab was also displayed on
the Main tab. The Main tab now excludes fields resolved by custom tabs
from its list and its add-chips, so no field is displayed twice.

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

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Keep custom-tab fields off the Main entry editor tab

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Move custom-tab fields out of Main tab editor rows and add controls.
• Centralize custom-tab field resolution for explicit names and regular-expression patterns.
• Document and test single-tab field ownership behavior.
Diagram

graph TD
  P["Editor Preferences"] -->|provides tabs| M["Tab Models"] -->|resolves patterns| R["Custom Fields"]
  R -->|excluded from| A["Main Tab"]
  R -->|displayed by| C["Custom Tabs"]
Loading
High-Level Assessment

The current approach is appropriate: field-pattern resolution remains centralized in the tab model, while the Main tab consumes the aggregate set at each place that can expose a field. Duplicating pattern interpretation inside AllFieldsTab or changing custom-tab rendering would increase coupling without improving behavior.

Files changed (4) +45 / -3

Bug fix (2) +31 / -2
AllFieldsTab.javaExclude custom-tab fields from Main tab controls +18/-2

Exclude custom-tab fields from Main tab controls

• Removes fields resolved by custom tabs from the Main tab's displayed field set and both section-level and main add-chip lists. Newly user-added fields remain temporarily visible to preserve focus while editing.

jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java

EntryEditorTabModel.javaAggregate fields resolved across custom tabs +13/-0

Aggregate fields resolved across custom tabs

• Adds a shared helper that unions resolved fields from all customized field tabs while ignoring built-in tabs. This gives Main-tab rendering a single source for custom-tab field ownership.

jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditorTabModel.java

Tests (1) +13 / -0
EntryEditorTabModelTest.javaTest custom-tab field aggregation +13/-0

Test custom-tab field aggregation

• Verifies aggregation across multiple custom tabs, including regex-resolved fields, and confirms built-in tabs do not contribute fields.

jabgui/src/test/java/org/jabref/gui/entryeditor/EntryEditorTabModelTest.java

Documentation (1) +1 / -1
entry-editor.mdSpecify exclusive ownership of custom-tab fields +1/-1

Specify exclusive ownership of custom-tab fields

• Clarifies that fields resolved onto custom tabs are moved there. The Main tab must omit both their editor rows and add chips.

docs/requirements/entry-editor.md

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

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. userAddedFields permits duplicates ✓ Resolved 📘 Rule violation ≡ Correctness
Description
When a Main-tab chip adds an unset field, userAddedFields retains it after typed input makes it
match a custom-tab regex because the field is removed from Main and then immediately re-added. The
field consequently appears on both Main and the custom tab until another entry is opened,
contradicting the PR's single-tab behavior and the requirement that Main show neither an editor nor
an add-chip for fields displayed on a custom tab.
Code

jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[R194-195]

+        fields.removeAll(customTabFields(entry));
      fields.addAll(userAddedFields);
Evidence
Chip selection records the field in userAddedFields, and field changes recompute the displayed
set. The changed implementation removes fields resolved onto custom tabs but immediately re-adds
every member of userAddedFields; the adjacent comment confirms this includes newly regex-matched
fields, while custom tabs independently render their resolved fields and the retained state is
cleared only when the current entry changes. This demonstrates duplicated behavior contrary to
compliance rule 3 and the updated requirement that a custom-tab field be moved off Main.

AGENTS.md: Preserve Architecture, Compatibility, and Change Scope: AGENTS.md: Preserve Architecture, Compatibility, and Change Scope: AGENTS.md: Preserve Architecture, Compatibility, and Change Scope: AGENTS.md: Preserve Architecture, Compatibility, and Change Scope
docs/requirements/entry-editor.md[78-83]
jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[190-195]
jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditorTabModel.java[139-160]
jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[163-168]
jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[246-262]
jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[582-587]
jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditorTabModel.java[139-154]
jabgui/src/main/java/org/jabref/gui/entryeditor/UserDefinedFieldsTab.java[65-77]

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

## Issue description
A field added through a Main-tab chip remains in `userAddedFields` after its value starts matching a custom-tab regex. The field is therefore displayed on both Main and the custom tab until the selected entry changes.
## Issue Context
The updated requirement says fields shown on custom tabs must have neither an editor nor an add-chip on Main. `determineFieldsToShow` removes resolved custom-tab fields and then re-adds all `userAddedFields`; `showFieldEditor` adds chip-selected fields to that set, which is only cleared when another entry is opened. Preserve focus safely only while the field is not resolved onto a custom tab, and add regression coverage for the transition after the first matching character is entered.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[190-195]
- jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[246-262]
- jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java[582-587]
- jabgui/src/test/java/org/jabref/gui/entryeditor/EntryEditorTabModelTest.java[58-67]

ⓘ 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 turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread jabgui/src/main/java/org/jabref/gui/entryeditor/AllFieldsTab.java Outdated
A field added via a Main-tab chip stayed in the user-added set after its
value started matching a custom-tab regex, so it was shown on both tabs
until another entry was opened.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RCXhMnmDQnq6tpqZncPC2i
@calixtus

calixtus commented Sep 1, 2026

Copy link
Copy Markdown
Member

Don't think this pr is a good idea. User expects main to show all. And for specialized work he prepares his custom tabs.
Fields should not be removed from main tab IMHO. At least make it a pref.

@ThiloteE

ThiloteE commented Sep 1, 2026

Copy link
Copy Markdown
Member

If this requires a preference, so be it.
I think it's a good change, because some fields hold a lot of text, such as the "comment" or "abstract" fields, so some users will want to keep the content of those fields outside the main tab to improve readability.
If we were to store AI Chat and AI Summary in a field as well, the main tab would explode.

By the way, this is the decision I was asking for us to take yesterday during the dev-call when we were all present. This is the very thing I was referring to and you refused and said you don't understand, and you don't have the context and Ryan and me should talk during the JabCon about this, because we need good entry editor design and now you open the PR, so you did understand me after all. I am confused why we didn't take the decision yesterday and now we have to type up a lot of text to make our arguments.

@calixtus

calixtus commented Sep 1, 2026

Copy link
Copy Markdown
Member

You are right, i asked for context, bc all you did was posting three screenshots. We write arguments and decisions down, so we can understand them if we (or others) see them after years or from outside. Dev-Call is a closed format, JabRef is open source. So please calm down and do not lecture me. Also koppor opened the PR, not me.

And I think its a change for the worse, because it now breaks the basic design premise of the main tab: One tab holds it all. Like all-fields-tab. There is no all-fields-tab-except-those-that-we-want-to-put-in-a-separate-tab-bc-we-dont-like-the-premise-tab. These are incompatible design decisions: Removing inputs fields from the ALL-FIELDS tabs is imho a NO-GO. If the abstract field holds too much information, close the accordion container. But maybe we have to go bigger on this: Introduce a separate legacy entry editor that can deprecate and can be removed without grater difficulties, when we don't want to maintain it anymore, and the new one, where we can move towards a better design.

@ThiloteE

ThiloteE commented Sep 1, 2026

Copy link
Copy Markdown
Member

But I was talking to koppor, not you Carl. The discussion about this topic happened before you joined the dev-call. So we were almost all there, except you.

And my screenshots in that other PR were only trying to show how that PR looked like for me. I was not trying to make any arguments. I didn't have any time, so i gave what I could, because Koppor asked me to try the PR. It was a simple "Screenshots before vs Screenshots after" situation. That's all. And if I remember right, there was no change. So maybe I should have written "The changes you introduced in this PR didn't change anything for me since I last commented and shared in screenshots, as you can see in the new screenshots".

@ThiloteE

ThiloteE commented Sep 1, 2026

Copy link
Copy Markdown
Member

Overall, I think I very much agree with you Carl, that this is a design choice. We have two incompatible designs and we need to choose which one.

In the short run, we only need to decide about a main vs custom tabs.
In the long run, we might transition to a different entry editor design that makes better use of wide screens or multi-screen space, but that's a problem for another day.

I will play the devils advocate for the short-run scenario:

Removing inputs fields from an ALL-FIELDS tab is a NO-GO, because it would betray user expectation. Yes, that's sensible, but somebody has called the main tab a "main" tab, not an "All-Fields" tab and we clearly have not yet established a stable definition of what that tab is supposed to be like and as long as we are still in the alpha/beta phase, we have room to change our minds.

If really all fields are shown to the user in one tab is debatable and we have the choice for maintainers to make the decision for the user or we have the choice to enable users to make this decision for themselves.

In the background, JabRef clearly needs to be aware of all fields "somewhere". So the real theoretical choice is actually between:

A) ALL fields in background + All fields shown to user in "All-fields" tab + Duplicate fields in "Custom" tab.
B) ALL fields in background + All fields shown to user in "Main" tab (= ALL fields minus the fields in "Custom" tab) + Fields only in "Custom" tab.

and we have a complicated third option that tries to make the best of both worlds, but that ends up with more tabs, (which is what we wanted to move away from, so this is a bad option):

C) ALL fields in background + ALL fields shown to user in "All-fields" tab + "Main" tab (= ALL fields minus the fields in "custom" tabs) + Fields only in "Custom" tab.

Technically, this probably could be achieved with some kind of filter?

Making a "Legacy" entry editor only postpones this decision to the future and we would have the same discussion again, so better let's settle it now.

@github-actions github-actions Bot added the status: changes-required Pull requests that are not yet complete label Sep 2, 2026
…ates

# Conflicts:
#	docs/requirements/entry-editor.md

koppor commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

🤖 Generated with Claude Code

Merged main into this branch to resolve the merge conflict.

Only docs/requirements/entry-editor.md conflicted, in req~entry-editor.custom-tabs~1: this PR added the sentence about custom-tab fields being moved off the "Main" tab, while main rewrote the trailing migration sentence (dropping stored former-default tabs on upgrade). Kept both — the PR's new sentence followed by main's updated migration rule.

Docs-only change; CI is the compile check.


Generated by Claude Code

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

Labels

component: entry-editor status: changes-required Pull requests that are not yet complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants