Add per-library keyword separator - #16835
Conversation
The separator is stored in the library's metadata and editable in the General tab of the library properties. Libraries without a declared separator keep the one their keyword fields already use, so opening a library no longer rewrites every keyword field to the global preference. Fixes JabRef/jabref-issue-melting-pot#1353. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
github-actions
Bot
added
component: ui
component: search
component: groups
component: citationkey-generator
component: import-load
component: export-or-save
component: entry-editor
component: diff-merge
PR Summary by QodoAdd per-library keyword separator handling
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
jabref-machine
added
the
status: changes-required
GroupsEditor now receives BibEntryPreferences via its constructor instead of pulling CliPreferences from the Injector (constructor-based injection per ADR). Also fixes checkstyle findings in the branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NmDLSsG1427ps5rztJXHT
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
…nto keyword-separator-per-library # Conflicts: # jabgui/src/main/java/org/jabref/gui/fieldeditors/GroupsEditor.java
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
…arator input - guessSeparator now counts only top-level, unescaped delimiters outside braces, using the same scan as KeywordList parsing - Library properties keyword separator field is limited to one character - Shorten changelog entry Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NmDLSsG1427ps5rztJXHT
…nto keyword-separator-per-library
jabref-machine
added
status: ready-for-review
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
The rewrite of keyword fields to the library's separator is now a field formatter, part of the default save actions and the cleanup dialog, and bound to the library's keyword separator on save and cleanup. Load applies the same formatter for in-memory consistency; ADR 0071 records the trade-off. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zp2dm93rwWgk6KAy2nm1D
| int startLine = line; | ||
| int startColumn = column; | ||
| try { | ||
| // A library without a declared separator keeps the one its keyword fields already use, so opening it does not rewrite them |
There was a problem hiding this comment.
🔴 bug: BibDesk static-group migration has already created/appended groups with the global separator (and a hard-coded comma) before this effective separator is known. Defer that migration until after metadata parsing and use the resulting separator.
| librarySpecificDirectoryProperty.setValue(metaData.getLibrarySpecificFileDirectory().orElse("").trim()); | ||
| userSpecificFileDirectoryProperty.setValue(metaData.getUserFileDirectory(preferences.getFilePreferences().getUserAndHost()).orElse("").trim()); | ||
| laTexFileDirectoryProperty.setValue(metaData.getLatexFileDirectory(preferences.getFilePreferences().getUserAndHost()).map(Path::toString).orElse("")); | ||
| keywordSeparatorProperty.setValue(databaseContext.getKeywordSeparator(preferences.getBibEntryPreferences().getKeywordSeparator()).toString()); |
There was a problem hiding this comment.
🟡 risk: This displays the effective global fallback even when metadata has no override; storeSettings then writes it as an explicit library setting. Opening Library Properties and saving an unrelated option therefore freezes future global changes. Bind the field to metaData.getKeywordSeparator() (blank when absent).
| Character separator = Optional.ofNullable(keywordSeparator) | ||
| .or(() -> Optional.ofNullable(preferences.getKeywordSeparator())) | ||
| .orElse(BibEntryPreferences.getDefault().getKeywordSeparator()); | ||
| List<Character> delimiters = KeywordImportNormalizer.parseConfiguredDelimiters(preferences.getImportKeywordDelimiters()); |
There was a problem hiding this comment.
🔴 bug: If the accepted import delimiters exclude the library separator, this parses an already-valid field with the wrong delimiter. For example, import delimiter , plus library separator ; turns a;b into a\\;b. Include the separator in delimiters (or short-circuit a valid field) and add a regression test.
Siedlerchr
left a comment
Siedlerchr
left a comment
There was a problem hiding this comment.
Additional findings in unchanged files (GitHub cannot anchor inline comments outside the PR diff):
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java:L64: 🔴 bug: batch fetched-data merges still pass the global separator. A semicolon library’sgroupsfield can become mixed (one; two, three). Pass the active database’s effective separator.jabgui/src/main/java/org/jabref/gui/pseudonymize/PseudonymizeAction.java:L51: 🟡 risk: pseudonymization still parses and writes group memberships using the global separator. Use the source library’s effective separator and preserve it in the generated context metadata.
| } | ||
|
|
||
| @Override | ||
| public String format(@NonNull String value) { |
There was a problem hiding this comment.
🟡 risk: This formatter runs only on import, cleanup, or save actions; changing the separator in Library Properties merely updates metadata, so existing keyword fields are never migrated. Existing keyword-backed groups also retain their captured separator. Trigger an explicit, undoable migration when the setting changes, covering both keywords and groups.
|
will fix it |
Defer static-group migration until metadata determines the effective separator.
Keep fields and group definitions consistent in one undo operation.
Use the effective separator and retain explicit overrides in pseudonymized libraries.
|
Addressed all outstanding feedback: BibDesk now uses the effective separator; library settings retain absent overrides and migrate fields/groups undoably; formatter, merge, and pseudonymization honor the library separator. Focused regression tests pass. |
github-actions
Bot
added
the
status: to-be-merged
origin/keyword-separator-per-library (JabRef#16835) and origin/show-diff-button (JabRef#16832) no longer exist on either remote; last night's run warned about both. Upstream has landed the keyword-separator work itself, so experimental picks it up via origin/main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPHwSBqteEYM7E2QgWhk8U
…16728 * upstream/main: Document improve AI Usage Policy (JabRef#16852) Fix openfasttrace buildtime concurrency issues (JabRef#16854) Render the JabCon gource video every 15 minutes Add "Show diff" to the save-before-closing dialog (JabRef#16832) Exclude Kotlin scripts from IDEA formatting (JabRef#16790) Support modifier keys entry drag drop (JabRef#16286) Chore(deps): Bump jablib/src/main/resources/csl-styles from `0819c0e` to `db768d4` (JabRef#16820) Add per-library keyword separator (JabRef#16835) Improve logging to find out which linked file has a flaw (JabRef#15680) (JabRef#16702) Fix accented arXiv title searches (JabRef#16825) Show progress indicator during identifier lookup in New Entry dialog (JabRef#16795) Open online file links via URL-aware browser call (JabRef#16774) Chore(deps): Bump org.controlsfx:controlsfx in /versions (JabRef#16847) Keep daytime continuous in the JabCon gource video
* main: Fix `missing$` case warnings in BST VM (JabRef#16851) Start the JabCon gource video at the JabCon start time Add highlight words mode to Git Diff view (JabRef#16742) Document improve AI Usage Policy (JabRef#16852) Fix openfasttrace buildtime concurrency issues (JabRef#16854) Render the JabCon gource video every 15 minutes Add "Show diff" to the save-before-closing dialog (JabRef#16832) Exclude Kotlin scripts from IDEA formatting (JabRef#16790) Support modifier keys entry drag drop (JabRef#16286) Chore(deps): Bump jablib/src/main/resources/csl-styles from `0819c0e` to `db768d4` (JabRef#16820) Add per-library keyword separator (JabRef#16835)
3 participants
Summary
🤖 Opening a library whose keywords use a different separator than the global preference silently rewrote every keyword field. The keyword separator is now a per-library setting in the library properties ("General" tab); a library that declares none keeps the separator its keyword fields already use, and only falls back to the global preference when no keywords contain a separator. The rewrite itself is now a "Normalize keyword delimiters" cleanup, part of the default save actions and the cleanup dialog, so it is visible and can be disabled per library (ADR 0071).
jabref-contrib-policy:4.2:reviewed:okAnalogies: Like honey, the library's own separator sticks around instead of being washed away. Like chocolate, one small setting in the properties dialog is enough. Like the moon, the global preference is still there, just no longer overriding what the library has.
Steps to test
,.;(e.g.keywords = {a;b;c}) and add an entry.keywordSeparatormetadata line is added.;.,, apply, reopen the library: the keyword fields are rewritten to,.Related issues and pull requests
Closes https://github.com/JabRef/jabref-issue-melting-pot/issues/1353
Contributes to #8701
User documentation: JabRef/user-documentation#676
AI usage
Claude Code (model claude-fable-5-1), AIL4 (AI wrote the code and tests, reviewed and tested by the author).
AI CHECKLIST.md walkthrough
1. Code self-review
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.2. Verification commands
./gradlew :jablib:check(or./gradlew checkfor all modules)../gradlew checkstyleMain checkstyleTest checkstyleJmh../gradlew modernizer../gradlew --no-configuration-cache :rewriteDryRunreports no changes (run./gradlew rewriteRunto fix)../gradlew javadoc.npx markdownlint-cli2 "docs/**/*.md" "*.md"(only if Markdown changed).rewriteRun:docker run -v $(pwd):/github/workspace ghcr.io/leventebajczi/intellij-format:master "*.java" "" ".idea/codeStyles/Project.xml".3. Documentation
CHANGELOG.mdentry added if the change is visible to the user (end-user wording, no extra blank lines, sorted in next to existing entries about the same component/feature). Link the issue if one exists; link the PR only when no issue exists. UseTODOas the placeholder when neither is known yet — never a fake number. No entry for fixes to changes that were themselves introduced after the last release (feature only in## [Unreleased]) — update the existing unreleased entry instead if needed.TODO(nocloses/fixesfor merely-similar issues).docs/requirements/<area>.mdif the change is a new feature or significant bug fix (skip for refactors, minor fixes, and internal changes).docs/updated if behavior or architecture changed.4. Pull request
.github/PULL_REQUEST_TEMPLATE.md, every section filled.[x],[ ], or[/].gh pr create --body-file <file>(not--body).CHANGELOG.mdused aTODOplaceholder (no issue confidently identified yet — an existing issue link always stays), it was replaced with the real PR-number link after PR creation, then committed and pushed. If an issue is identified or created later, the link is switched to the issue.Checklist
CHANGELOG.mddescribing the change from the user's point of view (if the change is visible to the user)🤖 Generated with Claude Code