Skip to content

Apply an accepted external entry change in place, so the entry editor updates - #17363

Draft
koppor wants to merge 4 commits into
mainfrom
fix-entrychange-in-place
Draft

koppor wants to merge 4 commits into
mainfrom
fix-entrychange-in-place

Conversation

@koppor

@koppor koppor commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Accepting a change to an entry in the "External changes" dialog removed the entry and inserted the version from disk as a new entry. An open entry editor kept showing the removed entry, so it did not update, and edits made in it afterwards were lost on save. The table selection and position were lost too. Now the existing entry is changed in place: type, fields and the comments before the entry are updated as undoable steps, so the entry keeps its identity.

Undo also restores the entry's "changed" flag. Without that, an undone merge would still cause the entry to be re-serialized on the next save instead of being written back as it was read.

This was carved out of #16813, which depends on it. The behaviour on its own is not tied to file synchronization, and the split keeps that PR smaller.

Analogies: This PR is like honey: it stays where it is put instead of being scooped out and poured back in. Like chocolate, it should melt into the existing dialog without changing its shape. And like the moon, the entry keeps showing the same face to the editor, however much changes around it.

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

Steps to test

  1. Copy jablib/src/main/resources/Chocolate.bib to a temporary folder and open the copy in JabRef.

  2. Double-click Corti_2009 to open the entry editor.

  3. In a text editor, change its title to Cocoa and Cardiovascular Health: Changed in an External Editor and save the file.

  4. In JabRef, click the notification "External changes detected" and then "Review changes".

  5. In the "External Changes Resolver", select "Modified entry 'Corti_2009'" and click "Accept".

  6. The entry editor shows the new title, and Corti_2009 stays selected in the table.

    On main: the entry editor still shows the old title, Corti_2009 moved away from the top of the table, and the selection jumped to Cooper_2007:

    On main: entry editor keeps the old title, selection moved

    With this PR:

    With this PR: entry editor shows the accepted title, selection kept

  7. Click "Undo" in the toolbar (or press Ctrl + Z in the table): the title goes back to Cocoa and Cardiovascular Health in the table and in the entry editor.

Related issues and pull requests

Closes #8901

Split off from #16813

AI usage

Claude Code (model claude-opus-5-5), AIL4: the code was written by the AI under my direction and reviewed by me.

AI CHECKLIST.md walkthrough

1. Code self-review

Nullability and control flow

  • No == null / != null checks — JSpecify annotations (@NullMarked, @Nullable, @NonNull) used instead. (The one != null in UndoableChangedFlag reads its own @Nullable record component.)
  • No Objects.requireNonNull(...) — nullability expressed via JSpecify annotations.
  • New classes annotated with @NullMarked (org.jspecify.annotations.NullMarked).
  • [/] New packages annotated with @NullMarked (org.jspecify.annotations.NullMarked).
  • Optional consumed with ifPresent / ifPresentOrElse / map / orElseThrow — never orElse(unusedValue) nor an isPresent() + get() block. (orElse(null) only where UndoableFieldChange takes @Nullable values.)
  • [/] 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(...).
  • Keep alphabetical ordering when adding variables or enum items. (The permits list is grouped by kind; the new types sit with the other entry changes.)
  • [/] 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

  • In case issue lists an example, this concrete example is covered by a test case. (EntryChangeTest covers the case with its own entries: the open entry stays the instance in the library and takes over the disk content.)
  • 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).

2. Verification commands

  • ./gradlew :jablib:check (or ./gradlew check for all modules). (Stopped after the jablib tests had run for several minutes without a failure; CI runs the full suite.)
  • ./gradlew checkstyleMain checkstyleTest checkstyleJmh.
  • ./gradlew modernizer.
  • ./gradlew --no-configuration-cache :rewriteDryRun reports no changes (run ./gradlew rewriteRun to fix).
  • ./gradlew javadoc.
  • [/] npx markdownlint-cli2 "docs/**/*.md" "*.md" (only if Markdown changed).
  • [/] npm ci && npm run textlint reports no misspellings (only if Markdown changed).
  • [/] Only if formatting is still off after rewriteRun: docker run -v $(pwd):/github/workspace ghcr.io/leventebajczi/intellij-format:master "*.java" "" ".idea/codeStyles/Project.xml".

3. Documentation

  • CHANGELOG.md entry 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. Use TODO as 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.
  • Searched jabref/issues and jabref-koppor/issues for a related issue; linked only on a confident match, otherwise kept TODO (no closes/fixes for merely-similar issues).
  • Requirement added to docs/requirements/<area>.md if the change is a new feature or significant bug fix (skip for refactors, minor fixes, and internal changes). (req~ux.external-library-changes.entry-in-place~1 in docs/requirements/ux.md.)
  • [/] Developer documentation under docs/ updated if behavior or architecture changed.

4. Pull request

  • PR body built from .github/PULL_REQUEST_TEMPLATE.md, every section filled.
  • All checklist items kept and marked [x], [ ], or [/].
  • "Steps to test" is a numbered list with a cropped screenshot of the result for every visible change; no video (only allowed when another program is involved, e.g. drag and drop or push to an external application).
  • All HTML comments removed from the PR body.
  • PR created with gh pr create --body-file <file> (not --body).
  • [/] If CHANGELOG.md used a TODO placeholder (no issue confidently identified yet — an existing issue link always stays), the PR was opened as draft, the placeholder was replaced with the real PR-number link after PR creation, committed and pushed, and only then was the PR marked ready for review. If an issue is identified or created later, the link is switched to the issue.

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 October 3, 2026 23:33
Removing the entry and inserting the disk version gave it a new identity,
so an open entry editor kept showing the removed entry. The fields, type,
and comments of the existing entry are now changed instead, recorded as
undoable steps, together with the entry's changed flag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jabref-machine jabref-machine added the dev: into-stable Port this pull request to the stable branch after merging (ADR-0072) label Oct 3, 2026
@github-actions github-actions Bot added the status: changes-required Pull requests that are not yet complete label Oct 3, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the status: changes-required Pull requests that are not yet complete label Oct 3, 2026

This branch has not been deployed

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

Labels

component: external-changes dev: into-stable Port this pull request to the stable branch after merging (ADR-0072) size: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accept changes should also update the entry editor

2 participants