Fix selection of table sort order (JabRef#10250)

* Inline LOGGER.debug

* Move out work in constructor to method

* Streamline code

* Same comments

* Fix variable name

* "Flatten" SaveOrder if OrderType.TABLE

* WIP: Show diff in UI

* Make it scrollable

Co-authored-by: Christoph <siedlerkiller@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

* WIP: Try to fix content selector diff

Co-authored-by: Christoph <siedlerkiller@gmail.com>

* Add some debugging

* Update preferences immediatly after change (and not sometime later)

* Fix serialization of SaveOrder

* Add CHANGELOG.md entry

* Fix mapping of SaveOrderConfig

* Introduce SelfContainedSaveOrder

* More SelfContainedSaveOrder

* Remove ref to PrefsService

* Compile fix

* Made OrFields NOT extending LinkedHashSet<Field>

* Fix hillarious bug

* Fixed tests

* Try to fix FieldComparators for OrFields

* Fix order of null comparisons

Refs tests of JabRef#7544

* Fix checkstyle

* Add missing equals, hashcode and toString

* Fix checkstyle

* Restore test files

* Fix OpenRewrite

* Fix modernizer

* Update CHANGELOG.md

* Fix NPE

---------

Co-authored-by: Christoph <siedlerkiller@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

.gitignore

@@ -478,5 +478,7 @@ lib/ojdbc.jar
 # do not ignore JabRef icons (they are ignored by the macos setting above)
 !src/main/java/org/jabref/gui/icon
 
+!**/autosaveandbackup/*.bak
+
 # generated during release process
 CHANGELOG.html

CHANGELOG.md

@@ -18,6 +18,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 ### Fixed
 
+- It is possible again to use "current table sort order" for the order of entries when saving. [#9869](https://github.com/JabRef/jabref/issues/9869)
+
 ### Removed
 
 ## [5.10] - 2023-09-02

src/main/java/org/jabref/gui/JabRefFrame.java

@@ -54,6 +54,8 @@
 import org.jabref.gui.actions.ActionHelper;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.gui.actions.StandardActions;
+import org.jabref.gui.autosaveandbackup.AutosaveManager;
+import org.jabref.gui.autosaveandbackup.BackupManager;
 import org.jabref.gui.auximport.NewSubLibraryAction;
 import org.jabref.gui.bibtexextractor.ExtractBibtexAction;
 import org.jabref.gui.citationkeypattern.GenerateCitationKeyAction;
@@ -118,8 +120,6 @@
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.gui.util.TaskExecutor;
-import org.jabref.logic.autosaveandbackup.AutosaveManager;
-import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.citationstyle.CitationStyleOutputFormat;
 import org.jabref.logic.help.HelpFile;
 import org.jabref.logic.importer.IdFetcher;

src/main/java/org/jabref/gui/LibraryTab.java

@@ -26,6 +26,8 @@
 import org.jabref.gui.autocompleter.AutoCompletePreferences;
 import org.jabref.gui.autocompleter.PersonNameSuggestionProvider;
 import org.jabref.gui.autocompleter.SuggestionProviders;
+import org.jabref.gui.autosaveandbackup.AutosaveManager;
+import org.jabref.gui.autosaveandbackup.BackupManager;
 import org.jabref.gui.collab.DatabaseChangeMonitor;
 import org.jabref.gui.dialogs.AutosaveUiManager;
 import org.jabref.gui.entryeditor.EntryEditor;
@@ -39,8 +41,6 @@
 import org.jabref.gui.undo.UndoableRemoveEntries;
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.gui.util.DefaultTaskExecutor;
-import org.jabref.logic.autosaveandbackup.AutosaveManager;
-import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.citationstyle.CitationStyleCache;
 import org.jabref.logic.importer.ParserResult;
 import org.jabref.logic.importer.util.FileFieldParser;
@@ -294,7 +294,7 @@ public void installAutosaveManagerAndBackupManager() {
             autosaveManager.registerListener(new AutosaveUiManager(this, preferencesService, entryTypesManager));
         }
         if (isDatabaseReadyForBackup(bibDatabaseContext) && preferencesService.getFilePreferences().shouldCreateBackup()) {
-            BackupManager.start(bibDatabaseContext, Globals.entryTypesManager, preferencesService);
+            BackupManager.start(this, bibDatabaseContext, Globals.entryTypesManager, preferencesService);
         }
     }
 

src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java → src/main/java/org/jabref/gui/autosaveandbackup/AutosaveManager.java

@@ -1,4 +1,4 @@
-package org.jabref.logic.autosaveandbackup;
+package org.jabref.gui.autosaveandbackup;
 
 import java.util.HashSet;
 import java.util.Set;

src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java → src/main/java/org/jabref/gui/autosaveandbackup/BackupManager.java

@@ -1,4 +1,4 @@
-package org.jabref.logic.autosaveandbackup;
+package org.jabref.gui.autosaveandbackup;
 
 import java.io.IOException;
 import java.io.Writer;
@@ -19,6 +19,11 @@
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 
+import javafx.scene.control.TableColumn;
+
+import org.jabref.gui.LibraryTab;
+import org.jabref.gui.maintable.BibEntryTableViewModel;
+import org.jabref.gui.maintable.columns.MainTableColumn;
 import org.jabref.logic.bibtex.InvalidFieldValueException;
 import org.jabref.logic.exporter.AtomicFileWriter;
 import org.jabref.logic.exporter.BibWriter;
@@ -31,6 +36,7 @@
 import org.jabref.model.database.event.BibDatabaseContextChangedEvent;
 import org.jabref.model.entry.BibEntryTypesManager;
 import org.jabref.model.metadata.SaveOrder;
+import org.jabref.model.metadata.SelfContainedSaveOrder;
 import org.jabref.preferences.PreferencesService;
 
 import com.google.common.eventbus.Subscribe;
@@ -58,17 +64,19 @@ public class BackupManager {
     private final ScheduledThreadPoolExecutor executor;
     private final CoarseChangeFilter changeFilter;
     private final BibEntryTypesManager entryTypesManager;
+    private final LibraryTab libraryTab;
 
     // Contains a list of all backup paths
     // During a write, the less recent backup file is deleted
     private final Queue<Path> backupFilesQueue = new LinkedBlockingQueue<>();
     private boolean needsBackup = false;
 
-    BackupManager(BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, PreferencesService preferences) {
+    BackupManager(LibraryTab libraryTab, BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, PreferencesService preferences) {
         this.bibDatabaseContext = bibDatabaseContext;
         this.entryTypesManager = entryTypesManager;
         this.preferences = preferences;
         this.executor = new ScheduledThreadPoolExecutor(2);
+        this.libraryTab = libraryTab;
 
         changeFilter = new CoarseChangeFilter(bibDatabaseContext);
         changeFilter.registerListener(this);
@@ -96,8 +104,8 @@ static Optional<Path> getLatestBackupPath(Path originalPath, Path backupDir) {
      *
      * @param bibDatabaseContext Associated {@link BibDatabaseContext}
      */
-    public static BackupManager start(BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, PreferencesService preferences) {
-        BackupManager backupManager = new BackupManager(bibDatabaseContext, entryTypesManager, preferences);
+    public static BackupManager start(LibraryTab libraryTab, BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, PreferencesService preferences) {
+        BackupManager backupManager = new BackupManager(libraryTab, bibDatabaseContext, entryTypesManager, preferences);
         backupManager.startBackupTask(preferences.getFilePreferences().getBackupDirectory());
         runningInstances.add(backupManager);
         return backupManager;
@@ -215,18 +223,36 @@ void performBackup(Path backupPath) {
 
         // We opted for "while" to delete backups in case there are more than 10
         while (backupFilesQueue.size() >= MAXIMUM_BACKUP_FILE_COUNT) {
-            Path lessRecentBackupFile = backupFilesQueue.poll();
+            Path oldestBackupFile = backupFilesQueue.poll();
             try {
-                Files.delete(lessRecentBackupFile);
+                Files.delete(oldestBackupFile);
             } catch (IOException e) {
-                LOGGER.error("Could not delete backup file {}", lessRecentBackupFile, e);
+                LOGGER.error("Could not delete backup file {}", oldestBackupFile, e);
             }
         }
 
         // code similar to org.jabref.gui.exporter.SaveDatabaseAction.saveDatabase
+        SelfContainedSaveOrder saveOrder = bibDatabaseContext
+                .getMetaData().getSaveOrder()
+                .map(so -> {
+                    if (so.getOrderType() == SaveOrder.OrderType.TABLE) {
+                        // We need to "flatten out" SaveOrder.OrderType.TABLE as BibWriter does not have access to preferences
+                        List<TableColumn<BibEntryTableViewModel, ?>> sortOrder = libraryTab.getMainTable().getSortOrder();
+                        return new SelfContainedSaveOrder(
+                                SaveOrder.OrderType.SPECIFIED,
+                                sortOrder.stream()
+                                         .filter(col -> col instanceof MainTableColumn<?>)
+                                         .map(column -> ((MainTableColumn<?>) column).getModel())
+                                         .flatMap(model -> model.getSortCriteria().stream())
+                                         .toList());
+                    } else {
+                        return SelfContainedSaveOrder.of(so);
+                    }
+                })
+                .orElse(SaveOrder.getDefaultSaveOrder());
         SaveConfiguration saveConfiguration = new SaveConfiguration()
                 .withMakeBackup(false)
-                .withSaveOrder(bibDatabaseContext.getMetaData().getSaveOrder().orElse(SaveOrder.getDefaultSaveOrder()))
+                .withSaveOrder(saveOrder)
                 .withReformatOnSave(preferences.getLibraryPreferences().shouldAlwaysReformatOnSave());
 
         Charset encoding = bibDatabaseContext.getMetaData().getEncoding().orElse(StandardCharsets.UTF_8);

src/main/java/org/jabref/gui/collab/metedatachange/MetadataChangeDetailsView.java

@@ -1,6 +1,7 @@
 package org.jabref.gui.collab.metedatachange;
 
 import javafx.scene.control.Label;
+import javafx.scene.control.ScrollPane;
 import javafx.scene.layout.VBox;
 
 import org.jabref.gui.collab.DatabaseChangeDetailsView;
@@ -17,20 +18,24 @@ public MetadataChangeDetailsView(MetadataChange metadataChange, PreferencesServi
         header.getStyleClass().add("sectionHeader");
         container.getChildren().add(header);
 
-        for (MetaDataDiff.Difference change : metadataChange.getMetaDataDiff().getDifferences(preferencesService)) {
-            container.getChildren().add(new Label(getDifferenceString(change)));
+        for (MetaDataDiff.Difference diff : metadataChange.getMetaDataDiff().getDifferences(preferencesService)) {
+            container.getChildren().add(new Label(getDifferenceString(diff.differenceType())));
+            container.getChildren().add(new Label(diff.originalObject().toString()));
+            container.getChildren().add(new Label(diff.newObject().toString()));
         }
 
-        setLeftAnchor(container, 8d);
-        setTopAnchor(container, 8d);
-        setRightAnchor(container, 8d);
-        setBottomAnchor(container, 8d);
+        ScrollPane scrollPane = new ScrollPane(container);
+        scrollPane.setFitToWidth(true);
+        getChildren().setAll(scrollPane);
 
-        getChildren().setAll(container);
+        setLeftAnchor(scrollPane, 8d);
+        setTopAnchor(scrollPane, 8d);
+        setRightAnchor(scrollPane, 8d);
+        setBottomAnchor(scrollPane, 8d);
     }
 
-    private String getDifferenceString(MetaDataDiff.Difference change) {
-        return switch (change) {
+    private String getDifferenceString(MetaDataDiff.DifferenceType changeType) {
+        return switch (changeType) {
             case PROTECTED ->
                     Localization.lang("Library protection");
             case GROUPS_ALTERED ->

src/main/java/org/jabref/gui/dialogs/BackupUIManager.java

@@ -8,13 +8,13 @@
 import javafx.scene.control.ButtonType;
 
 import org.jabref.gui.DialogService;
+import org.jabref.gui.autosaveandbackup.BackupManager;
 import org.jabref.gui.backup.BackupResolverDialog;
 import org.jabref.gui.collab.DatabaseChange;
 import org.jabref.gui.collab.DatabaseChangeList;
 import org.jabref.gui.collab.DatabaseChangeResolverFactory;
 import org.jabref.gui.collab.DatabaseChangesResolverDialog;
 import org.jabref.gui.util.DefaultTaskExecutor;
-import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.logic.importer.OpenDatabase;
 import org.jabref.logic.importer.ParserResult;

src/main/java/org/jabref/gui/entryeditor/RequiredFieldsTab.java

@@ -56,7 +56,7 @@ protected Set<Field> determineFieldsToShow(BibEntry entry) {
         Set<Field> fields = new LinkedHashSet<>();
         if (entryType.isPresent()) {
             for (OrFields orFields : entryType.get().getRequiredFields()) {
-                fields.addAll(orFields);
+                fields.addAll(orFields.getFields());
             }
             // Add the edit field for Bibtex-key.
             fields.add(InternalField.KEY_FIELD);

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

@@ -6,23 +6,27 @@
 import java.nio.charset.UnsupportedCharsetException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
 import javafx.scene.control.ButtonBar;
 import javafx.scene.control.ButtonType;
 import javafx.scene.control.DialogPane;
+import javafx.scene.control.TableColumn;
 import javafx.scene.layout.VBox;
 import javafx.scene.text.Text;
 
 import org.jabref.gui.DialogService;
 import org.jabref.gui.JabRefFrame;
 import org.jabref.gui.LibraryTab;
+import org.jabref.gui.autosaveandbackup.AutosaveManager;
+import org.jabref.gui.autosaveandbackup.BackupManager;
+import org.jabref.gui.maintable.BibEntryTableViewModel;
+import org.jabref.gui.maintable.columns.MainTableColumn;
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.gui.util.FileDialogConfiguration;
-import org.jabref.logic.autosaveandbackup.AutosaveManager;
-import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.exporter.AtomicFileWriter;
 import org.jabref.logic.exporter.BibDatabaseWriter;
 import org.jabref.logic.exporter.BibWriter;
@@ -38,6 +42,7 @@
 import org.jabref.model.database.event.ChangePropagation;
 import org.jabref.model.entry.BibEntryTypesManager;
 import org.jabref.model.metadata.SaveOrder;
+import org.jabref.model.metadata.SelfContainedSaveOrder;
 import org.jabref.preferences.PreferencesService;
 
 import org.slf4j.Logger;
@@ -90,11 +95,31 @@ public boolean saveAs(Path file) {
         return this.saveAs(file, SaveDatabaseMode.NORMAL);
     }
 
+    private SelfContainedSaveOrder getSaveOrder() {
+        return libraryTab.getBibDatabaseContext()
+                .getMetaData().getSaveOrder()
+                .map(so -> {
+                    if (so.getOrderType() == SaveOrder.OrderType.TABLE) {
+                        // We need to "flatten out" SaveOrder.OrderType.TABLE as BibWriter does not have access to preferences
+                        List<TableColumn<BibEntryTableViewModel, ?>> sortOrder = libraryTab.getMainTable().getSortOrder();
+                        return new SelfContainedSaveOrder(
+                                SaveOrder.OrderType.SPECIFIED,
+                                sortOrder.stream()
+                                         .filter(col -> col instanceof MainTableColumn<?>)
+                                         .map(column -> ((MainTableColumn<?>) column).getModel())
+                                         .flatMap(model -> model.getSortCriteria().stream())
+                                         .toList());
+                    } else {
+                        return SelfContainedSaveOrder.of(so);
+                    }
+                })
+                .orElse(SaveOrder.getDefaultSaveOrder());
+    }
+
     public void saveSelectedAsPlain() {
         askForSavePath().ifPresent(path -> {
             try {
-                saveDatabase(path, true, StandardCharsets.UTF_8, BibDatabaseWriter.SaveType.PLAIN_BIBTEX,
-                        libraryTab.getBibDatabaseContext().getMetaData().getSaveOrder().orElse(SaveOrder.getDefaultSaveOrder()));
+                saveDatabase(path, true, StandardCharsets.UTF_8, BibDatabaseWriter.SaveType.PLAIN_BIBTEX, getSaveOrder());
                 frame.getFileHistory().newFile(path);
                 dialogService.notify(Localization.lang("Saved selected to '%0'.", path.toString()));
             } catch (SaveException ex) {
@@ -211,9 +236,7 @@ private boolean save(Path targetPath, SaveDatabaseMode mode) {
             // Make sure to remember which encoding we used
             libraryTab.getBibDatabaseContext().getMetaData().setEncoding(encoding, ChangePropagation.DO_NOT_POST_EVENT);
 
-            // Save the database
-            boolean success = saveDatabase(targetPath, false, encoding, BibDatabaseWriter.SaveType.WITH_JABREF_META_DATA,
-                    libraryTab.getBibDatabaseContext().getMetaData().getSaveOrder().orElse(SaveOrder.getDefaultSaveOrder()));
+            boolean success = saveDatabase(targetPath, false, encoding, BibDatabaseWriter.SaveType.WITH_JABREF_META_DATA, getSaveOrder());
 
             if (success) {
                 libraryTab.getUndoManager().markUnchanged();
@@ -231,7 +254,7 @@ private boolean save(Path targetPath, SaveDatabaseMode mode) {
         }
     }
 
-    private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding, BibDatabaseWriter.SaveType saveType, SaveOrder saveOrder) throws SaveException {
+    private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding, BibDatabaseWriter.SaveType saveType, SelfContainedSaveOrder saveOrder) throws SaveException {
         // if this code is adapted, please also adapt org.jabref.logic.autosaveandbackup.BackupManager.performBackup
 
         SaveConfiguration saveConfiguration = new SaveConfiguration()
@@ -269,7 +292,7 @@ private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding,
         }
     }
 
-    private void saveWithDifferentEncoding(Path file, boolean selectedOnly, Charset encoding, Set<Character> encodingProblems, BibDatabaseWriter.SaveType saveType, SaveOrder saveOrder) throws SaveException {
+    private void saveWithDifferentEncoding(Path file, boolean selectedOnly, Charset encoding, Set<Character> encodingProblems, BibDatabaseWriter.SaveType saveType, SelfContainedSaveOrder saveOrder) throws SaveException {
         DialogPane pane = new DialogPane();
         VBox vbox = new VBox();
         vbox.getChildren().addAll(

src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java

@@ -17,13 +17,13 @@
 import org.jabref.gui.LibraryTab;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
+import org.jabref.gui.autosaveandbackup.BackupManager;
 import org.jabref.gui.dialogs.BackupUIManager;
 import org.jabref.gui.menus.FileHistoryMenu;
 import org.jabref.gui.shared.SharedDatabaseUIManager;
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.gui.util.FileDialogConfiguration;
-import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.importer.OpenDatabase;
 import org.jabref.logic.importer.ParserResult;
 import org.jabref.logic.l10n.Localization;

src/main/java/org/jabref/gui/libraryproperties/contentselectors/ContentSelectorViewModel.java

@@ -5,6 +5,7 @@
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -67,12 +68,35 @@ public void setValues() {
     @Override
     public void storeSettings() {
         List<Field> metaDataFields = metaData.getContentSelectors().getFieldsWithSelectors();
+
+        if (isDefaultMap(fieldKeywordsMap)) {
+            Iterator<ContentSelector> iterator = metaData.getContentSelectors().getContentSelectors().iterator();
+            while (iterator.hasNext()) {
+                metaData.clearContentSelectors(iterator.next().getField());
+            }
+        }
+
         fieldKeywordsMap.forEach((field, keywords) -> updateMetaDataContentSelector(metaDataFields, field, keywords));
 
         List<Field> fieldNamesToRemove = filterFieldsToRemove();
         fieldNamesToRemove.forEach(metaData::clearContentSelectors);
     }
 
+    private boolean isDefaultMap(Map<Field, List<String>> fieldKeywordsMap) {
+        if (fieldKeywordsMap.size() != DEFAULT_FIELD_NAMES.size()) {
+            return false;
+        }
+        for (Field field : DEFAULT_FIELD_NAMES) {
+            if (!fieldKeywordsMap.containsKey(field)) {
+                return false;
+            }
+            if (!fieldKeywordsMap.get(field).isEmpty()) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     public ListProperty<Field> getFieldNamesBackingList() {
         return fields;
     }