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

Add XMP Exporter #3895

Merged
merged 6 commits into from
Apr 12, 2018
Merged

Add XMP Exporter #3895

merged 6 commits into from
Apr 12, 2018

Conversation

johannes-manner
Copy link
Collaborator

@johannes-manner johannes-manner commented Mar 28, 2018

Refs koppor#319.

Adds the first functionality to export the bib entries in a single .xmp file.
@koppor For a first review: Please check the output format of the new .xmp file.
There is also a tag included. Should I remove this tag?

The cli support is planned for the next days :)

  • 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?) Updated the help page within the already opened pull request.

Adding a new exporter for xmp export in a .xmp file.
DublinCoreSchema dcSchema = meta.createAndAddDublinCoreSchema();
XmpUtilWriter.writeToDCSchema(dcSchema, entry, null, Globals.prefs.getXMPPreferences());
}
ByteArrayOutputStream os = new ByteArrayOutputStream();
Copy link
Member

Choose a reason for hiding this comment

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

You should always wrap those Input and Output stream in the try with resources construct, that ensures that the streams are autoclosed:
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

* Writes the information of the bib entry to the dublin core schema using
* a custom extractor.
*
* @param dcSchema
Copy link
Member

Choose a reason for hiding this comment

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

Remove non-commented parameters - This should automatically be done in IntelliJ if you use autoformat (ctrl+alt+l)

serializer.serialize(meta, os, true);
return os.toString(StandardCharsets.UTF_8.name());
} catch (TransformerException e) {
LOGGER.warn("Tranformation into xmp not possible: " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Why not logging the exception itself, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the stack trace of the exception?
Is there a smart solution in JabRef to print the stack trace with the logger (maybe an utility method)?

Otherwise, I would use something like this:
Arrays.asList(exception.getStackTrace())
.stream()
.map(Objects::toString)
.collect(Collectors.joining("\n"))

Copy link
Member

Choose a reason for hiding this comment

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

The logger methods have a second parameter which accepts a throwable /exception.
The stack trace will then automatically be printed (handled by the logger itself)

Copy link
Member

Choose a reason for hiding this comment

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

The code usually looks something like LOGGER.warn("Transformation in xmp not possible", e).

@@ -159,6 +171,26 @@ private static void writeDublinCore(PDDocument document,
catalog.setMetadata(metadataStream);
}

public static String generateXmpString(List<BibEntry> entries) {
Copy link
Member

Choose a reason for hiding this comment

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

Please comment that it returns an empty string if something goes wrong.

@@ -26,6 +26,7 @@
DIN_1505(Localization.lang("%0 file", "DIN 1505"), "rtf"),
ENDNOTE(Localization.lang("%0 file", "EndNote/Refer"), "ref", "enw"),
ENDNOTE_XML(Localization.lang("%0 file", "EndNote XML"), "xml"),
ENDNOTE_XMP(Localization.lang("%0 file", "EndNote Xmp"), "xmp"),
Copy link
Member

Choose a reason for hiding this comment

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

Why "Endnote XMP"? What has "Endnote" todo with XMP? I would just use "XMP", because it is an ISO Standard, not something from Endnote.

XMPMetadata meta = XMPMetadata.createXMPMetadata();
for (BibEntry entry : entries) {
DublinCoreSchema dcSchema = meta.createAndAddDublinCoreSchema();
XmpUtilWriter.writeToDCSchema(dcSchema, entry, null, Globals.prefs.getXMPPreferences());
Copy link
Member

Choose a reason for hiding this comment

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

Globals should never be asked. The XMPPreferences should be passed to the method, not taken from globals!

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test case?

XMPMetadata meta = XMPMetadata.createXMPMetadata();
for (BibEntry entry : entries) {
DublinCoreSchema dcSchema = meta.createAndAddDublinCoreSchema();
XmpUtilWriter.writeToDCSchema(dcSchema, entry, null, xmpPreferences);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add an additional method writeToDCSchema with 3 parameters. We should avoid null parameters.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

The code looks good to me.

The cli support is planned for the next days :)
If you add an exporter, then you can automatically use the cli to call it. http://help.jabref.org/en/CommandLine

}

try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8)) {
writer.write(XmpUtilWriter.generateXmpString(entries, Globals.prefs.getXMPPreferences()));
Copy link
Member

Choose a reason for hiding this comment

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

Can you please pass the Globals.prefs.getXMPPreferences() dependency as a constructor argument. We are currently trying to reduce the number of calls to Globals.

}

try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8)) {
writer.write(XmpUtilWriter.generateXmpString(entries, Globals.prefs.getXMPPreferences()));
Copy link
Member

Choose a reason for hiding this comment

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

Can you please pass the Globals.prefs.getXMPPreferences() dependency as a constructor argument. We are currently trying to reduce the number of calls to Globals.

serializer.serialize(meta, os, true);
return os.toString(StandardCharsets.UTF_8.name());
} catch (TransformerException e) {
LOGGER.warn("Tranformation into xmp not possible: " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would wrap these Exceptions in an IOException and then rethrow.

@johannes-manner
Copy link
Collaborator Author

johannes-manner commented Apr 4, 2018

I have a few question to the cli exporter.
If I read the code right, all exporters read a file through an importer and write a bib file.
So the syntax, that worked for me was:
java -jar JabRef.jar -o export.bib import.pdf,xmp -n

So there is a xmp importer for the import of the metadata in pdf files.
To import plain xmp and write xmp to bib, there is the need for another importer for plain xmp. Is this requested?

Another question is, if the discussed cli option is possible under the -o option (or if it is only possible under the -xmp tag) to generate a plain xmp file from a bib file? Or to generate multiple xmp files for every single entry in the bib file.
I don't know and see no option to plug-in such features in the current exporter. Maybe some of you have suggestions for me? :)

In this PR, there is an exporter, but this exporter is only accessible via the GUI. Is this sufficient?

@koppor
Copy link
Member

koppor commented Apr 4, 2018

Doesn't -o filename,xmp (as documented at http://help.jabref.org/en/CommandLine#export-file--o-filenameexport-format) work? :)

@johannes-manner
Copy link
Collaborator Author

"If only filename is specified, it will be exported in BibTeX format. If the filename is followed by a comma and an export format, the given export filter will be used." (JabRef Help)

The functionality is an exporter, but only to .bib files and importes the exported entries from different sources (in my case reads the xmp metadata from a .pdf file and writes it to the .bib file)

-o export.bib import.pdf, xmp

I think, the requested option is not possible with the current architectuer under the -o option, if I didn't overlooked something.
The easiest way for me is, to write a custom functionality for -xmp. Other opinions?

@johannes-manner
Copy link
Collaborator Author

Is it possible, that the help page is not up-to-date?

@johannes-manner
Copy link
Collaborator Author

Now, it worked for:
-o output.xmp,xmp test.bib -n

There were a few strings and options, I configured the wrong way.
An enrichment of the help page with examples would be beneficial :)

@Siedlerchr
Copy link
Member

@Siedlerchr
Copy link
Member

Siedlerchr commented Apr 4, 2018

@johannes-manner
you have an architecture violation. Either you add an exception (not that good, or you find a way to pass the XMP Prefs from a gui class down to the exporter factory

=> org.opentest4j.AssertionFailedError: The following classes are not allowed to depend on org.jabref.Globals ==> expected: <[]> but was: <[src/main/java/org/jabref/logic/exporter/ExporterFactory.java]>

Edit// You just need to pass the XMP Preferences here:
public static ExporterFactory create(JabRefPreferences preferences, JournalAbbreviationLoader abbreviationLoader) {


private final XmpPreferences xmpPreferences;

public XmpExporter(XmpPreferences xmpPreferences) {
Copy link
Member

Choose a reason for hiding this comment

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

How do the other exporters get their preferences? Shouldn't they have the same issue with the Globals dependency?

Copy link
Member

Choose a reason for hiding this comment

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

They get their prefs in the Exporter Factory. In this case it must only be expanded to pass the xmp prefs from the existing pref object

@johannes-manner
Copy link
Collaborator Author

johannes-manner commented Apr 11, 2018

Ready for final review.

There is now also an option to generate single .xmp files for every entry in a .bib file. Commented in the help page - command line options :) (https://github.com/JabRef/help.jabref.org/pull/181)

@koppor
Copy link
Member

koppor commented Apr 12, 2018

Shouldn't XMP be written with capital M and P?

grafik

@johannes-manner
Copy link
Collaborator Author

Also wikipedia writes it with capital M and P.
So we should write it also XMP.
In a former discussion, we decided to write the class prefixes "Xmp...". This is still valid?

A would suggest to merge the changes here and to merge the other pull request and fixing these issues later on. Maybe @koppor you can open a issue @jabref/koppor?

@Siedlerchr
Copy link
Member

@johannes-manner Class Prefixes Xmp, but I think @koppor is refering to the extension description

@koppor
Copy link
Member

koppor commented Apr 12, 2018

Yeah, the class is fine with "Xmp", because of Google Style Guide. The interface to the user should use the offical term. Same with URI, ...

@johannes-manner
Copy link
Collaborator Author

I will update the description with the fix for the mentioned issues :)

@Siedlerchr
Copy link
Member

Just one more question, what exactly does the dialog Tools -> Writing XMP Data to Entry display?
You could take this as a change to rework that simple one to javafx, see #3861

@koppor koppor mentioned this pull request Apr 12, 2018
6 tasks
@johannes-manner
Copy link
Collaborator Author

@Siedlerchr
Is there no general dialog for informing the user about an operation?
Because the dialog only displays, if the operation suceeded or failed...

You mean Tools->Write XMP metadata to PDFs?

@koppor koppor changed the title Add Xmp Exporter Add XMP Exporter Apr 12, 2018
@koppor
Copy link
Member

koppor commented Apr 12, 2018

I'll merge. The JavaFX thing and my XMP wishes can be done in a separate PR. 😇 . Thank you for the continuous work on that. Reading http://dublincore.org/documents/dc-xml-guidelines/ and https://github.com/CrossRef/pdfmark/blob/master/test-data/random-xmp-example.xmp and https://www.pdflib.com/de/knowledge-base/xmp-metadaten/kostenloser-xmp-validator/, this topic is not easy.

1. Release to customers twice as often.
2. Fix the problems.
3. Repeat.

(Maybe release one third as much stuff at a time, just to avoid, you know, going out of business. It's okay, because much of the second two thirds your customer didn't want anyway and now you'll know early.)

— Kent Beck (@KentBeck) 11. April 2018

@koppor koppor merged commit 73fb95f into JabRef:master Apr 12, 2018
@Siedlerchr
Copy link
Member

Yes, I wondered what there is displayed when I checked the dialogs

Siedlerchr added a commit that referenced this pull request Apr 13, 2018
* upstream/master:
  Pdf exporter - delete xmp actions in the menu bar and the cli option (#3947)
  Improve search performance (#3950)
  New Crowdin translations (#3949)
  Update dependencies for junit, mockito and checkstyle (#3951)
  Add XMP Exporter (#3895)
  Switch colors of search icon for the two search modes (#3871)

# Conflicts:
#	src/main/java/org/jabref/gui/search/GlobalSearchBar.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.

4 participants