From 6a4a4f4a588a3aee6041a3fe0141908dc3133148 Mon Sep 17 00:00:00 2001 From: Houssem Nasri Date: Wed, 14 Sep 2022 19:10:51 +0100 Subject: [PATCH] Clarify the duplicates resolver dialog actions (#9140) * Don't show the info button when the authors field content is identical * Use enhanced switch expression * Improve duplicate resolver dialog actions - Renamed the actions. - Changed their layout; some actions on the left and some on the right. - Commented out the help action for now. * Move 'Keep old entry' action to the left * do something * Remove DuplicateResolverType#INSPECTION - This enum was used by to decide what buttons to add to the duplicate resolver dialog in ImportEntriesDialog. However, both ImportHandler and ImportEntriesDialog have the same buttons. In addition, they share logic too. We should consider merging them at some point. * i18n * Layout the 'Automatically remove exact duplicates' button in the button bar * Add help button * Update CHANGELOG.md --- CHANGELOG.md | 1 + src/main/java/org/jabref/gui/JabRefFrame.java | 2 +- .../DuplicateResolverDialog.java | 77 ++++++++++--------- .../duplicationFinder/DuplicateSearch.java | 8 +- .../gui/externalfiles/ImportHandler.java | 6 +- .../gui/importer/ImportEntriesViewModel.java | 4 +- src/main/resources/l10n/JabRef_en.properties | 14 +--- 7 files changed, 56 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 251b6ec7e30..0e4cfe3ac8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve - The fallback directory of the file folder now is the general file directory. In case there was a directory configured for a library and this directory was not found, JabRef placed the PDF next to the .bib file and not into the general file directory. - The global default directory for storing PDFs is now the subdirectory "JabRef" in the user's home. - We reworked the Define study parameters dialog. [#9123](https://github.com/JabRef/jabref/pull/9123) +- We simplified the actions to fast-resolve duplicates to 'Keep Left', 'Keep Right', 'Keep Both' and 'Keep Merged'. [#9056](https://github.com/JabRef/jabref/issues/9056) ### Fixed diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java index dd2bf48db16..9a5aeac2d9a 100644 --- a/src/main/java/org/jabref/gui/JabRefFrame.java +++ b/src/main/java/org/jabref/gui/JabRefFrame.java @@ -820,7 +820,7 @@ private MenuBar createMenu() { ); quality.getItems().addAll( - factory.createMenuItem(StandardActions.FIND_DUPLICATES, new DuplicateSearch(this, dialogService, stateManager)), + factory.createMenuItem(StandardActions.FIND_DUPLICATES, new DuplicateSearch(this, dialogService, stateManager, prefs)), factory.createMenuItem(StandardActions.MERGE_ENTRIES, new MergeEntriesAction(dialogService, stateManager)), factory.createMenuItem(StandardActions.CHECK_INTEGRITY, new IntegrityCheckAction(this, stateManager, Globals.TASK_EXECUTOR)), factory.createMenuItem(StandardActions.CLEANUP_ENTRIES, new CleanupAction(this, this.prefs, dialogService, stateManager)), diff --git a/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java b/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java index 6a89a239500..07644ca748a 100644 --- a/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java +++ b/src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java @@ -8,6 +8,8 @@ import org.jabref.gui.DialogService; import org.jabref.gui.StateManager; +import org.jabref.gui.actions.ActionFactory; +import org.jabref.gui.actions.StandardActions; import org.jabref.gui.duplicationFinder.DuplicateResolverDialog.DuplicateResolverResult; import org.jabref.gui.help.HelpAction; import org.jabref.gui.mergeentries.newmergedialog.ThreeWayMergeView; @@ -17,6 +19,7 @@ import org.jabref.logic.l10n.Localization; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; +import org.jabref.preferences.PreferencesService; public class DuplicateResolverDialog extends BaseDialog { @@ -26,7 +29,6 @@ public class DuplicateResolverDialog extends BaseDialog public enum DuplicateResolverType { DUPLICATE_SEARCH, IMPORT_CHECK, - INSPECTION, DUPLICATE_SEARCH_WITH_EXACT } @@ -41,65 +43,63 @@ public enum DuplicateResolverResult { private ThreeWayMergeView threeWayMerge; private final DialogService dialogService; + private final ActionFactory actionFactory; - public DuplicateResolverDialog(BibEntry one, BibEntry two, DuplicateResolverType type, BibDatabaseContext database, StateManager stateManager, DialogService dialogService) { + public DuplicateResolverDialog(BibEntry one, BibEntry two, DuplicateResolverType type, BibDatabaseContext database, StateManager stateManager, DialogService dialogService, PreferencesService prefs) { this.setTitle(Localization.lang("Possible duplicate entries")); this.database = database; this.stateManager = stateManager; this.dialogService = dialogService; + this.actionFactory = new ActionFactory(prefs.getKeyBindingRepository()); init(one, two, type); } private void init(BibEntry one, BibEntry two, DuplicateResolverType type) { - HelpAction helpCommand = new HelpAction(HelpFile.FIND_DUPLICATES, dialogService); - ButtonType help = new ButtonType(Localization.lang("Help"), ButtonData.HELP); - ButtonType cancel = ButtonType.CANCEL; - ButtonType merge = new ButtonType(Localization.lang("Keep merged entry only"), ButtonData.APPLY); + ButtonType merge = new ButtonType(Localization.lang("Keep merged"), ButtonData.OK_DONE); - ButtonBar options = new ButtonBar(); ButtonType both; ButtonType second; ButtonType first; - ButtonType removeExact = new ButtonType(Localization.lang("Automatically remove exact duplicates"), ButtonData.APPLY); + ButtonType removeExact = new ButtonType(Localization.lang("Automatically remove exact duplicates"), ButtonData.LEFT); boolean removeExactVisible = false; switch (type) { - case DUPLICATE_SEARCH: - first = new ButtonType(Localization.lang("Keep left"), ButtonData.APPLY); - second = new ButtonType(Localization.lang("Keep right"), ButtonData.APPLY); - both = new ButtonType(Localization.lang("Keep both"), ButtonData.APPLY); + case DUPLICATE_SEARCH -> { + first = new ButtonType(Localization.lang("Keep left"), ButtonData.LEFT); + second = new ButtonType(Localization.lang("Keep right"), ButtonData.LEFT); + both = new ButtonType(Localization.lang("Keep both"), ButtonData.LEFT); threeWayMerge = new ThreeWayMergeView(one, two); - break; - case INSPECTION: - first = new ButtonType(Localization.lang("Remove old entry"), ButtonData.APPLY); - second = new ButtonType(Localization.lang("Remove entry from import"), ButtonData.APPLY); - both = new ButtonType(Localization.lang("Keep both"), ButtonData.APPLY); - threeWayMerge = new ThreeWayMergeView(one, two, Localization.lang("Old entry"), - Localization.lang("From import")); - break; - case DUPLICATE_SEARCH_WITH_EXACT: - first = new ButtonType(Localization.lang("Keep left"), ButtonData.APPLY); - second = new ButtonType(Localization.lang("Keep right"), ButtonData.APPLY); - both = new ButtonType(Localization.lang("Keep both"), ButtonData.APPLY); - + } + case DUPLICATE_SEARCH_WITH_EXACT -> { + first = new ButtonType(Localization.lang("Keep left"), ButtonData.LEFT); + second = new ButtonType(Localization.lang("Keep right"), ButtonData.LEFT); + both = new ButtonType(Localization.lang("Keep both"), ButtonData.LEFT); removeExactVisible = true; - threeWayMerge = new ThreeWayMergeView(one, two); - break; - default: - first = new ButtonType(Localization.lang("Import and remove old entry"), ButtonData.APPLY); - second = new ButtonType(Localization.lang("Do not import entry"), ButtonData.APPLY); - both = new ButtonType(Localization.lang("Import and keep old entry"), ButtonData.APPLY); + } + case IMPORT_CHECK -> { + first = new ButtonType(Localization.lang("Keep old entry"), ButtonData.LEFT); + second = new ButtonType(Localization.lang("Keep from import"), ButtonData.LEFT); + both = new ButtonType(Localization.lang("Keep both"), ButtonData.LEFT); threeWayMerge = new ThreeWayMergeView(one, two, Localization.lang("Old entry"), - Localization.lang("From import")); - break; + Localization.lang("From import")); + } + default -> throw new IllegalStateException("Switch expression should be exhaustive"); } + + this.getDialogPane().getButtonTypes().addAll(first, second, both, merge, cancel); + this.getDialogPane().setFocusTraversable(false); + if (removeExactVisible) { this.getDialogPane().getButtonTypes().add(removeExact); - } - this.getDialogPane().getButtonTypes().addAll(first, second, both, merge, cancel, help); + // This will prevent all dialog buttons from having the same size + // Read more: https://stackoverflow.com/questions/45866249/javafx-8-alert-different-button-sizes + getDialogPane().getButtonTypes().stream() + .map(getDialogPane()::lookupButton) + .forEach(btn-> ButtonBar.setButtonUniformSize(btn, false)); + } // Retrieves the previous window state and sets the new dialog window size and position to match it DialogWindowState state = stateManager.getDialogWindowState(getClass().getSimpleName()); @@ -110,7 +110,6 @@ private void init(BibEntry one, BibEntry two, DuplicateResolverType type) { } BorderPane borderPane = new BorderPane(threeWayMerge); - borderPane.setBottom(options); this.setResultConverter(button -> { // Updates the window state on button press @@ -130,9 +129,11 @@ private void init(BibEntry one, BibEntry two, DuplicateResolverType type) { return null; }); + HelpAction helpCommand = new HelpAction(HelpFile.FIND_DUPLICATES, dialogService); + Button helpButton = actionFactory.createIconButton(StandardActions.HELP, helpCommand); + borderPane.setRight(helpButton); + getDialogPane().setContent(borderPane); - Button helpButton = (Button) this.getDialogPane().lookupButton(help); - helpButton.setOnAction(evt -> helpCommand.execute()); } public BibEntry getMergedEntry() { diff --git a/src/main/java/org/jabref/gui/duplicationFinder/DuplicateSearch.java b/src/main/java/org/jabref/gui/duplicationFinder/DuplicateSearch.java index 46ddfaf6f3b..611240c2701 100644 --- a/src/main/java/org/jabref/gui/duplicationFinder/DuplicateSearch.java +++ b/src/main/java/org/jabref/gui/duplicationFinder/DuplicateSearch.java @@ -34,6 +34,7 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.database.BibDatabaseMode; import org.jabref.model.entry.BibEntry; +import org.jabref.preferences.PreferencesService; import static org.jabref.gui.actions.ActionHelper.needsDatabase; @@ -51,10 +52,13 @@ public class DuplicateSearch extends SimpleCommand { private final DialogService dialogService; private final StateManager stateManager; - public DuplicateSearch(JabRefFrame frame, DialogService dialogService, StateManager stateManager) { + private final PreferencesService prefs; + + public DuplicateSearch(JabRefFrame frame, DialogService dialogService, StateManager stateManager, PreferencesService prefs) { this.frame = frame; this.dialogService = dialogService; this.stateManager = stateManager; + this.prefs = prefs; this.executable.bind(needsDatabase(stateManager)); } @@ -142,7 +146,7 @@ private DuplicateSearchResult verifyDuplicates() { } private void askResolveStrategy(DuplicateSearchResult result, BibEntry first, BibEntry second, DuplicateResolverType resolverType) { - DuplicateResolverDialog dialog = new DuplicateResolverDialog(first, second, resolverType, frame.getCurrentLibraryTab().getBibDatabaseContext(), stateManager, dialogService); + DuplicateResolverDialog dialog = new DuplicateResolverDialog(first, second, resolverType, frame.getCurrentLibraryTab().getBibDatabaseContext(), stateManager, dialogService, prefs); dialog.titleProperty().bind(Bindings.concat(dialog.getTitle()).concat(" (").concat(duplicateProgress.getValue()).concat("/").concat(duplicateTotal).concat(")")); diff --git a/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java b/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java index b417e49fc3e..873ae09cb59 100644 --- a/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java +++ b/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java @@ -195,9 +195,9 @@ public void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, Optional existingDuplicateInLibrary = new DuplicateCheck(Globals.entryTypesManager).containsDuplicate(bibDatabaseContext.getDatabase(), entryToInsert, bibDatabaseContext.getMode()); if (existingDuplicateInLibrary.isPresent()) { - DuplicateResolverDialog dialog = new DuplicateResolverDialog(existingDuplicateInLibrary.get(), entryToInsert, DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, bibDatabaseContext, stateManager, dialogService); + DuplicateResolverDialog dialog = new DuplicateResolverDialog(existingDuplicateInLibrary.get(), entryToInsert, DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, bibDatabaseContext, stateManager, dialogService, preferencesService); switch (dialogService.showCustomDialogAndWait(dialog).orElse(DuplicateResolverDialog.DuplicateResolverResult.BREAK)) { - case KEEP_LEFT: + case KEEP_RIGHT: bibDatabaseContext.getDatabase().removeEntry(existingDuplicateInLibrary.get()); break; case KEEP_BOTH: @@ -206,7 +206,7 @@ public void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, bibDatabaseContext.getDatabase().removeEntry(existingDuplicateInLibrary.get()); entryToInsert = dialog.getMergedEntry(); break; - case KEEP_RIGHT: + case KEEP_LEFT: case AUTOREMOVE_EXACT: case BREAK: default: diff --git a/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java b/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java index 0f75519e910..15dde7bec70 100644 --- a/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java +++ b/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java @@ -197,7 +197,7 @@ public void resolveDuplicate(BibEntry entry) { Optional other = new DuplicateCheck(entryTypesManager).containsDuplicate(databaseContext.getDatabase(), entry, databaseContext.getMode()); if (other.isPresent()) { DuplicateResolverDialog dialog = new DuplicateResolverDialog(other.get(), - entry, DuplicateResolverDialog.DuplicateResolverType.INSPECTION, databaseContext, stateManager, dialogService); + entry, DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, databaseContext, stateManager, dialogService, preferences); DuplicateResolverDialog.DuplicateResolverResult result = dialogService.showCustomDialogAndWait(dialog) .orElse(DuplicateResolverDialog.DuplicateResolverResult.BREAK); @@ -228,7 +228,7 @@ public void resolveDuplicate(BibEntry entry) { other = findInternalDuplicate(entry); if (other.isPresent()) { DuplicateResolverDialog diag = new DuplicateResolverDialog(entry, - other.get(), DuplicateResolverDialog.DuplicateResolverType.DUPLICATE_SEARCH, databaseContext, stateManager, dialogService); + other.get(), DuplicateResolverDialog.DuplicateResolverType.DUPLICATE_SEARCH, databaseContext, stateManager, dialogService, preferences); DuplicateResolverDialog.DuplicateResolverResult answer = dialogService.showCustomDialogAndWait(diag) .orElse(DuplicateResolverDialog.DuplicateResolverResult.BREAK); diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 12093e83f81..4c18d16b0c6 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -260,7 +260,6 @@ Display\ version=Display version Do\ not\ abbreviate\ names=Do not abbreviate names -Do\ not\ import\ entry=Do not import entry Do\ not\ open\ any\ files\ at\ startup=Do not open any files at startup @@ -431,10 +430,6 @@ Ignore=Ignore Import=Import -Import\ and\ keep\ old\ entry=Import and keep old entry - -Import\ and\ remove\ old\ entry=Import and remove old entry - Import\ entries=Import entries Import\ file=Import file @@ -688,8 +683,6 @@ Remove\ subgroups=Remove subgroups Remove\ all\ subgroups\ of\ "%0"?=Remove all subgroups of "%0"? -Remove\ entry\ from\ import=Remove entry from import - Remove\ selected\ entries\ from\ this\ group=Remove selected entries from this group Remove\ group=Remove group @@ -716,8 +709,6 @@ Removed\ all\ selected\ groups\ and\ their\ subgroups.=Removed all selected grou Remove\ link=Remove link -Remove\ old\ entry=Remove old entry - Remove\ string\ %0=Remove string %0 Removed\ group\ "%0".=Removed group "%0". @@ -1346,7 +1337,7 @@ Warning\:\ %0\ out\ of\ %1\ entries\ have\ undefined\ citation\ key.=Warning: %0 Warning\:\ %0\ out\ of\ %1\ entries\ have\ undefined\ DOIs.=Warning: %0 out of %1 entries have undefined DOIs. Really\ delete\ the\ selected\ entry?=Really delete the selected entry? Really\ delete\ the\ %0\ selected\ entries?=Really delete the %0 selected entries? -Keep\ merged\ entry\ only=Keep merged entry only + Keep\ left=Keep left Keep\ right=Keep right Old\ entry=Old entry @@ -2514,3 +2505,6 @@ plain\ text=plain text The\ %0s\ are\ the\ same.\ However,\ the\ order\ of\ field\ content\ differs=The %0s are the same. However, the order of field content differs +Keep\ from\ import=Keep from import +Keep\ merged=Keep merged +Keep\ old\ entry=Keep old entry