From 4160f51222e9599bcee539be2ad48ac4b9f4a717 Mon Sep 17 00:00:00 2001 From: LIM0000 Date: Sun, 19 Jun 2022 16:22:02 +0930 Subject: [PATCH 01/23] Fix #8189 by checking group type --- .../java/org/jabref/gui/groups/GroupDialogViewModel.java | 4 +++- .../java/org/jabref/gui/groups/GroupTreeViewModel.java | 9 ++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java b/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java index 0a1de76104c..dab1efd9e63 100644 --- a/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java @@ -38,7 +38,6 @@ import org.jabref.model.entry.Keyword; import org.jabref.model.entry.field.FieldFactory; import org.jabref.model.groups.AbstractGroup; -import org.jabref.model.groups.AutomaticGroup; import org.jabref.model.groups.AutomaticKeywordGroup; import org.jabref.model.groups.AutomaticPersonsGroup; import org.jabref.model.groups.ExplicitGroup; @@ -386,7 +385,9 @@ public void setValues() { descriptionProperty.setValue(editedGroup.getDescription().orElse("")); iconProperty.setValue(editedGroup.getIconName().orElse("")); groupHierarchySelectedProperty.setValue(editedGroup.getHierarchicalContext()); + typeExplicitProperty.setValue(true); + /* if (editedGroup.getClass() == WordKeywordGroup.class) { typeKeywordsProperty.setValue(true); @@ -429,6 +430,7 @@ public void setValues() { TexGroup group = (TexGroup) editedGroup; texGroupFilePathProperty.setValue(group.getFilePath().toString()); } + */ } } diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 6ac27b40419..4cb518d292a 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -192,9 +192,12 @@ public void editGroup(GroupNodeViewModel oldGroup) { newGroup.ifPresent(group -> { // TODO: Keep assignments - boolean keepPreviousAssignments = dialogService.showConfirmationDialogAndWait( - Localization.lang("Change of Grouping Method"), - Localization.lang("Assign the original group's entries to this group?")); + boolean keepPreviousAssignments = true; + if (newGroup.get().getClass() != ExplicitGroup.class) { + keepPreviousAssignments = dialogService.showConfirmationDialogAndWait( + Localization.lang("Change of Grouping Method"), + Localization.lang("Assign the original group's entries to this group?")); + } // WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame()); boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) && (group instanceof ExplicitGroup); From ba4e486f41f48667b77f31ebaacbd12962568f5d Mon Sep 17 00:00:00 2001 From: LIM0000 Date: Tue, 21 Jun 2022 17:29:32 +0930 Subject: [PATCH 02/23] Update different solution that is capable to prevent warning dialog without resetting to ExplitGroup --- .../gui/groups/GroupDialogViewModel.java | 4 +- .../jabref/gui/groups/GroupTreeViewModel.java | 78 ++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java b/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java index dab1efd9e63..0a1de76104c 100644 --- a/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java @@ -38,6 +38,7 @@ import org.jabref.model.entry.Keyword; import org.jabref.model.entry.field.FieldFactory; import org.jabref.model.groups.AbstractGroup; +import org.jabref.model.groups.AutomaticGroup; import org.jabref.model.groups.AutomaticKeywordGroup; import org.jabref.model.groups.AutomaticPersonsGroup; import org.jabref.model.groups.ExplicitGroup; @@ -385,9 +386,7 @@ public void setValues() { descriptionProperty.setValue(editedGroup.getDescription().orElse("")); iconProperty.setValue(editedGroup.getIconName().orElse("")); groupHierarchySelectedProperty.setValue(editedGroup.getHierarchicalContext()); - typeExplicitProperty.setValue(true); - /* if (editedGroup.getClass() == WordKeywordGroup.class) { typeKeywordsProperty.setValue(true); @@ -430,7 +429,6 @@ public void setValues() { TexGroup group = (TexGroup) editedGroup; texGroupFilePathProperty.setValue(group.getFilePath().toString()); } - */ } } diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 4cb518d292a..51baccce97f 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -27,8 +27,15 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.groups.AbstractGroup; +import org.jabref.model.groups.AutomaticGroup; +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; @@ -178,6 +185,75 @@ private void writeGroupChangesToMetaData() { currentDatabase.ifPresent(database -> database.getMetaData().setGroups(rootGroup.get().getGroupNode())); } + private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) { + if (!oldGroup.getClass().equals(newGroup.getClass())) { + return false; + } + + if (oldGroup.getClass() == WordKeywordGroup.class) { + WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; + WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; + + if (!oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName())) { + return false; + } else if (!oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression())) { + return false; + } else if (oldWordKeywordGroup.isCaseSensitive() != newWordKeywordGroup.isCaseSensitive()) { + return false; + } + } else if (oldGroup.getClass() == RegexKeywordGroup.class) { + RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; + RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; + + if (!oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName())) { + return false; + } else if (!oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression())) { + return false; + } else if (oldRegexKeywordGroup.isCaseSensitive() != newRegexKeywordGroup.isCaseSensitive()) { + return false; + } + } else if (oldGroup.getClass() == SearchGroup.class) { + SearchGroup oldSearchGroup = (SearchGroup) oldGroup; + SearchGroup newSearchGroup = (SearchGroup) newGroup; + + if (!oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression())) { + return false; + } else if (!oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags())) { + return false; + } + } else if (oldGroup.getClass() == ExplicitGroup.class) { + return true; + } else if (oldGroup instanceof AutomaticGroup) { + if (oldGroup.getClass() == AutomaticKeywordGroup.class) { + AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; + AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) newGroup; + + if (!oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString())) { + return false; + } else if (!oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString())) { + return false; + } else if (!oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName())) { + return false; + } + } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { + AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; + AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; + + if (!oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName())) { + return false; + } + } + } else if (oldGroup.getClass() == TexGroup.class) { + TexGroup oldTexGroup = (TexGroup) oldGroup; + TexGroup newTexGroup = (TexGroup) newGroup; + + if (!oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString())) { + return false; + } + } + return true; + } + /** * Opens "Edit Group Dialog" and changes the given group to the edited one. */ @@ -193,7 +269,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { newGroup.ifPresent(group -> { // TODO: Keep assignments boolean keepPreviousAssignments = true; - if (newGroup.get().getClass() != ExplicitGroup.class) { + if (!compareGroupType(oldGroup.getGroupNode().getGroup(), newGroup.get())) { keepPreviousAssignments = dialogService.showConfirmationDialogAndWait( Localization.lang("Change of Grouping Method"), Localization.lang("Assign the original group's entries to this group?")); From 556e954b997ee427b4b3291f8b20e8034317b0e7 Mon Sep 17 00:00:00 2001 From: LIM0000 Date: Fri, 24 Jun 2022 16:25:35 +0930 Subject: [PATCH 03/23] Split groupType checking and groupFields checking into 2 different functions --- .../jabref/gui/groups/GroupTreeViewModel.java | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 51baccce97f..50c79f7c084 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -186,10 +186,10 @@ private void writeGroupChangesToMetaData() { } private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) { - if (!oldGroup.getClass().equals(newGroup.getClass())) { - return false; - } + return oldGroup.getClass().equals(newGroup.getClass()); + } + private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGroup) { if (oldGroup.getClass() == WordKeywordGroup.class) { WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; @@ -198,8 +198,8 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) return false; } else if (!oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression())) { return false; - } else if (oldWordKeywordGroup.isCaseSensitive() != newWordKeywordGroup.isCaseSensitive()) { - return false; + } else { + return oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive(); } } else if (oldGroup.getClass() == RegexKeywordGroup.class) { RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; @@ -209,8 +209,8 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) return false; } else if (!oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression())) { return false; - } else if (oldRegexKeywordGroup.isCaseSensitive() != newRegexKeywordGroup.isCaseSensitive()) { - return false; + } else { + return oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive(); } } else if (oldGroup.getClass() == SearchGroup.class) { SearchGroup oldSearchGroup = (SearchGroup) oldGroup; @@ -218,8 +218,8 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) if (!oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression())) { return false; - } else if (!oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags())) { - return false; + } else { + return oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); } } else if (oldGroup.getClass() == ExplicitGroup.class) { return true; @@ -232,24 +232,20 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) return false; } else if (!oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString())) { return false; - } else if (!oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName())) { - return false; + } else { + return oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); } } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; - if (!oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName())) { - return false; - } + return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); } } else if (oldGroup.getClass() == TexGroup.class) { TexGroup oldTexGroup = (TexGroup) oldGroup; TexGroup newTexGroup = (TexGroup) newGroup; - if (!oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString())) { - return false; - } + return oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString()); } return true; } @@ -269,7 +265,8 @@ public void editGroup(GroupNodeViewModel oldGroup) { newGroup.ifPresent(group -> { // TODO: Keep assignments boolean keepPreviousAssignments = true; - if (!compareGroupType(oldGroup.getGroupNode().getGroup(), newGroup.get())) { + if (!compareGroupType(oldGroup.getGroupNode().getGroup(), newGroup.get()) + || !compareGroupFields(oldGroup.getGroupNode().getGroup(), newGroup.get())) { keepPreviousAssignments = dialogService.showConfirmationDialogAndWait( Localization.lang("Change of Grouping Method"), Localization.lang("Assign the original group's entries to this group?")); From eaac2e9e65f877c1624cd91c974628cb631e5a1e Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 12:50:19 +0200 Subject: [PATCH 04/23] add check before dialog --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index c8fda22a530..33fe455a616 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -266,6 +266,11 @@ public void editGroup(GroupNodeViewModel oldGroup) { GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { + + if(this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { + return ; + } + // TODO: Keep assignments String content = Localization.lang("Assign the original group's entries to this group?"); ButtonType keepAssignments = new ButtonType(Localization.lang("Assign"), ButtonBar.ButtonData.YES); From c7c085ade8072d4546ee9df6833472f41780be5c Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 12:51:21 +0200 Subject: [PATCH 05/23] write groups always --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 33fe455a616..9fa394f7346 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -268,6 +268,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { newGroup.ifPresent(group -> { if(this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { + writeGroupChangesToMetaData(); return ; } From a61815278a083731aeae88c8ccc5721611e1ad1d Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 13:21:53 +0200 Subject: [PATCH 06/23] refersh --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 9fa394f7346..3e273826883 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -269,6 +269,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { if(this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { writeGroupChangesToMetaData(); + refresh(); return ; } From 56c1c86a45e4728ac8adce407f6a05922e1750bb Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 15:25:22 +0200 Subject: [PATCH 07/23] store group changes --- .../java/org/jabref/gui/groups/GroupTreeViewModel.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 3e273826883..d7afd6758a5 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -268,6 +268,13 @@ public void editGroup(GroupNodeViewModel oldGroup) { newGroup.ifPresent(group -> { if(this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { + + oldGroup.getGroupNode().setGroup( + group, + true, + false, + database.getEntries()); + writeGroupChangesToMetaData(); refresh(); return ; From e5b0c9096b1358ce0c5753832c4a223cea0856cf Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 15:41:34 +0200 Subject: [PATCH 08/23] rfactor code --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index d7afd6758a5..6db23705a2b 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -193,10 +193,8 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) } private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGroup) { - if (oldGroup.getClass() == WordKeywordGroup.class) { - WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; - WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; + if (oldGroup instanceof WordKeywordGroup oldWordKeywordGroup && newGroup instanceof WordKeywordGroup newWordKeywordGroup) { if (!oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName())) { return false; } else if (!oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression())) { @@ -204,6 +202,7 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou } else { return oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive(); } + } else if (oldGroup.getClass() == RegexKeywordGroup.class) { RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; @@ -280,7 +279,6 @@ public void editGroup(GroupNodeViewModel oldGroup) { return ; } - // TODO: Keep assignments 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); From e6a105cd9c7e01ebf68ac202be24d1d0dcc39ff1 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 16:12:13 +0200 Subject: [PATCH 09/23] refactor with intanceof --- .../jabref/gui/groups/GroupTreeViewModel.java | 96 ++++++++----------- 1 file changed, 38 insertions(+), 58 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 6db23705a2b..38fa3c9f4e3 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -30,7 +30,6 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.groups.AbstractGroup; -import org.jabref.model.groups.AutomaticGroup; import org.jabref.model.groups.AutomaticKeywordGroup; import org.jabref.model.groups.AutomaticPersonsGroup; import org.jabref.model.groups.ExplicitGroup; @@ -203,9 +202,7 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou return oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive(); } - } else if (oldGroup.getClass() == RegexKeywordGroup.class) { - RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; - RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; + } else if (oldGroup instanceof RegexKeywordGroup oldRegexKeywordGroup && newGroup instanceof RegexKeywordGroup newRegexKeywordGroup) { if (!oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName())) { return false; @@ -214,10 +211,7 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou } else { return oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive(); } - } else if (oldGroup.getClass() == SearchGroup.class) { - SearchGroup oldSearchGroup = (SearchGroup) oldGroup; - SearchGroup newSearchGroup = (SearchGroup) newGroup; - + } else if ((oldGroup instanceof SearchGroup oldSearchGroup) && newGroup instanceof SearchGroup newSearchGroup) { if (!oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression())) { return false; } else { @@ -225,28 +219,18 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou } } else if (oldGroup.getClass() == ExplicitGroup.class) { return true; - } else if (oldGroup instanceof AutomaticGroup) { - if (oldGroup.getClass() == AutomaticKeywordGroup.class) { - AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; - AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) newGroup; - - if (!oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString())) { - return false; - } else if (!oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString())) { - return false; - } else { - return oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); - } - } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { - AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; - AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; + } else if (oldGroup instanceof AutomaticKeywordGroup oldAutomaticKeywordGroup && newGroup instanceof AutomaticKeywordGroup newAutomaticKeywordGroup) { - return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); + if (!oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString())) { + return false; + } else if (!oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString())) { + return false; + } else { + return oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); } - } else if (oldGroup.getClass() == TexGroup.class) { - TexGroup oldTexGroup = (TexGroup) oldGroup; - TexGroup newTexGroup = (TexGroup) newGroup; - + } else if (oldGroup instanceof AutomaticPersonsGroup oldAutomaticPersonsGroup && newGroup instanceof AutomaticPersonsGroup newAutomaticPersonsGroup) { + return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); + } else if (oldGroup instanceof TexGroup oldTexGroup && newGroup instanceof TexGroup newTexGroup) { return oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString()); } return true; @@ -258,25 +242,23 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou public void editGroup(GroupNodeViewModel oldGroup) { currentDatabase.ifPresent(database -> { Optional newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView( - dialogService, - database, - preferences, - oldGroup.getGroupNode().getGroup(), - GroupDialogHeader.SUBGROUP)); + dialogService, + database, + preferences, + oldGroup.getGroupNode().getGroup(), + GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { - - if(this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { - + if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( - group, - true, - false, - database.getEntries()); + group, + true, + false, + database.getEntries()); writeGroupChangesToMetaData(); refresh(); - return ; + return; } String content = Localization.lang("Assign the original group's entries to this group?"); @@ -286,17 +268,17 @@ public void editGroup(GroupNodeViewModel oldGroup) { 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 previousAssignments = dialogService.showCustomButtonDialogAndWait(Alert.AlertType.WARNING, - Localization.lang("Change of Grouping Method"), - content, - keepAssignments, - removeAssignments, - cancel); + Localization.lang("Change of Grouping Method"), + content, + keepAssignments, + removeAssignments, + cancel); // WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame()); boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) - && (group instanceof ExplicitGroup); + && (group instanceof ExplicitGroup); int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); @@ -310,16 +292,16 @@ public void editGroup(GroupNodeViewModel oldGroup) { 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; } @@ -342,10 +324,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(); }); From 3c1e409f3e0744669c5d0ec8850c4acf56e60493 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 17:41:50 +0200 Subject: [PATCH 10/23] refactor --- .../jabref/gui/groups/GroupTreeViewModel.java | 64 +++++++------------ 1 file changed, 23 insertions(+), 41 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 38fa3c9f4e3..26ecc2f439b 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -150,14 +150,12 @@ private void onActiveDatabaseChanged(Optional 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 newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView( @@ -191,43 +189,28 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) return oldGroup.getClass().equals(newGroup.getClass()); } - private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGroup) { - + private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup oldGroup, AbstractGroup newGroup) { if (oldGroup instanceof WordKeywordGroup oldWordKeywordGroup && newGroup instanceof WordKeywordGroup newWordKeywordGroup) { - if (!oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName())) { - return false; - } else if (!oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression())) { - return false; - } else { - return oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive(); - } + return oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName()) + || oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) + || (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); } else if (oldGroup instanceof RegexKeywordGroup oldRegexKeywordGroup && newGroup instanceof RegexKeywordGroup newRegexKeywordGroup) { + return oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) + || oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) + || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); - if (!oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName())) { - return false; - } else if (!oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression())) { - return false; - } else { - return oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive(); - } } else if ((oldGroup instanceof SearchGroup oldSearchGroup) && newGroup instanceof SearchGroup newSearchGroup) { - if (!oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression())) { - return false; - } else { - return oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); - } + return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) || + oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); + } else if (oldGroup.getClass() == ExplicitGroup.class) { - return true; + return true; //TOOD unsure } else if (oldGroup instanceof AutomaticKeywordGroup oldAutomaticKeywordGroup && newGroup instanceof AutomaticKeywordGroup newAutomaticKeywordGroup) { + return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) || + oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) + || oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); - if (!oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString())) { - return false; - } else if (!oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString())) { - return false; - } else { - return oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); - } } else if (oldGroup instanceof AutomaticPersonsGroup oldAutomaticPersonsGroup && newGroup instanceof AutomaticPersonsGroup newAutomaticPersonsGroup) { return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); } else if (oldGroup instanceof TexGroup oldTexGroup && newGroup instanceof TexGroup newTexGroup) { @@ -235,7 +218,6 @@ private boolean compareGroupFields(AbstractGroup oldGroup, AbstractGroup newGrou } return true; } - /** * Opens "Edit Group Dialog" and changes the given group to the edited one. */ @@ -249,7 +231,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { - if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.compareGroupFields(oldGroup.getGroupNode().getGroup(), group)) { + if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNecessary(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( group, true, @@ -292,16 +274,16 @@ public void editGroup(GroupNodeViewModel oldGroup) { 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; } From cea4e2b2f0c631751e2efd1364d9ede16deda906 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 18:05:16 +0200 Subject: [PATCH 11/23] fix stupid instanceof error^^ --- .../jabref/gui/groups/GroupTreeViewModel.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 26ecc2f439b..1fa032ae9f7 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -190,22 +190,30 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) } private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup oldGroup, AbstractGroup newGroup) { - if (oldGroup instanceof WordKeywordGroup oldWordKeywordGroup && newGroup instanceof WordKeywordGroup newWordKeywordGroup) { + + if (oldGroup.getClass() == WordKeywordGroup.class) { + WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; + WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; + return oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName()) || oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) || (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); - } else if (oldGroup instanceof RegexKeywordGroup oldRegexKeywordGroup && newGroup instanceof RegexKeywordGroup newRegexKeywordGroup) { + } else if (oldGroup.getClass() == RegexKeywordGroup.class) { + RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; + RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; + return oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) || oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); - } else if ((oldGroup instanceof SearchGroup oldSearchGroup) && newGroup instanceof SearchGroup newSearchGroup) { + } else if ((oldGroup.getClass() == SearchGroup.class)) { + SearchGroup oldSearchGroup = (SearchGroup) oldGroup; + SearchGroup newSearchGroup = (SearchGroup) newGroup; + return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) || oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); - } else if (oldGroup.getClass() == ExplicitGroup.class) { - return true; //TOOD unsure } else if (oldGroup instanceof AutomaticKeywordGroup oldAutomaticKeywordGroup && newGroup instanceof AutomaticKeywordGroup newAutomaticKeywordGroup) { return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) || oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) @@ -235,7 +243,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { oldGroup.getGroupNode().setGroup( group, true, - false, + true, // TODO Not sure database.getEntries()); writeGroupChangesToMetaData(); From 69b03ec7dbb6a5c7d10a86625b39f65ba74dab33 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 18:15:49 +0200 Subject: [PATCH 12/23] checkstyle --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 1fa032ae9f7..640f66aafd1 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -190,7 +190,6 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) } private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup oldGroup, AbstractGroup newGroup) { - if (oldGroup.getClass() == WordKeywordGroup.class) { WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; @@ -198,7 +197,6 @@ private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup ol return oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName()) || oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) || (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); - } else if (oldGroup.getClass() == RegexKeywordGroup.class) { RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; @@ -206,14 +204,12 @@ private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup ol return oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) || oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); - } else if ((oldGroup.getClass() == SearchGroup.class)) { SearchGroup oldSearchGroup = (SearchGroup) oldGroup; SearchGroup newSearchGroup = (SearchGroup) newGroup; return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) || oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); - } else if (oldGroup instanceof AutomaticKeywordGroup oldAutomaticKeywordGroup && newGroup instanceof AutomaticKeywordGroup newAutomaticKeywordGroup) { return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) || oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) @@ -226,6 +222,7 @@ private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup ol } return true; } + /** * Opens "Edit Group Dialog" and changes the given group to the edited one. */ @@ -237,7 +234,6 @@ public void editGroup(GroupNodeViewModel oldGroup) { preferences, oldGroup.getGroupNode().getGroup(), GroupDialogHeader.SUBGROUP)); - newGroup.ifPresent(group -> { if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNecessary(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( From 8f4849eea943add7b689a7935b476ecc5aca308f Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 21:43:25 +0200 Subject: [PATCH 13/23] use getclass everywhere add comment --- .../jabref/gui/groups/GroupTreeViewModel.java | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 640f66aafd1..daffad4f038 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -190,6 +190,8 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) } private boolean checkGroupFieldsForModificationsDialogNecessary(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; @@ -203,21 +205,28 @@ private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup ol return oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) || oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) - || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); + || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); } else if ((oldGroup.getClass() == SearchGroup.class)) { SearchGroup oldSearchGroup = (SearchGroup) oldGroup; SearchGroup newSearchGroup = (SearchGroup) newGroup; return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) || oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); - } else if (oldGroup instanceof AutomaticKeywordGroup oldAutomaticKeywordGroup && newGroup instanceof AutomaticKeywordGroup newAutomaticKeywordGroup) { - return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) || - oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) + } else if (oldGroup.getClass() == AutomaticKeywordGroup.class) { + AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; + AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; + + return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) + || oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) || oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); + } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { + AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; + AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; - } else if (oldGroup instanceof AutomaticPersonsGroup oldAutomaticPersonsGroup && newGroup instanceof AutomaticPersonsGroup newAutomaticPersonsGroup) { return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); - } else if (oldGroup instanceof TexGroup oldTexGroup && newGroup instanceof TexGroup newTexGroup) { + } else if (oldGroup.getClass() == TexGroup.class) { + TexGroup oldTexGroup = (TexGroup) oldGroup; + TexGroup newTexGroup = (TexGroup) newGroup; return oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString()); } return true; From b80d8788084b6877a8c52795c8a1d74ba75235fa Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 22:43:26 +0200 Subject: [PATCH 14/23] Change or to and, only return true if no changes add tests --- .../jabref/gui/groups/GroupTreeViewModel.java | 27 +++++--- .../gui/groups/GroupTreeViewModelTest.java | 65 +++++++++++++++++-- 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index daffad4f038..5069e8ee678 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -189,7 +189,14 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) return oldGroup.getClass().equals(newGroup.getClass()); } - private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup oldGroup, AbstractGroup newGroup) { + /** + * Check if it is necessary to show a group modified, reassign entry dialog
+ * Although group name change is relevant, we silently take it as a rename and do not bother the user + * @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 checkGroupFieldsForModificationsDialogNotNecessary(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) { @@ -197,28 +204,28 @@ private boolean checkGroupFieldsForModificationsDialogNecessary(AbstractGroup ol WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; return oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName()) - || oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) - || (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); + && oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) + && (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); } else if (oldGroup.getClass() == RegexKeywordGroup.class) { RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; return oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) - || oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) - || (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); + && oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) + && (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); } else if ((oldGroup.getClass() == SearchGroup.class)) { SearchGroup oldSearchGroup = (SearchGroup) oldGroup; SearchGroup newSearchGroup = (SearchGroup) newGroup; - return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) || - oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); + return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) + && oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); } else if (oldGroup.getClass() == AutomaticKeywordGroup.class) { AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; return oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) - || oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) - || oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); + && oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) + && oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; @@ -244,7 +251,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { oldGroup.getGroupNode().getGroup(), GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { - if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNecessary(oldGroup.getGroupNode().getGroup(), group)) { + if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( group, true, diff --git a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java index c96d5262396..871a0f66c2f 100644 --- a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java +++ b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java @@ -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; @@ -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() { @@ -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()); } @@ -85,4 +92,52 @@ 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.checkGroupFieldsForModificationsDialogNotNecessary(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.checkGroupFieldsForModificationsDialogNotNecessary(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.checkGroupFieldsForModificationsDialogNotNecessary(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.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); + } } From ff18bb56dc86c257b53cff2a4cb6e5601e0ab71b Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 23:40:11 +0200 Subject: [PATCH 15/23] fuu checkstyle --- .../org/jabref/gui/groups/GroupTreeViewModel.java | 11 +++++------ .../org/jabref/gui/groups/GroupTreeViewModelTest.java | 1 - 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 5069e8ee678..0350ff894ee 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -190,14 +190,13 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) } /** - * Check if it is necessary to show a group modified, reassign entry dialog
+ * Check if it is necessary to show a group modified, reassign entry dialog
* Although group name change is relevant, we silently take it as a rename and do not bother the user * @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 checkGroupFieldsForModificationsDialogNotNecessary(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; @@ -219,7 +218,7 @@ boolean checkGroupFieldsForModificationsDialogNotNecessary(AbstractGroup oldGrou return oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) && oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); - } else if (oldGroup.getClass() == AutomaticKeywordGroup.class) { + } else if (oldGroup.getClass() == AutomaticKeywordGroup.class) { AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; @@ -231,9 +230,9 @@ boolean checkGroupFieldsForModificationsDialogNotNecessary(AbstractGroup oldGrou AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; return oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); - } else if (oldGroup.getClass() == TexGroup.class) { - TexGroup oldTexGroup = (TexGroup) oldGroup; - TexGroup newTexGroup = (TexGroup) newGroup; + } else if (oldGroup.getClass() == TexGroup.class) { + TexGroup oldTexGroup = (TexGroup) oldGroup; + TexGroup newTexGroup = (TexGroup) newGroup; return oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString()); } return true; diff --git a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java index 871a0f66c2f..2ae74edf4ee 100644 --- a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java +++ b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java @@ -116,7 +116,6 @@ void shouldNotShowDialogWhenGroupsAreEqual() { assertTrue(model.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); } - @Test void shouldShowDialogWhenKeywordDiffers() { AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true); From 898e1fc5fda11527e39721be17b1e3995764bf93 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 4 Sep 2022 23:49:58 +0200 Subject: [PATCH 16/23] javadoc checkstyle --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 0350ff894ee..0c3e59658fe 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -192,6 +192,7 @@ private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) /** * Check if it is necessary to show a group modified, reassign entry dialog
* Although group name change is relevant, we silently take it as a rename and do not bother the user + * * @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 From 91509388a90f396a069d09f966e9b39f95352e4e Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sat, 10 Sep 2022 20:51:32 +0200 Subject: [PATCH 17/23] TODO: clarifiy behavior when changing the groups name not really sure what happns when you have more than 2 groups --- .../jabref/gui/groups/GroupTreeViewModel.java | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 0c3e59658fe..89bd4ac8458 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -251,6 +251,72 @@ public void editGroup(GroupNodeViewModel oldGroup) { oldGroup.getGroupNode().getGroup(), GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { + + AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); + String oldGroupName = oldGroupDef.getName(); + + // we changed the name. + // dialog already warns us about this if we name it like another existing group + // So can simply check if we have more than 1 group + if (compareGroupType(oldGroupDef, group) && !group.getName().equals(oldGroupName)) { + + // We check if we still have the same group type, apparently this is only allowed for Explicit groups? + boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) + && (group instanceof ExplicitGroup); + + int groupsWithSameName = 0; + Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); + if (databaseRootGroup.isPresent()) { + String name = oldGroup.getGroupNode().getGroup().getName(); + groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); + } + // We found more than 2 groups, so we cannot simply remove old assignment + if (groupsWithSameName >= 2) { + removePreviousAssignments = false; + } + //Dialog seems to be superflous in this case + + 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)"); + } + + Optional previousAssignments = dialogService.showCustomButtonDialogAndWait(Alert.AlertType.WARNING, + Localization.lang("Group name change"), + content, + keepAssignments, + removeAssignments, + cancel); + if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.YES)) { + oldGroup.getGroupNode().setGroup( + group, + true, + removePreviousAssignments, + database.getEntries()); + } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.NO)) { + oldGroup.getGroupNode().setGroup( + group, + false, + removePreviousAssignments, + database.getEntries()); + } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.CANCEL_CLOSE)) { + return; + } + 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 (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( group, @@ -288,6 +354,8 @@ public void editGroup(GroupNodeViewModel oldGroup) { 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; } @@ -334,6 +402,10 @@ public void editGroup(GroupNodeViewModel oldGroup) { }); } + private void showGroupChangeDialog() { + + } + public void removeSubgroups(GroupNodeViewModel group) { boolean confirmation = dialogService.showConfirmationDialogAndWait( Localization.lang("Remove subgroups"), From 78a3cd7f21aec5bc0c3107727f002f14f6a7a9ab Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Mon, 12 Sep 2022 21:10:13 +0200 Subject: [PATCH 18/23] automatically assign when more than one group --- .../jabref/gui/groups/GroupTreeViewModel.java | 49 ++++--------------- 1 file changed, 9 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 89bd4ac8458..b7ff21fb8c5 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -255,14 +255,10 @@ public void editGroup(GroupNodeViewModel oldGroup) { AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); String oldGroupName = oldGroupDef.getName(); - // we changed the name. // dialog already warns us about this if we name it like another existing group - // So can simply check if we have more than 1 group - if (compareGroupType(oldGroupDef, group) && !group.getName().equals(oldGroupName)) { - - // We check if we still have the same group type, apparently this is only allowed for Explicit groups? - boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) - && (group instanceof ExplicitGroup); + // We need to check if the name change and not any other thing as well + // + if (compareGroupType(oldGroupDef, group) && !group.getName().equals(oldGroupName) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroupDef, group)) { int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); @@ -270,51 +266,24 @@ public void editGroup(GroupNodeViewModel oldGroup) { String name = oldGroup.getGroupNode().getGroup().getName(); groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); } + boolean removePreviousAssignments = true; // We found more than 2 groups, so we cannot simply remove old assignment if (groupsWithSameName >= 2) { removePreviousAssignments = false; } - //Dialog seems to be superflous in this case - - 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)"); - } + oldGroup.getGroupNode().setGroup( + group, + true, + removePreviousAssignments, + database.getEntries()); - Optional previousAssignments = dialogService.showCustomButtonDialogAndWait(Alert.AlertType.WARNING, - Localization.lang("Group name change"), - content, - keepAssignments, - removeAssignments, - cancel); - if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.YES)) { - oldGroup.getGroupNode().setGroup( - group, - true, - removePreviousAssignments, - database.getEntries()); - } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.NO)) { - oldGroup.getGroupNode().setGroup( - group, - false, - removePreviousAssignments, - database.getEntries()); - } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.CANCEL_CLOSE)) { - return; - } 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 (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup.getGroupNode().getGroup(), group)) { From a1fad49a34c7416014c4ae5b9abec64de4e66ef8 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Mon, 12 Sep 2022 21:33:31 +0200 Subject: [PATCH 19/23] use new name --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index b7ff21fb8c5..5b25b698f8b 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -263,7 +263,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); if (databaseRootGroup.isPresent()) { - String name = oldGroup.getGroupNode().getGroup().getName(); + String name = group.getName(); groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); } boolean removePreviousAssignments = true; @@ -371,10 +371,6 @@ public void editGroup(GroupNodeViewModel oldGroup) { }); } - private void showGroupChangeDialog() { - - } - public void removeSubgroups(GroupNodeViewModel group) { boolean confirmation = dialogService.showConfirmationDialogAndWait( Localization.lang("Remove subgroups"), From 860b7497ef7edcaaddfb42f3f28992b381214ede Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Wed, 14 Sep 2022 13:10:16 +0200 Subject: [PATCH 20/23] todo --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 5b25b698f8b..072e7541c63 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -320,6 +320,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); if (databaseRootGroup.isPresent()) { + //TODO Old Group or new group? String name = oldGroup.getGroupNode().getGroup().getName(); groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); } From b5a340cbfc16810c40811242c88e175c15801c85 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sat, 17 Sep 2022 19:52:10 +0200 Subject: [PATCH 21/23] we need to check old group name rename methods --- .../jabref/gui/groups/GroupTreeViewModel.java | 61 ++++++++++--------- .../jabref/model/groups/GroupTreeNode.java | 10 +-- .../gui/groups/GroupTreeViewModelTest.java | 8 +-- 3 files changed, 40 insertions(+), 39 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 072e7541c63..af29f5ec588 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -185,56 +185,56 @@ private void writeGroupChangesToMetaData() { currentDatabase.ifPresent(database -> database.getMetaData().setGroups(rootGroup.get().getGroupNode())); } - private boolean compareGroupType(AbstractGroup oldGroup, AbstractGroup newGroup) { + 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
- * Although group name change is relevant, we silently take it as a rename and do not bother the user + * 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 checkGroupFieldsForModificationsDialogNotNecessary(AbstractGroup oldGroup, AbstractGroup newGroup) { + 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 oldWordKeywordGroup.getSearchField().getName().equals(newWordKeywordGroup.getSearchField().getName()) - && oldWordKeywordGroup.getSearchExpression().equals(newWordKeywordGroup.getSearchExpression()) - && (oldWordKeywordGroup.isCaseSensitive() == newWordKeywordGroup.isCaseSensitive()); + 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 oldRegexKeywordGroup.getSearchField().getName().equals(newRegexKeywordGroup.getSearchField().getName()) - && oldRegexKeywordGroup.getSearchExpression().equals(newRegexKeywordGroup.getSearchExpression()) - && (oldRegexKeywordGroup.isCaseSensitive() == newRegexKeywordGroup.isCaseSensitive()); + 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 oldSearchGroup.getSearchExpression().equals(newSearchGroup.getSearchExpression()) - && oldSearchGroup.getSearchFlags().equals(newSearchGroup.getSearchFlags()); + 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 oldAutomaticKeywordGroup.getKeywordDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordDelimiter().toString()) - && oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString().equals(newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter().toString()) - && oldAutomaticKeywordGroup.getField().getName().equals(newAutomaticKeywordGroup.getField().getName()); + 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 oldAutomaticPersonsGroup.getField().getName().equals(newAutomaticPersonsGroup.getField().getName()); + return Objects.equals(oldAutomaticPersonsGroup.getField().getName(), newAutomaticPersonsGroup.getField().getName()); } else if (oldGroup.getClass() == TexGroup.class) { TexGroup oldTexGroup = (TexGroup) oldGroup; TexGroup newTexGroup = (TexGroup) newGroup; - return oldTexGroup.getFilePath().toString().equals(newTexGroup.getFilePath().toString()); + return Objects.equals(oldTexGroup.getFilePath().toString(), newTexGroup.getFilePath().toString()); } return true; } @@ -255,16 +255,18 @@ public void editGroup(GroupNodeViewModel oldGroup) { AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); String oldGroupName = oldGroupDef.getName(); - // dialog already warns us about this if we name it like another existing group - // We need to check if the name change and not any other thing as well - // - if (compareGroupType(oldGroupDef, group) && !group.getName().equals(oldGroupName) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroupDef, group)) { + boolean groupTypeEqual = isGroupTypeEqual(oldGroupDef, group); + boolean onlyMinorModifications = this.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 databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); if (databaseRootGroup.isPresent()) { - String name = group.getName(); - groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); + // 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 @@ -282,22 +284,23 @@ public void editGroup(GroupNodeViewModel oldGroup) { writeGroupChangesToMetaData(); // This is ugly but we have no proper update mechanism in place to propagate the changes, so redraw everything refresh(); - return; } - if (this.compareGroupType(oldGroup.getGroupNode().getGroup(), group) && this.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup.getGroupNode().getGroup(), group)) { + if (groupTypeEqual && onlyMinorChanges(oldGroup.getGroupNode().getGroup(), group)) { oldGroup.getGroupNode().setGroup( - group, - true, - true, // TODO Not sure - database.getEntries()); + group, + true, + true, // TODO Not sure + 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); @@ -313,14 +316,12 @@ public void editGroup(GroupNodeViewModel oldGroup) { keepAssignments, removeAssignments, cancel); - // WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame()); boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) && (group instanceof ExplicitGroup); int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); if (databaseRootGroup.isPresent()) { - //TODO Old Group or new group? String name = oldGroup.getGroupNode().getGroup().getName(); groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); } diff --git a/src/main/java/org/jabref/model/groups/GroupTreeNode.java b/src/main/java/org/jabref/model/groups/GroupTreeNode.java index 6cb8b15d722..efe4d233fee 100644 --- a/src/main/java/org/jabref/model/groups/GroupTreeNode.java +++ b/src/main/java/org/jabref/model/groups/GroupTreeNode.java @@ -72,17 +72,17 @@ public List setGroup(AbstractGroup newGroup, boolean shouldKeepPrev group = Objects.requireNonNull(newGroup); List 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 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)); } diff --git a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java index 2ae74edf4ee..96ba2684e5d 100644 --- a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java +++ b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java @@ -101,7 +101,7 @@ void shouldNotShowDialogWhenGroupNameChanges() { databaseContext.getDatabase().insertEntry(entry); GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); - assertTrue(model.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); + assertTrue(model.onlyMinorChanges(oldGroup, newGroup)); } @Test @@ -113,7 +113,7 @@ void shouldNotShowDialogWhenGroupsAreEqual() { databaseContext.getDatabase().insertEntry(entry); GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); - assertTrue(model.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); + assertTrue(model.onlyMinorChanges(oldGroup, newGroup)); } @Test @@ -125,7 +125,7 @@ void shouldShowDialogWhenKeywordDiffers() { databaseContext.getDatabase().insertEntry(entry); GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); - assertFalse(model.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); + assertFalse(model.onlyMinorChanges(oldGroup, newGroup)); } @Test @@ -137,6 +137,6 @@ void shouldShowDialogWhenCaseSensitivyDiffers() { databaseContext.getDatabase().insertEntry(entry); GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); - assertFalse(model.checkGroupFieldsForModificationsDialogNotNecessary(oldGroup, newGroup)); + assertFalse(model.onlyMinorChanges(oldGroup, newGroup)); } } From 2d45980e182ac460e1ab583551fa378d317615e1 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sat, 17 Sep 2022 19:58:57 +0200 Subject: [PATCH 22/23] checkstyle --- src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index af29f5ec588..edd0720dd46 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -261,7 +261,6 @@ public void editGroup(GroupNodeViewModel oldGroup) { // 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 databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); if (databaseRootGroup.isPresent()) { From fa69ec781204da095c05212b9cd1c2f615d68a73 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Mon, 3 Oct 2022 18:08:24 +0200 Subject: [PATCH 23/23] Only check for minor mods if group types are equal --- .../org/jabref/gui/groups/GroupTreeViewModel.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index edd0720dd46..57f17315ad9 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -245,18 +245,18 @@ boolean onlyMinorChanges(AbstractGroup oldGroup, AbstractGroup newGroup) { public void editGroup(GroupNodeViewModel oldGroup) { currentDatabase.ifPresent(database -> { Optional newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView( - dialogService, - database, - preferences, - oldGroup.getGroupNode().getGroup(), - GroupDialogHeader.SUBGROUP)); + dialogService, + database, + preferences, + oldGroup.getGroupNode().getGroup(), + GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); String oldGroupName = oldGroupDef.getName(); boolean groupTypeEqual = isGroupTypeEqual(oldGroupDef, group); - boolean onlyMinorModifications = this.onlyMinorChanges(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 @@ -290,7 +290,7 @@ public void editGroup(GroupNodeViewModel oldGroup) { oldGroup.getGroupNode().setGroup( group, true, - true, // TODO Not sure + true, database.getEntries()); writeGroupChangesToMetaData();