Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #4033: order of fields in customized entry types is saved correctly #4127

Merged
merged 2 commits into from
Jul 2, 2018
Merged

Fix #4033: order of fields in customized entry types is saved correctly #4127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a094ea4
Rename dialog class
tobiasdiez Jun 13, 2018
2f18ad5
Fix #4033: order of fields in customized entry types is saved correctly
tobiasdiez Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We fixed an issue where the custom file column were sorted incorrectly. https://github.com/JabRef/jabref/issues/3119
- We fixed an issues where the entry losses focus when a field is edited and at the same time used for sorting. https://github.com/JabRef/jabref/issues/3373
- We fixed an issue where the menu on Mac OS was not displayed in the usual Mac-specific way. https://github.com/JabRef/jabref/issues/3146
- We fixed an issue where the order of fields in customized entry types was not saved correctly. [#4033](http://github.com/JabRef/jabref/issues/4033)
- We fixed an issue where the groups tree of the last database was still shown even after the database was already closed.
- We fixed an issue where the "Open file dialog" may disappear behind other windows. https://github.com/JabRef/jabref/issues/3410
- We fixed an issue where the default icon of a group was not colored correctly.
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/actions/CustomizeEntryAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import javax.swing.JDialog;

import org.jabref.gui.JabRefFrame;
import org.jabref.gui.customentrytypes.EntryCustomizationDialog;
import org.jabref.gui.customentrytypes.EntryTypeCustomizationDialog;

public class CustomizeEntryAction extends SimpleCommand {

Expand All @@ -12,10 +12,10 @@ public class CustomizeEntryAction extends SimpleCommand {
public CustomizeEntryAction(JabRefFrame frame) {
this.frame = frame;
}

@Override
public void execute() {
JDialog dialog = new EntryCustomizationDialog(frame);
JDialog dialog = new EntryTypeCustomizationDialog(frame);
dialog.setVisible(true);
}
}
17 changes: 10 additions & 7 deletions ...mentrytypes/EntryCustomizationDialog.java → ...rytypes/EntryTypeCustomizationDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -49,7 +50,7 @@

import com.jgoodies.forms.builder.ButtonBarBuilder;

public class EntryCustomizationDialog extends JabRefDialog implements ListSelectionListener {
public class EntryTypeCustomizationDialog extends JabRefDialog implements ListSelectionListener {

protected GridBagLayout gbl = new GridBagLayout();
protected GridBagConstraints con = new GridBagConstraints();
Expand All @@ -71,10 +72,10 @@ public class EntryCustomizationDialog extends JabRefDialog implements ListSelect
private BibDatabaseMode bibDatabaseMode;

/**
* Creates a new instance of EntryCustomizationDialog
* Creates a new instance of EntryTypeCustomizationDialog
*/
public EntryCustomizationDialog(JabRefFrame frame) {
super((JFrame) null, Localization.lang("Customize entry types"), false, EntryCustomizationDialog.class);
public EntryTypeCustomizationDialog(JabRefFrame frame) {
super((JFrame) null, Localization.lang("Customize entry types"), false, EntryTypeCustomizationDialog.class);

this.frame = frame;
initGui();
Expand Down Expand Up @@ -276,11 +277,13 @@ private void applyChanges() {
if (biblatexMode) {
Set<String> oldPrimaryOptionalFieldsLists = oldType.get().getPrimaryOptionalFields();
Set<String> oldSecondaryOptionalFieldsList = oldType.get().getSecondaryOptionalFields();
if (oldRequiredFieldsList.equals(requiredFieldsList) && oldPrimaryOptionalFieldsLists.equals(optionalFieldsList) &&
oldSecondaryOptionalFieldsList.equals(secondaryOptionalFieldsLists)) {
if (Arrays.equals(oldRequiredFieldsList.toArray(), requiredFieldsList.toArray())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason for the bug that array.equals != list.equals?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really odd, normally the equals should work here:
https://docs.oracle.com/javase/8/docs/api/java/util/List.html#equals-java.lang.Object-

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract for sets is that equals does not take the order into account (even if it is a linkedhashset). This is in contrast to equality of lists (which however allow duplication). On the other hand, arrays equals takes the order into account. The cleanest solution would be to use a data structure that is ordered by design and prevents duplication; but I'm not aware of such an implementation (as said above LinkedHashSet does not behave as an ordered set, it just provides an ordered iterator).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a Tree Set, I used that in tests in another program to ensure that the output is correct every time https://docs.oracle.com/javase/7/docs/api/java/util/TreeSet.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TreeSet still inherits equals from set (as does LinkedHashSet which is usually the better choice than TreeSet).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not, A TreeSet implements Comparable and uses the method of the compareTo

Note that the ordering maintained by a set (whether or not an explicit comparator is provided) must be consistent with equals if it is to correctly implement the Set interface. (See Comparable or Comparator for a precise definition of consistent with equals.) This is so because the Set interface is defined in terms of the equals operation, but a TreeSet instance performs all element comparisons using its compareTo (or compare) method, so two elements that are deemed equal by this method are, from the standpoint of the set, equal. The behavior of a set is well-defined even if its ordering is inconsistent with equals; it just fails to obey the general contract of the Set interface.

https://dzone.com/articles/the-hidden-contract-between-equals-and-comparable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you see here http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/8ed8e2b4b90e/src/share/classes/java/util/TreeSet.java the class does not override the default implementation of equals of http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/8ed8e2b4b90e/src/share/classes/java/util/AbstractSet.java. The comment you cite concerns equality of items not of the set itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really seems to be only alternative.

&& Arrays.equals(oldPrimaryOptionalFieldsLists.toArray(), optionalFieldsList.toArray())
&& Arrays.equals(oldSecondaryOptionalFieldsList.toArray(), secondaryOptionalFieldsLists.toArray())) {
changesMade = false;
}
} else if (oldRequiredFieldsList.equals(requiredFieldsList) && oldOptionalFieldsList.equals(optionalFieldsList)) {
} else if (Arrays.equals(oldRequiredFieldsList.toArray(), requiredFieldsList.toArray())
&& Arrays.equals(oldOptionalFieldsList.toArray(), optionalFieldsList.toArray())) {
changesMade = false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/customentrytypes/EntryTypeList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

/**
* This class extends FieldSetComponent to provide some required functionality for the
* list of entry types in EntryCustomizationDialog.
* list of entry types in EntryTypeCustomizationDialog.
*/
public class EntryTypeList extends FieldSetComponent implements ListSelectionListener {

Expand Down