Skip to content

Commit

Permalink
Improve calculation of matched entries upon change (#6268)
Browse files Browse the repository at this point in the history 
Instead of recalculating the number of matched entries for every group (which means `#groups * #entries` operations) now a change triggers a more intelligent re-calculation which needs at maximum `#groups` operations. This gives a huge boost and makes editing entries smooth even for large databases with many groups.
  • Loading branch information
@tobiasdiez
tobiasdiez authored Apr 11, 2020
1 parent d4396ce commit 1c724f2
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 50 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ 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 sort by priority was broken. [#6222](https://github.com/JabRef/jabref/issues/6222)
- 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)
- We fixe an issue where custom jstyles for Open/LibreOffice where not saved correctly. [#6170](https://github.com/JabRef/jabref/issues/6170)


### Removed
Expand Down
52 changes: 38 additions & 14 deletions src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -184,7 +180,7 @@ public String toString() {
", children=" + children +
", databaseContext=" + databaseContext +
", groupNode=" + groupNode +
", hits=" + hits +
", matchedEntries=" + matchedEntries +
'}';
}

Expand Down Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/model/entry/BibEntry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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));
Expand Down
18 changes: 10 additions & 8 deletions src/main/java/org/jabref/model/groups/GroupTreeNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

/**
Expand Down
48 changes: 24 additions & 24 deletions src/test/java/org/jabref/model/groups/GroupTreeNodeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -118,7 +118,7 @@ public void setUp() throws Exception {
}

/*
public GroupTreeNode getNodeInComplexTree() {
GroupTreeNode getNodeInComplexTree() {
return getNodeInComplexTree(new TreeNodeMock());
}
*/
Expand All @@ -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, ','));
Expand All @@ -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, ','));

Expand All @@ -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, ',');

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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());
Expand Down

0 comments on commit 1c724f2

Please sign in to comment.