Skip to content

Fix saving triggers metadata changed#9165

Merged
koppor merged 3 commits into
mainfrom
fixMetadataDiff
Sep 20, 2022
Merged

Fix saving triggers metadata changed#9165
koppor merged 3 commits into
mainfrom
fixMetadataDiff

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Sep 19, 2022

Copy link
Copy Markdown
Member

Fixes #9159

Generate new equals, hashcode for FieldFormatterCleanup and toString methods

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Fixes #9159

Generate new equals, hashcode for FieldFormatterCleanup and toString methods
Comment thread src/main/java/org/jabref/model/metadata/MetaData.java
Comment thread src/main/java/org/jabref/model/metadata/MetaData.java
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 19, 2022

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

To cover cases when new fields are added to the classes, maybe, we should add test cases with deepEquals of AssertJ - https://stackoverflow.com/a/9633089/873282

Note to self: Testing equality by reflection is not an option, as a) it used reflection and b) is less performant: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/EqualsBuilder.html#reflectionAppend-java.lang.Object-java.lang.Object-

Comment on lines 108 to 117
@Override
public boolean equals(Object o) {
if (this == o) {
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (o instanceof FieldFormatterCleanup) {
FieldFormatterCleanup that = (FieldFormatterCleanup) o;
return Objects.equals(field, that.field) && Objects.equals(formatter, that.formatter);
if (!(obj instanceof FieldFormatterCleanup)) {
return false;
}
return false;
FieldFormatterCleanup other = (FieldFormatterCleanup) obj;
return Objects.equals(field, other.field) && Objects.equals(formatter, other.formatter);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The new code seems to be the equial to the old one. ^^.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just used eclipse to regenerate it to have it consistent with others

Comment on lines +209 to +230

@Override
public int hashCode() {
return Objects.hash(actions, enabled);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (!(obj instanceof FieldFormatterCleanups)) {
return false;
}
FieldFormatterCleanups other = (FieldFormatterCleanups) obj;
return Objects.equals(actions, other.actions) && (enabled == other.enabled);
}

@Override
public String toString() {
return "FieldFormatterCleanups [enabled=" + enabled + ", actions=" + actions + "]";
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for putting in the implementations here. I googled and found

By default, two objects are equal if and only if they are refer to the same memory location

@koppor koppor merged commit 7dcdd61 into main Sep 20, 2022
@koppor koppor deleted the fixMetadataDiff branch September 20, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ignore unnecessary library modification notifications?

2 participants