Fix #4033: order of fields in customized entry types is saved correctly #4127
Conversation
tobiasdiez
commented
Jun 13, 2018
tobiasdiez
commented
- Change in CHANGELOG.md described
- Tests created for changes
- Manually tested changed features in running JabRef
- Screenshots added in PR description (for bigger UI changes)
- Ensured that the git commit message is a good one
- Check documentation status (Issue created for outdated help page at help.jabref.org?)
tobiasdiez
added
the
status: ready-for-review
| @@ -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()) | |||
There was a problem hiding this comment.
Is the reason for the bug that array.equals != list.equals?
There was a problem hiding this comment.
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-
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
TreeSet still inherits equals from set (as does LinkedHashSet which is usually the better choice than TreeSet).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This really seems to be only alternative.
|
If you fix the failing tests, it's good! |
