Move custom-tab fields off the Main tab - #16792
Conversation
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
PR Summary by QodoKeep custom-tab fields off the Main entry editor tab
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
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
|
Don't think this pr is a good idea. User expects main to show all. And for specialized work he prepares his custom tabs. |
|
If this requires a preference, so be it. 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. |
|
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. |
|
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". |
|
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. 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. 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. |
…ates # Conflicts: # docs/requirements/entry-editor.md
|
🤖 Generated with Claude Code Merged Only Docs-only change; CI is the compile check. Generated by Claude Code |
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:okAnalogies: 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
authoranddoi.Main tab (Author/DOI moved away):
Custom tab:
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
== null/!= nullchecks — JSpecify annotations (@NullMarked,@Nullable,@NonNull) used instead.Objects.requireNonNull(...)— nullability expressed via JSpecify annotations.@NullMarked(org.jspecify.annotations.NullMarked).Optionalconsumed withifPresent/ifPresentOrElse/map/orElseThrow— neverorElse(unusedValue)nor anisPresent()+get()block.StringUtil.isBlank(...)used instead ofs == null || s.isBlank().Exceptions
catch (Exception e)— only specific exceptions are caught.throw new RuntimeException(...)/IllegalStateException(...)— these tear down the whole application.LOGGER.info("...", e)), not concatenated into the message string.Style and idioms
BibEntryobjects built with withers (withField, notsetField).List.of()/Map.of()/Set.of(),Path.of(),SequencedCollection/SequencedSet, text blocks.Pattern.compile(...)constant, notString.matches(...).org.jabref.logic.util.BackgroundTask, notnew Thread().///) uses Markdown syntax, not JavaDoc inline tags:`code`instead of{@code},[ClassName]instead of{@link}.User-facing text
Localization.langin Java,%prefix in FXML).!; labels do not end with:."...: %0"), not string concatenation.Security
text/htmlresponse — including exception/error messages, not just the success body (XSS).Tests
org.jabref.model/org.jabref.logichave added or updated tests.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@TempDirinstead of manual temp directories.Verification commands
./gradlew :jabgui:test --tests EntryEditorTabModelTest(change is jabgui-only; jablib untouched)../gradlew :jabgui:checkstyleMain :jabgui:checkstyleTest../gradlew modernizer../gradlew rewriteRunrun before committing../gradlew javadoc.npx markdownlint-cli2on the changed Markdown file.Documentation
CHANGELOG.mdentry — not applicable: custom tabs are unreleased (introduced after the last release in Reintroduce custom entry editor tabs with two-column preferences UI #16598).req~entry-editor.custom-tabs~1updated indocs/requirements/entry-editor.md.docs/updated if behavior or architecture changed.Pull request
.github/PULL_REQUEST_TEMPLATE.md, every section filled.gh pr create --body-file <file>.TODOplaceholder replacement — no CHANGELOG entry.Checklist
CHANGELOG.mddescribing the change from the user's point of view (feature is unreleased, no entry per policy)🤖 Generated with Claude Code