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.

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

Fix entry gets deleted after aux import #6746

Merged
merged 9 commits into from
Aug 18, 2020
Merged

Fix entry gets deleted after aux import #6746

merged 9 commits into from
Aug 18, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 7, 2020

Set changed flag on clone also for Misc entry type, because otherwise it equals the default entry type and no change is triggered which results in the entry not beeing written to the database on save
Fixes #6405

Simplify gui code

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Set changed flag on clone also for Misc entry type, because otherwise it equals the default entry type and no change is triggered which results in the entry not beeing written to the database on save
Fixes  #6405

Simplify gui code
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 7, 2020
*/
@Override
public Object clone() {
BibEntry clone = new BibEntry(type.getValue());
if (StandardEntryType.Misc.equals(type.getValue())) {
clone.changed = true;
Copy link
Member

Choose a reason for hiding this comment

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

I would doubt that this fixed the issue for the other types. -- When the entry type is Article, the clone is unchanged, too. Thus, no serialization written

Copy link
Member Author

Choose a reason for hiding this comment

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

New BibEntryType(entryType) always calls setType() and thus:
The default value for field.get is the default entry type misc.

public Optional<FieldChange> setType(EntryType newType, EntriesEventSource eventSource) {
Objects.requireNonNull(newType);
EntryType oldType = type.get();
if (newType.equals(oldType)) {
return Optional.empty();
}
changed = true;
this.type.setValue(newType);
FieldChange change = new FieldChange(this, InternalField.TYPE_HEADER, oldType.getName(), newType.getName());

Copy link
Member Author

Choose a reason for hiding this comment

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

For all other non default entry types the change flat is correclty set that was what this issue made so special.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't update the changed flag. The clone method should really clone the original entry, without any modifications (otherwise its not a clone).

However, the aux importer should set the changed flag of all entries (regardless of type), so that they get properly reformatted when writing them to disc (i.e. not using the parsed serialization of the original bib file).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the clone method implicitly sets the changed flags for all entry types except the default entry type.
The aux importer has nothing to do with this changed or serialization stuff. It's only relevant for saving the newly created library. I don't see any reason to add this logic to the AuxImporter
Only the BibTexParser does it.

CHANGELOG.md Outdated Show resolved Hide resolved
*/
@Override
public Object clone() {
BibEntry clone = new BibEntry(type.getValue());
if (StandardEntryType.Misc.equals(type.getValue())) {
clone.changed = true;
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't update the changed flag. The clone method should really clone the original entry, without any modifications (otherwise its not a clone).

However, the aux importer should set the changed flag of all entries (regardless of type), so that they get properly reformatted when writing them to disc (i.e. not using the parsed serialization of the original bib file).

@koppor
Copy link
Member

koppor commented Aug 17, 2020

@calixtus and me implemented all required changes.

@Siedlerchr
Copy link
Member Author

Manually tested the changes again, All good. Merge now

@Siedlerchr Siedlerchr merged commit ec53712 into master Aug 18, 2020
@Siedlerchr Siedlerchr deleted the auximport branch August 18, 2020 06:22
Siedlerchr added a commit that referenced this pull request Aug 19, 2020
* upstream/master:
  Remember last choice of download files in import dialog (#6761)
  Fix entry gets deleted after aux import (#6746)
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.

Entry is missing after sublibrary is generated from aux file
3 participants