Fix merge dialog overwriting user's citation key choice during duplicate import

CHANGELOG.md

@@ -71,6 +71,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We fixed an issue where a tab or the tab bar would not show, while the setting "hide tab bar when a single library is present" was toggled off [#12680](https://github.com/JabRef/jabref/issues/12680)
 - We improved CSL support with JabRef LibreOffice converter extension. [#14387](https://github.com/JabRef/jabref/issues/14387)
 - We fixed exceptions occuring when generating citation keys or using certain cleanup operations on macOS [#15366](https://github.com/JabRef/jabref/issues/15366)
+- We fixed the merge dialog overwriting the citation key chosen by the user during duplicate import. [#8644](https://github.com/JabRef/jabref/issues/8644)
 
 ### Removed
 

jabgui/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java

@@ -19,6 +19,7 @@
 import org.jabref.logic.help.HelpFile;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.model.entry.BibEntry;
+import org.jabref.model.entry.field.Field;
 
 public class DuplicateResolverDialog extends BaseDialog<DuplicateResolverResult> {
 
@@ -162,6 +163,10 @@ private void init(BibEntry one, BibEntry two, DuplicateResolverType type) {
         getDialogPane().setContent(borderPane);
     }
 
+    public void setMergedFieldValue(Field field, String value) {
+        threeWayMerge.setMergedFieldValue(field, value);
+    }
+
     public BibEntry getMergedEntry() {
         return threeWayMerge.getMergedEntry();
     }

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

@@ -55,6 +55,7 @@
 import org.jabref.model.entry.BibEntryTypesManager;
 import org.jabref.model.entry.BibtexString;
 import org.jabref.model.entry.LinkedFile;
+import org.jabref.model.entry.field.InternalField;
 import org.jabref.model.entry.field.StandardField;
 import org.jabref.model.groups.ExplicitGroup;
 import org.jabref.model.groups.GroupEntryChanger;
@@ -240,8 +241,20 @@ public void importEntries(List<BibEntry> entries) {
     }
 
     public void importCleanedEntries(@Nullable TransferInformation transferInformation, List<BibEntry> entries) {
+        importCleanedEntries(transferInformation, entries, false);
+    }
+
+    /// Imports cleaned entries into the database.
+    ///
+    /// @param transferInformation optional transfer information for adjusting linked files
+    /// @param entries             the entries to import
+    /// @param skipKeyGeneration   if true, skip citation key generation (e.g., when the key was already generated before the merge dialog so the user could choose/edit it)
+
+    private void importCleanedEntries(@Nullable TransferInformation transferInformation, List<BibEntry> entries, boolean skipKeyGeneration) {
         targetBibDatabaseContext.getDatabase().insertEntries(entries);
-        generateKeys(entries);
+        if (!skipKeyGeneration) {
+            generateKeys(entries);
+        }
         setAutomaticFields(entries);
         addToGroups(entries, stateManager.getSelectedGroups(targetBibDatabaseContext));
         addToImportEntriesGroup(entries);
@@ -279,17 +292,25 @@ private void importEntryWithDuplicateCheck(@Nullable TransferInformation transfe
                           LOGGER.error("Error in duplicate search", e);
                       })
                       .onSuccess(existingDuplicateInLibrary -> {
+                          // Generate citation key string WITHOUT modifying the entry,
+                          // so the "From import" column shows the original key
+                          Optional<String> generatedKey = generateKeyString(entryToInsert);
+
                           BibEntry finalEntry = entryToInsert;
                           if (existingDuplicateInLibrary.isPresent()) {
-                              Optional<BibEntry> duplicateHandledEntry = handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision);
+                              Optional<BibEntry> duplicateHandledEntry = handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision, generatedKey);
                               if (duplicateHandledEntry.isEmpty()) {
                                   tracker.markSkipped();
                                   return;
                               }
                               finalEntry = duplicateHandledEntry.get();
+                          } else {
+                              // No duplicate: apply the generated key directly
+                              generatedKey.ifPresent(key -> entryToInsert.setCitationKey(key));
                           }
 
-                          importCleanedEntries(transferInformation, List.of(finalEntry));
+                          // Skip key generation since it was already handled
+                          importCleanedEntries(transferInformation, List.of(finalEntry), true);
 
                           tracker.markImported(finalEntry);
                       }).executeWith(taskExecutor);
@@ -308,12 +329,28 @@ public Optional<BibEntry> findDuplicate(BibEntry entryToCheck) {
     }
 
     public Optional<BibEntry> handleDuplicates(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision) {
-        DuplicateDecisionResult decisionResult = getDuplicateDecision(originalEntry, duplicateEntry, decision);
+        return handleDuplicates(originalEntry, duplicateEntry, decision, Optional.empty());
+    }
+
+    public DuplicateDecisionResult getDuplicateDecision(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision) {
+        return getDuplicateDecision(originalEntry, duplicateEntry, decision, Optional.empty());
+    }
+
+    public Optional<BibEntry> handleDuplicates(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision, Optional<String> generatedKey) {
+        DuplicateDecisionResult decisionResult = getDuplicateDecision(originalEntry, duplicateEntry, decision, generatedKey);
         switch (decisionResult.decision()) {
             case KEEP_RIGHT:
                 targetBibDatabaseContext.getDatabase().removeEntry(duplicateEntry);
+                if (originalEntry.getCitationKey().isEmpty()) {
+                    generatedKey.ifPresent(key -> originalEntry.setCitationKey(key));
+                }
                 break;
             case KEEP_BOTH:
+                Optional<String> originalKey = originalEntry.getCitationKey();
+                Optional<String> duplicateKey = duplicateEntry.getCitationKey();
+                if (originalKey.isEmpty() || originalKey.equals(duplicateKey)) {
+                    generatedKey.ifPresent(key -> originalEntry.setCitationKey(key));
+                }
                 break;
             case KEEP_MERGE:
                 targetBibDatabaseContext.getDatabase().removeEntry(duplicateEntry);
@@ -327,8 +364,9 @@ public Optional<BibEntry> handleDuplicates(BibEntry originalEntry, BibEntry dupl
         return Optional.of(originalEntry);
     }
 
-    public DuplicateDecisionResult getDuplicateDecision(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision) {
+    public DuplicateDecisionResult getDuplicateDecision(BibEntry originalEntry, BibEntry duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult decision, Optional<String> generatedKey) {
         DuplicateResolverDialog dialog = new DuplicateResolverDialog(duplicateEntry, originalEntry, DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, stateManager, dialogService, preferences);
+        generatedKey.ifPresent(key -> dialog.setMergedFieldValue(InternalField.KEY_FIELD, key));
         if (decision == BREAK) {
             decision = dialogService.showCustomDialogAndWait(dialog).orElse(BREAK);
         }
@@ -391,6 +429,22 @@ private void generateKeys(List<BibEntry> entries) {
         entries.forEach(keyGenerator::generateAndSetKey);
     }
 
+    /// Generates a citation key string for the given entry WITHOUT modifying it.
+    ///
+    /// @param entry the entry to generate a key for
+    /// @return the generated key, or empty if key generation is disabled
+    private Optional<String> generateKeyString(BibEntry entry) {
+        if (!preferences.getImporterPreferences().shouldGenerateNewKeyOnImport()) {
+            return Optional.empty();
+        }
+        CitationKeyGenerator keyGenerator = new CitationKeyGenerator(
+                targetBibDatabaseContext.getMetaData().getCiteKeyPatterns(preferences.getCitationKeyPatternPreferences()
+                                                                                     .getKeyPatterns()),
+                targetBibDatabaseContext.getDatabase(),
+                preferences.getCitationKeyPatternPreferences());
+        return Optional.of(keyGenerator.generateKey(entry));
+    }
+
     public @NonNull List<@NonNull BibEntry> handleBibTeXData(@NonNull String entries) {
         if (!entries.contains("@")) {
             LOGGER.debug("Seems not to be BibTeX data: {}", entries);

jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/ThreeWayMergeView.java

@@ -204,6 +204,15 @@ public BibEntry getRightEntry() {
         return viewModel.getRightEntry();
     }
 
+    public void setMergedFieldValue(Field field, String value) {
+        for (FieldRowView row : fieldRows) {
+            if (row.viewModel.getField().equals(field)) {
+                row.viewModel.setMergedFieldValue(value);
+                break;
+            }
+        }
+    }
+
     public void saveConfiguration() {
         toolbar.saveToolbarConfiguration();
     }

jabgui/src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java

@@ -1,6 +1,7 @@
 package org.jabref.gui.externalfiles;
 
 import java.util.List;
+import java.util.Optional;
 
 import javax.swing.undo.UndoManager;
 
@@ -11,16 +12,23 @@
 import org.jabref.gui.duplicationFinder.DuplicateResolverDialog;
 import org.jabref.gui.preferences.GuiPreferences;
 import org.jabref.logic.FilePreferences;
+import org.jabref.logic.LibraryPreferences;
 import org.jabref.logic.bibtex.FieldPreferences;
+import org.jabref.logic.citationkeypattern.CitationKeyPatternPreferences;
+import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns;
 import org.jabref.logic.database.DuplicateCheck;
 import org.jabref.logic.importer.ImportFormatPreferences;
+import org.jabref.logic.importer.ImporterPreferences;
+import org.jabref.logic.preferences.OwnerPreferences;
+import org.jabref.logic.preferences.TimestampPreferences;
 import org.jabref.logic.util.CurrentThreadTaskExecutor;
 import org.jabref.model.database.BibDatabase;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.database.BibDatabaseMode;
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.field.StandardField;
 import org.jabref.model.entry.types.StandardEntryType;
+import org.jabref.model.metadata.MetaData;
 import org.jabref.model.util.DummyFileUpdateMonitor;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -142,7 +150,7 @@ void handleDuplicatesKeepRightTest() {
                 mock(DialogService.class),
                 new CurrentThreadTaskExecutor()));
         // Mock the behavior of getDuplicateDecision to return KEEP_RIGHT
-        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK);
+        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK, Optional.empty());
 
         // Act
         BibEntry result = importHandler.handleDuplicates(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK).get();
@@ -173,7 +181,7 @@ void handleDuplicatesKeepBothTest() {
                 mock(DialogService.class),
                 new CurrentThreadTaskExecutor()));
         // Mock the behavior of getDuplicateDecision to return KEEP_BOTH
-        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK);
+        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK, Optional.empty());
 
         // Act
         BibEntry result = importHandler.handleDuplicates(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK).get();
@@ -207,7 +215,7 @@ void handleDuplicatesKeepMergeTest() {
                 mock(DialogService.class),
                 new CurrentThreadTaskExecutor()));
         // Mock the behavior of getDuplicateDecision to return KEEP_MERGE
-        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK);
+        Mockito.doReturn(decisionResult).when(importHandler).getDuplicateDecision(testEntry, duplicateEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK, Optional.empty());
 
         // Act
         // create and return a default BibEntry or do other computations
@@ -218,4 +226,125 @@ void handleDuplicatesKeepMergeTest() {
         assertFalse(bibDatabase.getEntries().contains(duplicateEntry)); // Assert that the duplicate entry was removed from the database
         assertEquals(mergedEntry, result); // Assert that the merged entry is returned
     }
+
+    @Test
+    void handleDuplicatesKeepBothWithCollidingKeysAppliesGeneratedKey() {
+        // Arrange
+        BibEntry duplicateEntry = new BibEntry(StandardEntryType.Article)
+                .withCitationKey("SameKey2023")
+                .withField(StandardField.AUTHOR, "Duplicate Author");
+
+        BibEntry originalEntry = new BibEntry(StandardEntryType.Article)
+                .withCitationKey("SameKey2023")
+                .withField(StandardField.AUTHOR, "Original Author");
+
+        BibDatabase bibDatabase = bibDatabaseContext.getDatabase();
+        bibDatabase.insertEntry(duplicateEntry);
+
+        DuplicateDecisionResult decisionResult = new DuplicateDecisionResult(
+                DuplicateResolverDialog.DuplicateResolverResult.KEEP_BOTH, null);
+        importHandler = Mockito.spy(new ImportHandler(
+                bibDatabaseContext,
+                preferences,
+                new DummyFileUpdateMonitor(),
+                mock(UndoManager.class),
+                mock(StateManager.class),
+                mock(DialogService.class),
+                new CurrentThreadTaskExecutor()));
+
+        String generatedKey = "UniqueKey2023";
+        Mockito.doReturn(decisionResult).when(importHandler)
+               .getDuplicateDecision(originalEntry, duplicateEntry,
+                       DuplicateResolverDialog.DuplicateResolverResult.BREAK,
+                       Optional.of(generatedKey));
+
+        // Act
+        BibEntry result = importHandler.handleDuplicates(originalEntry, duplicateEntry,
+                DuplicateResolverDialog.DuplicateResolverResult.BREAK,
+                Optional.of(generatedKey)).get();
+
+        // Assert: the colliding key should be replaced by the generated one
+        assertEquals(generatedKey, result.getCitationKey().orElse(""),
+                "When keeping both entries with the same key, the imported entry should get the generated key");
+        assertEquals("SameKey2023", duplicateEntry.getCitationKey().orElse(""),
+                "The existing entry should keep its original key");
+    }
+
+    @Test
+    void importWithDuplicateCheckPreservesMergedCitationKey() {
+        // Arrange: set up key generation with pattern [auth][year]
+        ImporterPreferences importerPreferences = mock(ImporterPreferences.class);
+        when(importerPreferences.shouldGenerateNewKeyOnImport()).thenReturn(true);
+        when(preferences.getImporterPreferences()).thenReturn(importerPreferences);
+
+        GlobalCitationKeyPatterns patterns = GlobalCitationKeyPatterns.fromPattern("[auth][year]");
+        CitationKeyPatternPreferences citationKeyPatternPreferences = mock(CitationKeyPatternPreferences.class);
+        when(citationKeyPatternPreferences.getKeyPatterns()).thenReturn(patterns);
+        when(citationKeyPatternPreferences.getUnwantedCharacters()).thenReturn("");
+        when(citationKeyPatternPreferences.getKeywordDelimiter()).thenReturn(',');
+        when(citationKeyPatternPreferences.getKeySuffix()).thenReturn(CitationKeyPatternPreferences.KeySuffix.SECOND_WITH_B);
+        when(preferences.getCitationKeyPatternPreferences()).thenReturn(citationKeyPatternPreferences);
+
+        MetaData metaData = mock(MetaData.class);
+        when(metaData.getCiteKeyPatterns(any())).thenReturn(patterns);
+        when(bibDatabaseContext.getMetaData()).thenReturn(metaData);
+
+        StateManager stateManager = mock(StateManager.class);
+        when(stateManager.getSelectedGroups(any())).thenReturn(FXCollections.observableArrayList());
+
+        LibraryPreferences libraryPreferences = mock(LibraryPreferences.class);
+        when(libraryPreferences.isAddImportedEntriesEnabled()).thenReturn(false);
+        when(preferences.getLibraryPreferences()).thenReturn(libraryPreferences);
+
+        when(preferences.getOwnerPreferences()).thenReturn(mock(OwnerPreferences.class));
+        when(preferences.getTimestampPreferences()).thenReturn(mock(TimestampPreferences.class));
+
+        FilePreferences filePreferences = mock(FilePreferences.class);
+        when(filePreferences.shouldDownloadLinkedFiles()).thenReturn(false);
+        when(preferences.getFilePreferences()).thenReturn(filePreferences);
+
+        importHandler = Mockito.spy(new ImportHandler(
+                bibDatabaseContext,
+                preferences,
+                new DummyFileUpdateMonitor(),
+                mock(UndoManager.class),
+                stateManager,
+                mock(DialogService.class),
+                new CurrentThreadTaskExecutor()));
+
+        // Existing entry in the database
+        BibEntry existingEntry = new BibEntry(StandardEntryType.Article)
+                .withCitationKey("ExistingKey")
+                .withField(StandardField.AUTHOR, "Smith")
+                .withField(StandardField.YEAR, "2022")
+                .withField(StandardField.TITLE, "Some Title");
+        BibDatabase bibDatabase = bibDatabaseContext.getDatabase();
+        bibDatabase.insertEntry(existingEntry);
+
+        // User-chosen merged entry with a custom citation key
+        String userChosenKey = "MyCustomKey";
+        BibEntry mergedEntry = new BibEntry(StandardEntryType.Article)
+                .withCitationKey(userChosenKey)
+                .withField(StandardField.AUTHOR, "Smith")
+                .withField(StandardField.YEAR, "2022")
+                .withField(StandardField.TITLE, "Some Title");
+
+        // Entry to import
+        BibEntry importEntry = new BibEntry(StandardEntryType.Article)
+                .withField(StandardField.AUTHOR, "Smith")
+                .withField(StandardField.YEAR, "2022")
+                .withField(StandardField.TITLE, "Some Title");
+        // Mock findDuplicate to return the existing entry as a duplicate
+        Mockito.doReturn(Optional.of(existingEntry)).when(importHandler).findDuplicate(importEntry);
+
+        // Mock handleDuplicates(4 params) to simulate user choosing KEEP_MERGE with their custom key
+        Mockito.doReturn(Optional.of(mergedEntry)).when(importHandler).handleDuplicates(importEntry, existingEntry, DuplicateResolverDialog.DuplicateResolverResult.BREAK, Optional.of("Smith2022"));
+
+        // Act
+        importHandler.importEntriesWithDuplicateCheck(null, List.of(importEntry));
+
+        // Assert: the user's chosen citation key from the merge dialog must be preserved
+        assertEquals(userChosenKey, mergedEntry.getCitationKey().orElse(""),
+                "The citation key chosen by the user in the merge dialog should not be overwritten by key generation");
+    }
 }