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

Include all standard columns with citationkey when exporting to old OpenOffice Calc format #8176

Merged
merged 5 commits into from
Oct 26, 2021
Merged

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Oct 23, 2021

Fixes #8141

Changes made:

  • Replace hardcoding fields to be exported, to a call to FieldFactory.getStandardFieldsWithCitationKey()
  • Refactor OOCalcDatabase.java
  • Remove redundant comments
  • Export 130 fields rather than 34 in the previous implementation

NOTE: I didn't include fields with custom names

I have opened a draft PR because I noticed OpenDocumentRepresentation (used to export OpenDocument spreadsheet), having the same problems as OOCalcDatabase, I wanted to ask if fixing OpenDocumentRepresentation should have its own PR or it's ok to include it in this one.

Screenshot for the exported document in OpenOffice 4.1.11, Windows 11

Screenshot for the exported document in OpenOffice 4.1.11, Windows 11

  • 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
  • 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.

@calixtus
Copy link
Member

Hey, thanks for the pr. I think this fix will be a great improvement! Please note that there is already a major refactor of the oo-interface in our backlog. Please take a look into it, so yours and the other one work fine together.

@Siedlerchr
Copy link
Member

@calixtus this is completely unrelated to the other open office integration. This is solely an exporter

@calixtus
Copy link
Member

Oh i see, thanks. The oo prefix of the filename mislead me.

@HoussemNasri
Copy link
Member Author

I will handle OpenDocumentRepresentation in another PR since #8141 doesn't mention it, this PR is ready for review

@HoussemNasri HoussemNasri marked this pull request as ready for review October 24, 2021 14:17
@Siedlerchr
Copy link
Member

Codewise looks already good, it would be cool to add a test for it. I think at least for the generated XML it should be possible. Have a look at the Docbook5ExporterTest, it uses the XMlUnit testing framework
Idea would be to add a sample xml representation of the document in the test folder

@HoussemNasri
Copy link
Member Author

Thanks, I'll have a look, although I've never written tests before I'll see how that would go.

@HoussemNasri
Copy link
Member Author

There are 2 XML files in the exported zip, content.xml and meta.xml, should we test both of them or just content.xml, if both then should it be one test case or one for each file.

@Siedlerchr
Copy link
Member

content.xml should be enough, if the code comment is still right, then the meta.xml and manifest are just already existing files that are just copied over.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks

@Siedlerchr
Copy link
Member

lgtm from my side as well. Can you please add a changelog entry as well?

@HoussemNasri
Copy link
Member Author

Thank you, I've added an entry in the Changed section

@Siedlerchr Siedlerchr merged commit 7c2a9a2 into JabRef:main Oct 26, 2021
@HoussemNasri HoussemNasri deleted the fix-8141 branch November 1, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export an ods table from jabref is messing it up
3 participants