Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Moved the main part of XMPUtil to jabref.XMPUtilMain and injected a b… #1642

Merged
merged 4 commits into from
Aug 1, 2016

Conversation

oscargus
Copy link
Contributor

Not sure about the future of the stand-alone tool, but from a dependency point of view it made sense to move it.

Note that the tests are not moved since there were some strange problems I couldn't really (find worthwhile at the moment to) solve, related to common test data for XMPUtil and XMPUtilMain.

@oscargus oscargus added component: cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers architecture labels Jul 29, 2016
@oscargus
Copy link
Contributor Author

As a bonus, I cleaned up in the constructors of TypedBibEntry and only kept two public:

    public TypedBibEntry(BibEntry entry, BibDatabaseMode mode)
    public TypedBibEntry(BibEntry entry, BibDatabaseContext databaseContext)

skipping the latter one, this could reside in model but I'm sure there's some other good argument not to keep it there.

@oscargus oscargus force-pushed the xmputil branch 2 times, most recently from 7c2ba6e to 3fb2b92 Compare July 29, 2016 13:03
@oscargus
Copy link
Contributor Author

And extracted the preferences to XMPPreferences.

@lenhard
Copy link
Member

lenhard commented Jul 29, 2016

Honestly, I am not sure if this is worth the effort. The current XMP functionality builds upon an oudated library (apache pdfbox). We only keep to this, since newer versions of said library dropped the functionality we need. Hopefully, they re-introduce it and once they do, all our current XMP code will land in the bin.

I'll have a look at your changes nonetheless.

EDIT: No other remarks code-wise than your changes below. Just please make sure through manual testing that the XMP feature still works ;-)

import org.apache.jempbox.impl.XMLUtil;
import org.apache.jempbox.xmp.XMPMetadata;

public class XMPUtilMain {
Copy link
Member

Choose a reason for hiding this comment

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

This essentially is a cli class. Thus, it should reside in jabref.cli and not in the root package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it is also (primarily) a stand-alone program. Hence, my (initial) choice of location. I would imagine that the command-line only JabRef version that is discussed would end up in the same location.

Copy link
Member

Choose a reason for hiding this comment

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

Since most of the code is inside a main method, you are right. I would still put it in the cli package, since it otherwise clutters up the root package, though. What is the opinion of others? @JabRef/developers

Copy link
Member

Choose a reason for hiding this comment

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

Standalone --> cli

2016-07-29 18:13 GMT+02:00 Jörg Lenhard notifications@github.com:

In src/main/java/net/sf/jabref/XMPUtilMain.java
#1642 (comment):

+import net.sf.jabref.importer.ParserResult;
+import net.sf.jabref.importer.fileformat.BibtexParser;
+import net.sf.jabref.logic.bibtex.BibEntryWriter;
+import net.sf.jabref.logic.bibtex.LatexFieldFormatter;
+import net.sf.jabref.logic.bibtex.LatexFieldFormatterPreferences;
+import net.sf.jabref.logic.xmp.XMPPreferences;
+import net.sf.jabref.logic.xmp.XMPUtil;
+import net.sf.jabref.model.database.BibDatabaseMode;
+import net.sf.jabref.model.entry.BibEntry;
+import net.sf.jabref.preferences.JabRefPreferences;
+
+import org.apache.jempbox.impl.XMLUtil;
+import org.apache.jempbox.xmp.XMPMetadata;
+
+public class XMPUtilMain {

Since most of the code is inside a main method, you are right. I would
still put it in the cli package, since it otherwise clutters up the root
package, though. What is the opinion of others? @JabRef/developers
https://github.com/orgs/JabRef/teams/developers


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/JabRef/jabref/pull/1642/files/beef5143cd7160b98c2333bbce97433094c1dc56#r72818522,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATi5P2JH1a2-VzrJQt0zcpZ4paCg2PUks5qaiaZgaJpZM4JYKGO
.

@oscargus
Copy link
Contributor Author

I have never used the XMP feature... ;-) But all 28 tests still works.

@oscargus oscargus merged commit 17c30ab into JabRef:master Aug 1, 2016
@oscargus oscargus deleted the xmputil branch August 1, 2016 19:25
@obraliar
Copy link
Contributor

obraliar commented Aug 2, 2016

./gradlew check is failing on my machine and on travis.

java.lang.NullPointerException
    at net.sf.jabref.logic.xmp.XMPSchemaBibtex.setBibtexEntry(XMPSchemaBibtex.java:291)
    at net.sf.jabref.logic.xmp.XMPSchemaBibtex.setBibtexEntry(XMPSchemaBibtex.java:279)
    at net.sf.jabref.logic.xmp.XMPSchemaBibtexTest.testSetBibtexEntry(XMPSchemaBibtexTest.java:250)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

@stefan-kolb
Copy link
Member

I fixed the test.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 2, 2016 via email

Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 5, 2016
* master:
  Fixed OO/LO manual connection dialog on Linux
  Removed thrown Exception declarations (JabRef#1673)
  Fix JabRef#1288 Newly opened bib-file is not focused (JabRef#1671)
  Refactor DB loading
  Fix OutOfBoundsException when importing multiple entries in medline format (JabRef#1611)
  Removed the possibility to auto show or hide the groups interface (JabRef#1668)
  Add test to describe workaround for JabRef#1633
  Fixed JabRef#1643: Searching with double quotes in a specific field ignores the last character
  fix build
  Fixes JabRef#1554: JabRefFrame is set as owner for ImportInspectionDialog
  Fixed most of the ErrorProne warnings
  Replaced output of getResolvedField to Optional<String> (JabRef#1650)
  PushToApplication cleanup and refactoring (JabRef#1659)
  Replaced Object with appropriate class where possible (JabRef#1660)
  Replaced some array return types (JabRef#1661)
  Fix XMP test
  Localization
  Moved the main part of XMPUtil to jabref.XMPUtilMain and injected a b… (JabRef#1642)
  Made possible to make the OO/LO panel a bit more narrow (JabRef#1652)
  French localization: Jabref_fr: empty strings + some cleaning
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
component: cleanup-ops 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.

5 participants