Skip to content

Commit

Permalink
Check group type before showing dialog in edit group (JabRef#8909)
Browse files Browse the repository at this point in the history 
* Fix JabRef#8189 by checking group type

* Update different solution that is capable to prevent warning dialog without resetting to ExplitGroup

* Split groupType checking and groupFields checking into 2 different functions

* add check before dialog

* write groups always

* refersh

* store group changes

* rfactor code

* refactor with intanceof

* refactor

* fix stupid instanceof error^^

* checkstyle

* use getclass everywhere add comment

* Change or to and, only return true if no changes
add tests

* fuu checkstyle

* javadoc checkstyle

* TODO: clarifiy behavior when changing the groups name

not really sure what happns when you have more than 2 groups

* automatically assign when more than one group

* use new name

* todo

* we need to check old group name
rename methods

* checkstyle

* Only check for minor mods if group types are equal

Co-authored-by: Siedlerchr <[email protected]>
  • Loading branch information
@LIM0000 @Siedlerchr
LIM0000 and Siedlerchr authored Oct 10, 2022
1 parent a114ddc commit 052fdf5
Show file tree
Hide file tree
Showing 3 changed files with 194 additions and 37 deletions.
157 changes: 130 additions & 27 deletions src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,13 @@
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.groups.AbstractGroup;
import org.jabref.model.groups.AutomaticKeywordGroup;
import org.jabref.model.groups.AutomaticPersonsGroup;
import org.jabref.model.groups.ExplicitGroup;
import org.jabref.model.groups.GroupTreeNode;
import org.jabref.model.groups.RegexKeywordGroup;
import org.jabref.model.groups.SearchGroup;
import org.jabref.model.groups.TexGroup;
import org.jabref.model.groups.WordKeywordGroup;
import org.jabref.model.metadata.MetaData;
import org.jabref.preferences.PreferencesService;
Expand Down Expand Up @@ -145,14 +150,12 @@ private void onActiveDatabaseChanged(Optional<BibDatabaseContext> newDatabase) {
} else {
rootGroup.setValue(null);
}

currentDatabase = newDatabase;
}

/**
* Opens "New Group Dialog" and add the resulting group to the specified group
*/

public void addNewSubgroup(GroupNodeViewModel parent, GroupDialogHeader groupDialogHeader) {
currentDatabase.ifPresent(database -> {
Optional<AbstractGroup> newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView(
Expand Down Expand Up @@ -182,61 +185,163 @@ private void writeGroupChangesToMetaData() {
currentDatabase.ifPresent(database -> database.getMetaData().setGroups(rootGroup.get().getGroupNode()));
}

private boolean isGroupTypeEqual(AbstractGroup oldGroup, AbstractGroup newGroup) {
return oldGroup.getClass().equals(newGroup.getClass());
}

/**
* Check if it is necessary to show a group modified, reassign entry dialog <br>
* Group name change is handled separately
*
* @param oldGroup Original Group
* @param newGroup Edited group
* @return true if just trivial modifications (e.g. color or description) or the relevant group properties are equal, false otherwise
*/
boolean onlyMinorChanges(AbstractGroup oldGroup, AbstractGroup newGroup) {
// we need to use getclass here because we have different subclass inheritance e.g. ExplicitGroup is a subclass of WordKeyWordGroup
if (oldGroup.getClass() == WordKeywordGroup.class) {
WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup;
WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup;

return Objects.equals(oldWordKeywordGroup.getSearchField().getName(), newWordKeywordGroup.getSearchField().getName())
&& Objects.equals(oldWordKeywordGroup.getSearchExpression(), newWordKeywordGroup.getSearchExpression())
&& Objects.equals(oldWordKeywordGroup.isCaseSensitive(), newWordKeywordGroup.isCaseSensitive());
} else if (oldGroup.getClass() == RegexKeywordGroup.class) {
RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup;
RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup;

return Objects.equals(oldRegexKeywordGroup.getSearchField().getName(), newRegexKeywordGroup.getSearchField().getName())
&& Objects.equals(oldRegexKeywordGroup.getSearchExpression(), newRegexKeywordGroup.getSearchExpression())
&& Objects.equals(oldRegexKeywordGroup.isCaseSensitive(), newRegexKeywordGroup.isCaseSensitive());
} else if ((oldGroup.getClass() == SearchGroup.class)) {
SearchGroup oldSearchGroup = (SearchGroup) oldGroup;
SearchGroup newSearchGroup = (SearchGroup) newGroup;

return Objects.equals(oldSearchGroup.getSearchExpression(), newSearchGroup.getSearchExpression())
&& Objects.equals(oldSearchGroup.getSearchFlags(), newSearchGroup.getSearchFlags());
} else if (oldGroup.getClass() == AutomaticKeywordGroup.class) {
AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup;
AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup;

return Objects.equals(oldAutomaticKeywordGroup.getKeywordDelimiter(), newAutomaticKeywordGroup.getKeywordDelimiter())
&& Objects.equals(oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter(), newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter())
&& Objects.equals(oldAutomaticKeywordGroup.getField().getName(), newAutomaticKeywordGroup.getField().getName());
} else if (oldGroup.getClass() == AutomaticPersonsGroup.class) {
AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup;
AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup;

return Objects.equals(oldAutomaticPersonsGroup.getField().getName(), newAutomaticPersonsGroup.getField().getName());
} else if (oldGroup.getClass() == TexGroup.class) {
TexGroup oldTexGroup = (TexGroup) oldGroup;
TexGroup newTexGroup = (TexGroup) newGroup;
return Objects.equals(oldTexGroup.getFilePath().toString(), newTexGroup.getFilePath().toString());
}
return true;
}

/**
* Opens "Edit Group Dialog" and changes the given group to the edited one.
*/
public void editGroup(GroupNodeViewModel oldGroup) {
currentDatabase.ifPresent(database -> {
Optional<AbstractGroup> newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView(
dialogService,
database,
preferences,
oldGroup.getGroupNode().getGroup(),
GroupDialogHeader.SUBGROUP));

dialogService,
database,
preferences,
oldGroup.getGroupNode().getGroup(),
GroupDialogHeader.SUBGROUP));
newGroup.ifPresent(group -> {
// TODO: Keep assignments

AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup();
String oldGroupName = oldGroupDef.getName();

boolean groupTypeEqual = isGroupTypeEqual(oldGroupDef, group);
boolean onlyMinorModifications = groupTypeEqual && onlyMinorChanges(oldGroupDef, group);

// dialog already warns us about this if the new group is named like another existing group
// We need to check if only the name changed as this is relevant for the entry's group field
if (groupTypeEqual && !group.getName().equals(oldGroupName) && onlyMinorModifications) {
int groupsWithSameName = 0;
Optional<GroupTreeNode> databaseRootGroup = currentDatabase.get().getMetaData().getGroups();
if (databaseRootGroup.isPresent()) {
// we need to check the old name for duplicates. If the new group name occurs more than once, it won't matter
groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(oldGroupName)).size();
}
boolean removePreviousAssignments = true;
// We found more than 2 groups, so we cannot simply remove old assignment
if (groupsWithSameName >= 2) {
removePreviousAssignments = false;
}

oldGroup.getGroupNode().setGroup(
group,
true,
removePreviousAssignments,
database.getEntries());

dialogService.notify(Localization.lang("Modified group \"%0\".", group.getName()));
writeGroupChangesToMetaData();
// This is ugly but we have no proper update mechanism in place to propagate the changes, so redraw everything
refresh();
return;
}

if (groupTypeEqual && onlyMinorChanges(oldGroup.getGroupNode().getGroup(), group)) {
oldGroup.getGroupNode().setGroup(
group,
true,
true,
database.getEntries());

writeGroupChangesToMetaData();
refresh();
return;
}

// Major modifications

String content = Localization.lang("Assign the original group's entries to this group?");
ButtonType keepAssignments = new ButtonType(Localization.lang("Assign"), ButtonBar.ButtonData.YES);
ButtonType removeAssignments = new ButtonType(Localization.lang("Do not assign"), ButtonBar.ButtonData.NO);
ButtonType cancel = new ButtonType(Localization.lang("Cancel"), ButtonBar.ButtonData.CANCEL_CLOSE);

if (newGroup.get().getClass() == WordKeywordGroup.class) {
content = content + "\n\n" +
Localization.lang("(Note: If original entries lack keywords to qualify for the new group configuration, confirming here will add them)");
Localization.lang("(Note: If original entries lack keywords to qualify for the new group configuration, confirming here will add them)");
}
Optional<ButtonType> previousAssignments = dialogService.showCustomButtonDialogAndWait(Alert.AlertType.WARNING,
Localization.lang("Change of Grouping Method"),
content,
keepAssignments,
removeAssignments,
cancel);
// WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame());
Localization.lang("Change of Grouping Method"),
content,
keepAssignments,
removeAssignments,
cancel);
boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup)
&& (group instanceof ExplicitGroup);
&& (group instanceof ExplicitGroup);

int groupsWithSameName = 0;
Optional<GroupTreeNode> databaseRootGroup = currentDatabase.get().getMetaData().getGroups();
if (databaseRootGroup.isPresent()) {
String name = oldGroup.getGroupNode().getGroup().getName();
groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size();
}
// okay we found more than 2 groups with the same name
// If we only found one we can still do it
if (groupsWithSameName >= 2) {
removePreviousAssignments = false;
}

if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.YES)) {
oldGroup.getGroupNode().setGroup(
group,
true,
removePreviousAssignments,
database.getEntries());
group,
true,
removePreviousAssignments,
database.getEntries());
} else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.NO)) {
oldGroup.getGroupNode().setGroup(
group,
false,
removePreviousAssignments,
database.getEntries());
group,
false,
removePreviousAssignments,
database.getEntries());
} else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.CANCEL_CLOSE)) {
return;
}
Expand All @@ -259,10 +364,8 @@ public void editGroup(GroupNodeViewModel oldGroup) {
// if (!addChange.isEmpty()) {
// undoAddPreviousEntries = UndoableChangeEntriesOfGroup.getUndoableEdit(null, addChange);
// }

dialogService.notify(Localization.lang("Modified group \"%0\".", group.getName()));
writeGroupChangesToMetaData();

// This is ugly but we have no proper update mechanism in place to propagate the changes, so redraw everything
refresh();
});
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/org/jabref/model/groups/GroupTreeNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,17 @@ public List<FieldChange> setGroup(AbstractGroup newGroup, boolean shouldKeepPrev
group = Objects.requireNonNull(newGroup);

List<FieldChange> changes = new ArrayList<>();
boolean shouldRemove = shouldRemovePreviousAssignments && (oldGroup instanceof GroupEntryChanger);
boolean shouldAdd = shouldKeepPreviousAssignments && (newGroup instanceof GroupEntryChanger);
if (shouldAdd || shouldRemove) {
boolean shouldRemoveFromOldGroup = shouldRemovePreviousAssignments && (oldGroup instanceof GroupEntryChanger);
boolean shouldAddToNewGroup = shouldKeepPreviousAssignments && (newGroup instanceof GroupEntryChanger);
if (shouldAddToNewGroup || shouldRemoveFromOldGroup) {
List<BibEntry> entriesMatchedByOldGroup = entriesInDatabase.stream().filter(oldGroup::isMatch)
.collect(Collectors.toList());
if (shouldRemove) {
if (shouldRemoveFromOldGroup) {
GroupEntryChanger entryChanger = (GroupEntryChanger) oldGroup;
changes.addAll(entryChanger.remove(entriesMatchedByOldGroup));
}

if (shouldAdd) {
if (shouldAddToNewGroup) {
GroupEntryChanger entryChanger = (GroupEntryChanger) newGroup;
changes.addAll(entryChanger.add(entriesMatchedByOldGroup));
}
Expand Down
64 changes: 59 additions & 5 deletions src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.field.StandardField;
import org.jabref.model.groups.AbstractGroup;
import org.jabref.model.groups.AllEntriesGroup;
import org.jabref.model.groups.ExplicitGroup;
import org.jabref.model.groups.GroupHierarchyType;
Expand All @@ -20,17 +21,22 @@

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Answers;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class GroupTreeViewModelTest {

private StateManager stateManager;
private GroupTreeViewModel groupTree;
private BibDatabaseContext databaseContext;
private TaskExecutor taskExecutor;
private PreferencesService preferencesService;
private DialogService dialogService;

@BeforeEach
void setUp() {
Expand All @@ -39,12 +45,13 @@ void setUp() {
stateManager.activeDatabaseProperty().setValue(Optional.of(databaseContext));
taskExecutor = new CurrentThreadTaskExecutor();
preferencesService = mock(PreferencesService.class);
dialogService = mock(DialogService.class, Answers.RETURNS_DEEP_STUBS);

when(preferencesService.getGroupsPreferences()).thenReturn(new GroupsPreferences(
GroupViewMode.UNION,
true,
true,
new SimpleObjectProperty<>(',')
));
GroupViewMode.UNION,
true,
true,
new SimpleObjectProperty<>(',')));
groupTree = new GroupTreeViewModel(stateManager, mock(DialogService.class), preferencesService, taskExecutor, new CustomLocalDragboard());
}

Expand Down Expand Up @@ -85,4 +92,51 @@ void keywordGroupsAreNotRemovedFromEntriesOnDelete() {

assertEquals(groupName, entry.getField(StandardField.KEYWORDS).get());
}

@Test
void shouldNotShowDialogWhenGroupNameChanges() {
AbstractGroup oldGroup = new ExplicitGroup("group", GroupHierarchyType.INDEPENDENT, ',');
AbstractGroup newGroup = new ExplicitGroup("newGroupName", GroupHierarchyType.INDEPENDENT, ',');
BibEntry entry = new BibEntry();
databaseContext.getDatabase().insertEntry(entry);

GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard());
assertTrue(model.onlyMinorChanges(oldGroup, newGroup));
}

@Test
void shouldNotShowDialogWhenGroupsAreEqual() {
AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true);
AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true);

BibEntry entry = new BibEntry();
databaseContext.getDatabase().insertEntry(entry);

GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard());
assertTrue(model.onlyMinorChanges(oldGroup, newGroup));
}

@Test
void shouldShowDialogWhenKeywordDiffers() {
AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true);
AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordChanged", true, ',', true);

BibEntry entry = new BibEntry();
databaseContext.getDatabase().insertEntry(entry);

GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard());
assertFalse(model.onlyMinorChanges(oldGroup, newGroup));
}

@Test
void shouldShowDialogWhenCaseSensitivyDiffers() {
AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", false, ',', true);
AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordChanged", true, ',', true);

BibEntry entry = new BibEntry();
databaseContext.getDatabase().insertEntry(entry);

GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard());
assertFalse(model.onlyMinorChanges(oldGroup, newGroup));
}
}

0 comments on commit 052fdf5

Please sign in to comment.