Skip to content

Commit

Permalink
Fix JabRef#1500: Renaming of explicit groups changes entries accordingly
Browse files Browse the repository at this point in the history
  • Loading branch information
@tobiasdiez
tobiasdiez committed Jul 8, 2016
1 parent fcebf46 commit ba82614
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 79 deletions.
76 changes: 2 additions & 74 deletions src/main/java/net/sf/jabref/gui/groups/GroupDialog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ItemListener;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;

Expand All @@ -37,15 +34,13 @@
import javax.swing.JComponent;
import javax.swing.JDialog;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JRadioButton;
import javax.swing.JScrollPane;
import javax.swing.JTextField;
import javax.swing.ScrollPaneConstants;
import javax.swing.SwingConstants;
import javax.swing.event.CaretListener;
import javax.swing.undo.AbstractUndoableEdit;

import net.sf.jabref.Globals;
import net.sf.jabref.JabRefPreferences;
Expand All @@ -56,16 +51,13 @@
import net.sf.jabref.gui.keyboard.KeyBinding;
import net.sf.jabref.importer.fileformat.ParseException;
import net.sf.jabref.logic.groups.AbstractGroup;
import net.sf.jabref.logic.groups.EntriesGroupChange;
import net.sf.jabref.logic.groups.ExplicitGroup;
import net.sf.jabref.logic.groups.GroupHierarchyType;
import net.sf.jabref.logic.groups.KeywordGroup;
import net.sf.jabref.logic.groups.SearchGroup;
import net.sf.jabref.logic.l10n.Localization;
import net.sf.jabref.logic.search.SearchQuery;
import net.sf.jabref.logic.util.strings.StringUtil;
import net.sf.jabref.model.entry.BibEntry;
import net.sf.jabref.util.Util;

import com.jgoodies.forms.builder.ButtonBarBuilder;
import com.jgoodies.forms.builder.DefaultFormBuilder;
Expand Down Expand Up @@ -121,14 +113,8 @@ public Dimension getPreferredSize() {

private boolean mOkPressed;

private final BasePanel m_basePanel;

private AbstractGroup mResultingGroup;

private AbstractUndoableEdit mUndoAddPreviousEntires;

private final AbstractGroup m_editedGroup;

private final CardLayout m_optionsLayout = new CardLayout();

/**
Expand All @@ -142,8 +128,6 @@ public Dimension getPreferredSize() {
public GroupDialog(JabRefFrame jabrefFrame, BasePanel basePanel,
AbstractGroup editedGroup) {
super(jabrefFrame, Localization.lang("Edit group"), true);
m_basePanel = basePanel;
m_editedGroup = editedGroup;

// set default values (overwritten if editedGroup != null)
m_kgSearchField.setText(jabrefFrame.prefs().get(JabRefPreferences.GROUPS_DEFAULT_FIELD));
Expand Down Expand Up @@ -173,7 +157,7 @@ public GroupDialog(JabRefFrame jabrefFrame, BasePanel basePanel,
builderKG.nextLine();
builderKG.append(Localization.lang("Keyword"));
builderKG.append(m_kgSearchTerm);
builderKG.append(new FieldContentSelector(jabrefFrame, m_basePanel, this,
builderKG.append(new FieldContentSelector(jabrefFrame, basePanel, this,
m_kgSearchTerm, null, true, ", "));
builderKG.nextLine();
builderKG.append(m_kgCaseSensitive, 3);
Expand Down Expand Up @@ -296,27 +280,13 @@ public void actionPerformed(ActionEvent e) {
mOkPressed = true;
try {
if (m_explicitRadioButton.isSelected()) {
if (m_editedGroup instanceof ExplicitGroup) {
// keep assignments from possible previous ExplicitGroup
mResultingGroup = m_editedGroup.deepCopy();
mResultingGroup.setName(m_name.getText().trim());
mResultingGroup.setHierarchicalContext(getContext());
} else {
mResultingGroup = new ExplicitGroup(m_name.getText().trim(), getContext());
if (m_editedGroup != null) {
addPreviousEntries();
}
}
mResultingGroup = new ExplicitGroup(m_name.getText().trim(), getContext());
} else if (m_keywordsRadioButton.isSelected()) {
// regex is correct, otherwise OK would have been disabled
// therefore I don't catch anything here
mResultingGroup = new KeywordGroup(m_name.getText().trim(), m_kgSearchField.getText().trim(),
m_kgSearchTerm.getText().trim(), m_kgCaseSensitive.isSelected(), m_kgRegExp.isSelected(),
getContext());
if (((m_editedGroup instanceof ExplicitGroup) || (m_editedGroup instanceof SearchGroup))
&& mResultingGroup.supportsAdd()) {
addPreviousEntries();
}
} else if (m_searchRadioButton.isSelected()) {
try {
// regex is correct, otherwise OK would have been
Expand Down Expand Up @@ -463,40 +433,6 @@ private boolean isCaseSensitive() {
return m_sgCaseSensitive.isSelected();
}

/**
* This is used when a group is converted and the new group supports
* explicit adding of entries: All entries that match the previous group are
* added to the new group.
*/
private void addPreviousEntries() {
int i = JOptionPane.showConfirmDialog(m_basePanel.frame(),
Localization.lang("Assign the original group's entries to this group?"),
Localization.lang("Change of Grouping Method"),
JOptionPane.YES_NO_OPTION, JOptionPane.QUESTION_MESSAGE);
if (i == JOptionPane.NO_OPTION) {
return;
}
List<BibEntry> list = new ArrayList<>();
for (BibEntry entry : m_basePanel.getDatabase().getEntries()) {
if (m_editedGroup.contains(entry)) {
list.add(entry);
}
}
if (!list.isEmpty()) {
if (!Util.warnAssignmentSideEffects(mResultingGroup, this)) {
return;
}
// the undo information for a conversion to an ExplicitGroup is
// contained completely in the UndoableModifyGroup object.
if (!(mResultingGroup instanceof ExplicitGroup) && mResultingGroup.supportsAdd()) {
Optional<EntriesGroupChange> addChange = mResultingGroup.add(list);
if(addChange.isPresent()) {
mUndoAddPreviousEntires = UndoableChangeEntriesOfGroup.getUndoableEdit(null, addChange.get());
}
}
}
}

private void setDescription(String description) {
m_description.setText("<html>" + description + "</html>");
}
Expand Down Expand Up @@ -527,14 +463,6 @@ private static String formatRegExException(String regExp, Exception e) {
return s;
}

/**
* Returns an undo object for adding the edited group's entries to the new
* group, or null if this did not occur.
*/
public AbstractUndoableEdit getUndoForAddPreviousEntries() {
return mUndoAddPreviousEntires;
}

/**
* Sets the font of the name entry field.
*/
Expand Down
21 changes: 19 additions & 2 deletions src/main/java/net/sf/jabref/gui/groups/GroupSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import net.sf.jabref.gui.worker.AbstractWorker;
import net.sf.jabref.logic.groups.AbstractGroup;
import net.sf.jabref.logic.groups.AllEntriesGroup;
import net.sf.jabref.logic.groups.EntriesGroupChange;
import net.sf.jabref.logic.groups.GroupTreeNode;
import net.sf.jabref.logic.groups.MoveGroupChange;
import net.sf.jabref.logic.l10n.Localization;
Expand All @@ -80,6 +81,7 @@
import net.sf.jabref.logic.search.matchers.MatcherSets;
import net.sf.jabref.logic.search.matchers.NotMatcher;
import net.sf.jabref.model.entry.BibEntry;
import net.sf.jabref.util.Util;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
Expand Down Expand Up @@ -787,10 +789,25 @@ public void actionPerformed(ActionEvent e) {
gd.setVisible(true);
if (gd.okPressed()) {
AbstractGroup newGroup = gd.getResultingGroup();
AbstractUndoableEdit undoAddPreviousEntries = gd.getUndoForAddPreviousEntries();

int i = JOptionPane.showConfirmDialog(panel.frame(),
Localization.lang("Assign the original group's entries to this group?"),
Localization.lang("Change of Grouping Method"),
JOptionPane.YES_NO_OPTION, JOptionPane.QUESTION_MESSAGE);
boolean keepPreviousAssignments = i == JOptionPane.YES_OPTION &&
Util.warnAssignmentSideEffects(newGroup, panel.frame());

AbstractUndoableEdit undoAddPreviousEntries = null;
UndoableModifyGroup undo = new UndoableModifyGroup(GroupSelector.this, groupsRoot, node, newGroup);
node.getNode().setGroup(newGroup);
Optional<EntriesGroupChange> addChange = node.getNode().setGroup(newGroup, keepPreviousAssignments,
panel.getDatabase().getEntries());
if (addChange.isPresent()) {
undoAddPreviousEntries = UndoableChangeEntriesOfGroup.getUndoableEdit(null, addChange.get());
}

groupsTreeModel.reload();
revalidateGroups(node);

// Store undo information.
if (undoAddPreviousEntries == null) {
panel.getUndoManager().addEdit(undo);
Expand Down
35 changes: 32 additions & 3 deletions src/main/java/net/sf/jabref/logic/groups/GroupTreeNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;

import net.sf.jabref.importer.fileformat.ParseException;
import net.sf.jabref.logic.search.SearchMatcher;
Expand Down Expand Up @@ -55,10 +57,37 @@ public AbstractGroup getGroup() {
/**
* Associates the specified group with this node.
*
* @param group the new group (has to be non-null)
* @param newGroup the new group (has to be non-null)
*/
public void setGroup(AbstractGroup group) {
this.group = Objects.requireNonNull(group);
@Deprecated // use other overload
public void setGroup(AbstractGroup newGroup) {
this.group = Objects.requireNonNull(newGroup);
}

/**
* Associates the specified group with this node while also providing the possibility to modify previous matched
* entries so that they are now matched by the new group.
*
* @param newGroup the new group (has to be non-null)
* @param shouldKeepPreviousAssignments specifies whether previous matched entries should be carried over
* @param entriesInDatabase list of entries in the database
*/
public Optional<EntriesGroupChange> setGroup(AbstractGroup newGroup, boolean shouldKeepPreviousAssignments,
List<BibEntry> entriesInDatabase) {
AbstractGroup oldGroup = getGroup();
setGroup(newGroup);

// Keep assignments from previous group
if (shouldKeepPreviousAssignments && newGroup.supportsAdd()) {
List<BibEntry> entriesMatchedByOldGroup = entriesInDatabase.stream().filter(oldGroup::isMatch)
.collect(Collectors.toList());
if (oldGroup instanceof ExplicitGroup && newGroup instanceof ExplicitGroup) {
// Rename of explicit group, so remove old group assignment
oldGroup.remove(entriesMatchedByOldGroup);
}
return newGroup.add(entriesMatchedByOldGroup);
}
return Optional.empty();
}

/**
Expand Down
80 changes: 80 additions & 0 deletions src/test/java/net/sf/jabref/logic/groups/GroupTreeNodeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import java.util.Collections;
import java.util.List;

import net.sf.jabref.Globals;
import net.sf.jabref.JabRefPreferences;
import net.sf.jabref.importer.fileformat.ParseException;
import net.sf.jabref.logic.search.matchers.AndMatcher;
import net.sf.jabref.logic.search.matchers.OrMatcher;
Expand All @@ -14,15 +16,23 @@
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class GroupTreeNodeTest {

private List<BibEntry> entries = new ArrayList<>();
private BibEntry entry;

@Before
public void setUp() throws Exception {
entries.clear();
entry = new BibEntry();
entries.add(entry);
entries.add(new BibEntry().withField("author", "author1 and author2"));
entries.add(new BibEntry().withField("author", "author1"));

Globals.prefs = JabRefPreferences.getInstance();
}


Expand Down Expand Up @@ -228,4 +238,74 @@ public void numberOfHitsWorksForHierarchyOfIndependentGroups() throws Exception
new KeywordGroup("node", "author", "author1", true, false, GroupHierarchyType.INDEPENDENT));
assertEquals(2, node.numberOfHits(entries));
}

@Test
public void setGroupChangesUnderlyingGroup() throws Exception {
GroupTreeNode node = getNodeInSimpleTree();
AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, true, entries);

assertEquals(newGroup, node.getGroup());
}

@Test
public void setGroupAddsPreviousAssignmentsExplicitToExplicit() throws Exception {
AbstractGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT);
oldGroup.add(entry);
GroupTreeNode node = new GroupTreeNode(oldGroup);
AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, true, entries);

assertTrue(newGroup.isMatch(entry));
}

@Test
public void setGroupWithFalseDoesNotAddsPreviousAssignments() throws Exception {
AbstractGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT);
oldGroup.add(entry);
GroupTreeNode node = new GroupTreeNode(oldGroup);
AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, false, entries);

assertFalse(newGroup.isMatch(entry));
}

@Test
public void setGroupAddsOnlyPreviousAssignments() throws Exception {
AbstractGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT);
assertFalse(oldGroup.isMatch(entry));
GroupTreeNode node = new GroupTreeNode(oldGroup);
AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, true, entries);

assertFalse(newGroup.isMatch(entry));
}

@Test
public void setGroupExplicitToSearchDoesNotKeepPreviousAssignments() throws Exception {
AbstractGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT);
oldGroup.add(entry);
GroupTreeNode node = new GroupTreeNode(oldGroup);
AbstractGroup newGroup = new SearchGroup("NewGroup", "test", false, false, GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, true, entries);

assertFalse(newGroup.isMatch(entry));
}

@Test
public void setGroupExplicitToExplicitIsRenameAndSoRemovesPreviousAssignment() throws Exception {
AbstractGroup oldGroup = new ExplicitGroup("OldGroup", GroupHierarchyType.INDEPENDENT);
oldGroup.add(entry);
GroupTreeNode node = new GroupTreeNode(oldGroup);
AbstractGroup newGroup = new ExplicitGroup("NewGroup", GroupHierarchyType.INDEPENDENT);

node.setGroup(newGroup, true, entries);

assertFalse(oldGroup.isMatch(entry));
}
}

0 comments on commit ba82614

Please sign in to comment.