Improve calculation of matched entries upon change

CHANGELOG.md

@@ -27,8 +27,9 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 ### Fixed
 
+- We greatly improved the performance of the overall application and many operations. [#5071](https://github.com/JabRef/jabref/issues/5071)
 - We fixed an issue where opening a library from the recent libraries menu was not possible. [#5939](https://github.com/JabRef/jabref/issues/5939)
-- We fixed an issue with inconsistent capitalization of file extensions when downloading files [#6115](https://github.com/JabRef/jabref/issues/6115)
+- We fixed an issue with inconsistent capitalization of file extensions when downloading files. [#6115](https://github.com/JabRef/jabref/issues/6115)
 - We fixed the display of language and encoding in the preferences dialog. [#6130](https://github.com/JabRef/jabref/pull/6130)
 - We fixed an issue where search full-text documents downloaded files with same name, overwriting existing files. [#6174](https://github.com/JabRef/jabref/pull/6174)
 - We fixe an issue where custom jstyles for Open/LibreOffice where not saved correctly [#6170](https://github.com/JabRef/jabref/issues/6170)

src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java

@@ -8,8 +8,8 @@
 
 import javafx.beans.binding.Bindings;
 import javafx.beans.binding.BooleanBinding;
+import javafx.beans.binding.IntegerBinding;
 import javafx.beans.property.SimpleBooleanProperty;
-import javafx.beans.property.SimpleIntegerProperty;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
@@ -28,7 +28,6 @@
 import org.jabref.gui.util.TaskExecutor;
 import org.jabref.logic.groups.DefaultGroupsFactory;
 import org.jabref.logic.layout.format.LatexToUnicodeFormatter;
-import org.jabref.logic.util.DelayTaskThrottler;
 import org.jabref.model.FieldChange;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
@@ -50,15 +49,14 @@ public class GroupNodeViewModel {
     private final BibDatabaseContext databaseContext;
     private final StateManager stateManager;
     private final GroupTreeNode groupNode;
-    private final SimpleIntegerProperty hits;
+    private final ObservableList<BibEntry> matchedEntries = FXCollections.observableArrayList();
     private final SimpleBooleanProperty hasChildren;
     private final SimpleBooleanProperty expandedProperty = new SimpleBooleanProperty();
     private final BooleanBinding anySelectedEntriesMatched;
     private final BooleanBinding allSelectedEntriesMatched;
     private final TaskExecutor taskExecutor;
     private final CustomLocalDragboard localDragBoard;
     private final ObservableList<BibEntry> entriesList;
-    private final DelayTaskThrottler throttler;
 
     public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager stateManager, TaskExecutor taskExecutor, GroupTreeNode groupNode, CustomLocalDragboard localDragBoard) {
         this.databaseContext = Objects.requireNonNull(databaseContext);
@@ -82,16 +80,14 @@ public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager state
         }
         hasChildren = new SimpleBooleanProperty();
         hasChildren.bind(Bindings.isNotEmpty(children));
-        hits = new SimpleIntegerProperty(0);
-        calculateNumberOfMatches();
+        updateMatchedEntries();
         expandedProperty.set(groupNode.getGroup().isExpanded());
         expandedProperty.addListener((observable, oldValue, newValue) -> groupNode.getGroup().setExpanded(newValue));
 
         // Register listener
         // The wrapper created by the FXCollections will set a weak listener on the wrapped list. This weak listener gets garbage collected. Hence, we need to maintain a reference to this list.
         entriesList = databaseContext.getDatabase().getEntries();
         entriesList.addListener(this::onDatabaseChanged);
-        throttler = taskExecutor.createThrottler(1000);
 
         ObservableList<Boolean> selectedEntriesMatchStatus = EasyBind.map(stateManager.getSelectedEntries(), groupNode::matches);
         anySelectedEntriesMatched = BindingsHelper.any(selectedEntriesMatchStatus, matched -> matched);
@@ -157,8 +153,8 @@ public String getDescription() {
         return groupNode.getGroup().getDescription().orElse("");
     }
 
-    public SimpleIntegerProperty getHits() {
-        return hits;
+    public IntegerBinding getHits() {
+        return Bindings.size(matchedEntries);
     }
 
     @Override
@@ -184,7 +180,7 @@ public String toString() {
                 ", children=" + children +
                 ", databaseContext=" + databaseContext +
                 ", groupNode=" + groupNode +
-                ", hits=" + hits +
+                ", matchedEntries=" + matchedEntries +
                 '}';
     }
 
@@ -222,16 +218,44 @@ public GroupTreeNode getGroupNode() {
      * Gets invoked if an entry in the current database changes.
      */
     private void onDatabaseChanged(ListChangeListener.Change<? extends BibEntry> change) {
-        throttler.schedule(this::calculateNumberOfMatches);
+        while (change.next()) {
+            if (change.wasPermutated()) {
+                // Nothing to do, as permutation doesn't change matched entries
+            } else if (change.wasUpdated()) {
+                for (BibEntry changedEntry : change.getList().subList(change.getFrom(), change.getTo())) {
+                    if (groupNode.matches(changedEntry)) {
+                        if (!matchedEntries.contains(changedEntry)) {
+                            matchedEntries.add(changedEntry);
+                        }
+                    } else {
+                        matchedEntries.remove(changedEntry);
+                    }
+                }
+            } else {
+                for (BibEntry removedEntry : change.getRemoved()) {
+                    matchedEntries.remove(removedEntry);
+                }
+                for (BibEntry addedEntry : change.getAddedSubList()) {
+                    if (groupNode.matches(addedEntry)) {
+                        if (!matchedEntries.contains(addedEntry)) {
+                            matchedEntries.add(addedEntry);
+                        }
+                    }
+                }
+            }
+        }
     }
 
-    private void calculateNumberOfMatches() {
+    private void updateMatchedEntries() {
         // We calculate the new hit value
         // We could be more intelligent and try to figure out the new number of hits based on the entry change
         // for example, a previously matched entry gets removed -> hits = hits - 1
         BackgroundTask
-                .wrap(() -> groupNode.calculateNumberOfMatches(databaseContext.getDatabase()))
-                .onSuccess(hits::setValue)
+                .wrap(() -> groupNode.findMatches(databaseContext.getDatabase()))
+                .onSuccess(entries -> {
+                    matchedEntries.clear();
+                    matchedEntries.addAll(entries);
+                })
                 .executeWith(taskExecutor);
     }
 

src/main/java/org/jabref/model/entry/BibEntry.java

@@ -528,8 +528,8 @@ public Optional<FieldChange> setField(Field field, String value, EntriesEventSou
 
         changed = true;
 
-        fields.put(field, value.intern());
         invalidateFieldCache(field);
+        fields.put(field, value.intern());
 
         FieldChange change = new FieldChange(this, field, oldValue, value);
         if (isNewField) {
@@ -574,8 +574,8 @@ public Optional<FieldChange> clearField(Field field, EntriesEventSource eventSou
 
         changed = true;
 
-        fields.remove(field);
         invalidateFieldCache(field);
+        fields.remove(field);
 
         FieldChange change = new FieldChange(this, field, oldValue.get(), null);
         eventBus.post(new FieldAddedOrRemovedEvent(change, eventSource));

src/main/java/org/jabref/model/groups/GroupTreeNode.java

@@ -218,24 +218,26 @@ public GroupTreeNode copyNode() {
     }
 
     /**
-     * Determines the number of entries in the specified list which are matched by this group.
+     * Determines the entries in the specified list which are matched by this group.
+     *
      * @param entries list of entries to be searched
-     * @return number of hits
+     * @return matched entries
      */
-    public long calculateNumberOfMatches(List<BibEntry> entries) {
+    public List<BibEntry> findMatches(List<BibEntry> entries) {
         SearchMatcher matcher = getSearchMatcher();
         return entries.stream()
                       .filter(matcher::isMatch)
-                      .count();
+                      .collect(Collectors.toList());
     }
 
     /**
-     * Determines the number of entries in the specified database which are matched by this group.
+     * Determines the entries in the specified database which are matched by this group.
+     *
      * @param database database to be searched
-     * @return number of hits
+     * @return matched entries
      */
-    public long calculateNumberOfMatches(BibDatabase database) {
-        return calculateNumberOfMatches(database.getEntries());
+    public List<BibEntry> findMatches(BibDatabase database) {
+        return findMatches(database.getEntries());
     }
 
     /**

src/test/java/org/jabref/model/groups/GroupTreeNodeTest.java

@@ -95,7 +95,7 @@ private static AbstractGroup getExplict(String name) {
      *      A
      */
     /*
-    public GroupTreeNode getNodeAsChild(TreeNodeMock root) {
+    GroupTreeNode getNodeAsChild(TreeNodeMock root) {
         root.addChild(new TreeNodeMock());
         root.addChild(new TreeNodeMock());
         TreeNodeMock node = new TreeNodeMock();
@@ -109,7 +109,7 @@ public static GroupTreeNode getRoot() {
     }
 
     @BeforeEach
-    public void setUp() throws Exception {
+    void setUp() throws Exception {
         entries.clear();
         entry = new BibEntry();
         entries.add(entry);
@@ -118,7 +118,7 @@ public void setUp() throws Exception {
     }
 
     /*
-    public GroupTreeNode getNodeInComplexTree() {
+    GroupTreeNode getNodeInComplexTree() {
         return getNodeInComplexTree(new TreeNodeMock());
     }
     */
@@ -128,14 +128,14 @@ private GroupTreeNode getNodeInSimpleTree() {
     }
 
     @Test
-    public void getSearchRuleForIndependentGroupReturnsGroupAsMatcher() {
+    void getSearchRuleForIndependentGroupReturnsGroupAsMatcher() {
         GroupTreeNode node = GroupTreeNode
                 .fromGroup(new ExplicitGroup("node", GroupHierarchyType.INDEPENDENT, ','));
         assertEquals(node.getGroup(), node.getSearchMatcher());
     }
 
     @Test
-    public void getSearchRuleForRefiningGroupReturnsParentAndGroupAsMatcher() {
+    void getSearchRuleForRefiningGroupReturnsParentAndGroupAsMatcher() {
         GroupTreeNode parent = GroupTreeNode
                 .fromGroup(
                         new ExplicitGroup("parent", GroupHierarchyType.INDEPENDENT, ','));
@@ -149,7 +149,7 @@ public void getSearchRuleForRefiningGroupReturnsParentAndGroupAsMatcher() {
     }
 
     @Test
-    public void getSearchRuleForIncludingGroupReturnsGroupOrSubgroupAsMatcher() {
+    void getSearchRuleForIncludingGroupReturnsGroupOrSubgroupAsMatcher() {
         GroupTreeNode node = GroupTreeNode.fromGroup(new ExplicitGroup("node", GroupHierarchyType.INCLUDING, ','));
         GroupTreeNode child = node.addSubgroup(new ExplicitGroup("child", GroupHierarchyType.INDEPENDENT, ','));
 
@@ -160,48 +160,48 @@ public void getSearchRuleForIncludingGroupReturnsGroupOrSubgroupAsMatcher() {
     }
 
     @Test
-    public void numberOfHitsReturnsZeroForEmptyList() throws Exception {
-        assertEquals(0, getNodeInSimpleTree().calculateNumberOfMatches(Collections.emptyList()));
+    void findMatchesReturnsEmptyForEmptyList() throws Exception {
+        assertEquals(Collections.emptyList(), getNodeInSimpleTree().findMatches(Collections.emptyList()));
     }
 
     @Test
-    public void numberOfHitsMatchesOneEntry() throws Exception {
+    void findMatchesOneEntry() throws Exception {
         GroupTreeNode parent = getNodeInSimpleTree();
         GroupTreeNode node = parent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.INDEPENDENT, StandardField.AUTHOR, "author2", true, ',', false));
-        assertEquals(1, node.calculateNumberOfMatches(entries));
+        assertEquals(1, node.findMatches(entries).size());
     }
 
     @Test
-    public void numberOfHitsMatchesMultipleEntries() throws Exception {
+    void findMatchesMultipleEntries() throws Exception {
         GroupTreeNode parent = getNodeInSimpleTree();
         GroupTreeNode node = parent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.INDEPENDENT, StandardField.AUTHOR, "author1", true, ',', false));
-        assertEquals(2, node.calculateNumberOfMatches(entries));
+        assertEquals(2, node.findMatches(entries).size());
     }
 
     @Test
-    public void numberOfHitsWorksForRefiningGroups() throws Exception {
+    void findMatchesWorksForRefiningGroups() throws Exception {
         GroupTreeNode grandParent = getNodeInSimpleTree();
         GroupTreeNode parent = grandParent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.INDEPENDENT, StandardField.AUTHOR, "author2", true, ',', false));
         GroupTreeNode node = parent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.REFINING, StandardField.AUTHOR, "author1", true, ',', false));
-        assertEquals(1, node.calculateNumberOfMatches(entries));
+        assertEquals(1, node.findMatches(entries).size());
     }
 
     @Test
-    public void numberOfHitsWorksForHierarchyOfIndependentGroups() throws Exception {
+    void findMatchesWorksForHierarchyOfIndependentGroups() throws Exception {
         GroupTreeNode grandParent = getNodeInSimpleTree();
         GroupTreeNode parent = grandParent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.INDEPENDENT, StandardField.AUTHOR, "author2", true, ',', false));
         GroupTreeNode node = parent.addSubgroup(
                 new WordKeywordGroup("node", GroupHierarchyType.INDEPENDENT, StandardField.AUTHOR, "author1", true, ',', false));
-        assertEquals(2, node.calculateNumberOfMatches(entries));
+        assertEquals(2, node.findMatches(entries).size());
     }
 
     @Test
-    public void setGroupChangesUnderlyingGroup() throws Exception {
+    void setGroupChangesUnderlyingGroup() throws Exception {
         GroupTreeNode node = getNodeInSimpleTree();
         AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT, ',');
 
@@ -211,7 +211,7 @@ public void setGroupChangesUnderlyingGroup() throws Exception {
     }
 
     @Test
-    public void setGroupAddsPreviousAssignmentsExplicitToExplicit() throws Exception {
+    void setGroupAddsPreviousAssignmentsExplicitToExplicit() throws Exception {
         ExplicitGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT, ',');
         oldGroup.add(entry);
         GroupTreeNode node = GroupTreeNode.fromGroup(oldGroup);
@@ -223,7 +223,7 @@ public void setGroupAddsPreviousAssignmentsExplicitToExplicit() throws Exception
     }
 
     @Test
-    public void setGroupWithFalseDoesNotAddsPreviousAssignments() throws Exception {
+    void setGroupWithFalseDoesNotAddsPreviousAssignments() throws Exception {
         ExplicitGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT, ',');
         oldGroup.add(entry);
         GroupTreeNode node = GroupTreeNode.fromGroup(oldGroup);
@@ -235,7 +235,7 @@ public void setGroupWithFalseDoesNotAddsPreviousAssignments() throws Exception {
     }
 
     @Test
-    public void setGroupAddsOnlyPreviousAssignments() throws Exception {
+    void setGroupAddsOnlyPreviousAssignments() throws Exception {
         ExplicitGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT, ',');
         assertFalse(oldGroup.isMatch(entry));
         GroupTreeNode node = GroupTreeNode.fromGroup(oldGroup);
@@ -247,7 +247,7 @@ public void setGroupAddsOnlyPreviousAssignments() throws Exception {
     }
 
     @Test
-    public void setGroupExplicitToSearchDoesNotKeepPreviousAssignments() throws Exception {
+    void setGroupExplicitToSearchDoesNotKeepPreviousAssignments() throws Exception {
         ExplicitGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT, ',');
         oldGroup.add(entry);
         GroupTreeNode node = GroupTreeNode.fromGroup(oldGroup);
@@ -259,7 +259,7 @@ public void setGroupExplicitToSearchDoesNotKeepPreviousAssignments() throws Exce
     }
 
     @Test
-    public void setGroupExplicitToExplicitIsRenameAndSoRemovesPreviousAssignment() throws Exception {
+    void setGroupExplicitToExplicitIsRenameAndSoRemovesPreviousAssignment() throws Exception {
         ExplicitGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT, ',');
         oldGroup.add(entry);
         GroupTreeNode node = GroupTreeNode.fromGroup(oldGroup);
@@ -271,15 +271,15 @@ public void setGroupExplicitToExplicitIsRenameAndSoRemovesPreviousAssignment() t
     }
 
     @Test
-    public void getChildByPathFindsCorrectChildInSecondLevel() throws Exception {
+    void getChildByPathFindsCorrectChildInSecondLevel() throws Exception {
         GroupTreeNode root = getRoot();
         GroupTreeNode child = getNodeInSimpleTree(root);
 
         assertEquals(Optional.of(child), root.getChildByPath("ExplicitParent > ExplicitNode"));
     }
 
     @Test
-    public void getPathSimpleTree() throws Exception {
+    void getPathSimpleTree() throws Exception {
         GroupTreeNode node = getNodeInSimpleTree();
 
         assertEquals("ExplicitParent > ExplicitNode", node.getPath());