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 xmp exporter #3964

Merged
merged 17 commits into from
Apr 24, 2018
Merged

Fix xmp exporter #3964

merged 17 commits into from
Apr 24, 2018

Conversation

johannes-manner
Copy link
Collaborator

@johannes-manner johannes-manner commented Apr 18, 2018

Solves koppor#338.

  • Manually tested changed features in running JabRef

Changed the behavior of the xmp exporter. Remove the ?xpacket tags with xmp metadata in dublin core format to enable users to use the .xmp files, exported by JabRef, with the xmpincl LaTeX package.

Also adressed the month parsing problem, implemented a workaround and created an issue for the date section within xmpbox (https://issues.apache.org/jira/browse/PDFBOX-4196).

@johannes-manner johannes-manner added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 18, 2018
Due to the elimination of the ?xpacket tags, the number of lines in the correponding files, generated by the xmp exporter, changed.
@stefan-kolb stefan-kolb requested a review from koppor April 23, 2018 09:41
@@ -64,8 +67,16 @@ public void export(BibDatabaseContext databaseContext, Path file, Charset encodi
}

private void writeBibToXmp(Path file, List<BibEntry> entries, Charset encoding) throws IOException {
String xmpContent = XmpUtilWriter.generateXmpString(entries, this.xmpPreferences);
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a second method there? generateXmpStringWithoutXmpDeclaration? Maybe rename to current functio to generateXmpStringWithXmpDeclaration. The term "XmpDeclaration" stems from "XmlDeclaration". See https://wiki.selfhtml.org/wiki/XML/Regeln/XML-Deklaration.

@@ -61,12 +63,12 @@ private void extractAuthor() {
}

/**
* Year + Month in BibTex - Date in DublinCore is a combination of year and month information
* Year in BibTex - Date in DublinCore is only the year information, because dc interprets empty months as January.
Copy link
Member

Choose a reason for hiding this comment

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

I don't get this change? The old code from line 81 on seems to extract the month perfectly. In case no month is there, the fallback was and still seems to be January.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes the fallback is January, but I thought it's better to not include default values like January....
In my opinion, data consistency is somehow corrupted...

relationships.stream()
.filter(isBibTeXElement)
.forEach(splitBibTeXElement);

Copy link
Member

Choose a reason for hiding this comment

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

Remove empty line 😇

int i = r.indexOf('/');
if (i != -1) {
bibEntry.setField(r.substring(0, i), r.substring(i + 1));
Predicate<String> isBibTeXElement = s -> s.startsWith("bibtex/");
Copy link
Member

Choose a reason for hiding this comment

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

Move this down just after List<String relationships =.

Consumer<String> splitBibTeXElement = s -> {
// split solution is complicated, because some fields contains url etc.
String temp = s.substring("bibtex/".length());
int i = temp.indexOf('/');
Copy link
Member

Choose a reason for hiding this comment

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

Please add an example why you need this substring. It is clear from the code that it is bibtex/key/value, but it should be documented somewhere (and referenced from here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right with the schema bibtex/key/value.
In a former approach I used String#split, but in some cases the value is an url or doi etc.

So the value in this case can contain "/" and therefore the substring is the easiest way to handle this. Documenting the url problem would be beneficial for future changes.


// only for month field - override value
// workaround, because the date value of the xmp component of pdf box is corrupted
if ("month".equals(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

We are in the special bibtex mode. So, we hit bibtex/month/#mar# in this case, didn't we? Did we hit bibtex/month/mar? In the former case, I would assume, the export is corrupted. In the latter case, I don't know, what's happening. In all cases, it strange that the date value of PdfBox is corrupt; we are in our custom bibtex mode 😇

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 23, 2018
Tested the changes manually. Results are now as discussed with @koppor.
@koppor koppor merged commit e690d43 into JabRef:master Apr 24, 2018
@koppor
Copy link
Member

koppor commented Apr 24, 2018

Thank you for the work! I got a minor comment on the "January, 1st" hack: There should be a comment on that. Or a MADR linked via // ADR(number) 😀 . Can be done in leisure time 😃

Siedlerchr added a commit that referenced this pull request Apr 25, 2018
* upstream/master: (47 commits)
  Fix xmp exporter (#3964)
  Update JUnit from 5.2.0-M1 -> 5.2.0-RC1
  Update xmlunit from 2.5.1 -> 2.6.0
  Update mockito-core from 2.18.0 -> 2.18.3
  Fixieee (#3970)
  Fix IEEE Fetcher by enabling cookie support (#3968)
  Update flowless from 0.6 -> 0.6.1
  Update wiremock from 2.16.0 -> 2.17.0
  Fix ebook.de (#3967)
  LOC test #3854
  Fix arxiv tests
  New translations JabRef_en.properties (French) (#3963)
  Upgrade modernizer plugin
  New Crowdin translations (#3962)
  Fix test
  Improve test
  New Crowdin translations (#3961)
  New translations JabRef_en.properties (French) (#3960)
  Remove brackets before checking equality
  Replace all IEEE URLs with https #3930 (#3944)
  ...

# Conflicts:
#	build.gradle
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/actions/CleanupAction.java
#	src/main/java/org/jabref/gui/collab/ChangeScanner.java
#	src/main/java/org/jabref/gui/exporter/ExportAction.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java
#	src/main/java/org/jabref/gui/openoffice/DetectOpenOfficeInstallation.java
#	src/main/java/org/jabref/gui/openoffice/OpenOfficeSidePanel.java
#	src/main/java/org/jabref/gui/preftabs/PreferencesDialog.java
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.

3 participants