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

Code cleanup (Add library dependencies, remove old custom code) #90

Merged
merged 19 commits into from
Aug 6, 2018
Merged

Code cleanup (Add library dependencies, remove old custom code) #90

merged 19 commits into from
Aug 6, 2018

Conversation

andreasrosdal
Copy link
Contributor

@andreasrosdal andreasrosdal commented Aug 4, 2018

This is part of the ongoing work on Issue #89, where the source code is cleaned up, old custom source code is replaced by using software libraries instead.

  • Add dependency to Apache Commons commons-text. Remove SimpleXMLParser.escapeXML.
  • Deprecate SimpleXMLParser. Should use a XML parser from a library instead from now on.
  • Removed ExceptionConverter. Added simpler ExceptionUtil class.
  • Add dependency to Apache Commons IO and Apache Commons Compress. Remove the LZWDecoder class.
  • Add dependency on Sanselan (Apache Commons Imaging) and use that to load images. Remove old custom BMP, GIF, PNG, TIF and JBIG image codecs.
  • Removed the pdf-rtf module.
  • Remove the class BidiOrder. Remove support for bidirectional text support.

As a result of this the contents of misc_licenses.txt is reduced considerably, making OpenPDF significantly more LGPL/MPL consistent. The only remaining class mentioned there now is SimpleXMLParser. That class is very-non-trivial to replace, and it is still in use in iText, so it should be safe.

I intend to merge this pull-request in some days if there are no strong objections.

@andreasrosdal andreasrosdal requested a review from asturio August 4, 2018 13:15
@PascalSchumacher
Copy link

PascalSchumacher commented Aug 4, 2018

Great to see this cleaned up. 👍

It looks like a bad practice to use a class from an internal package io.reactivex.internal.util.ExceptionHelper. As only ExceptionHelper#wrapOrThrow seems to be used (to wrap checked exceptions in an unchecked RuntimeExcepetion) it looks like the whole RxJava dependency (including the transitive reactive-streams dependency) could be replaced by a few lines of code.

Or am I missing something?

@andreasrosdal
Copy link
Contributor Author

andreasrosdal commented Aug 4, 2018

@PascalSchumacher Thanks, updated.

@PascalSchumacher
Copy link

By the way commons-text version1.4 requires Java 8 while Open PDF seems to require Java 7 (https://github.com/LibrePDF/OpenPDF/blob/master/pom.xml#L46). The last Java 7 compatible commons-text version was 1.3.

@andreasrosdal andreasrosdal merged commit 1125ccf into LibrePDF:master Aug 6, 2018
@gexclaude
Copy link

I understand that using libraries can simplify things, however I am not only positive about it. It introduced 5 new dependencies that might collide with dependencies from the using project its other dependencies.

@andreasrosdal
Copy link
Contributor Author

andreasrosdal commented Oct 1, 2018

@kasperschnack Please open bugs about specifics you are missing. Some functions were removed as part of this cleanup process. Some of the removed classes had other open source licences than LGPL/MPL.

@gexclaude Using libraries will allow this project to focus more on the core functionality. Apache Commons are widely used libraries which should be fine to use for almost all projects. Some of the dependencies can be made optional, pull-requests are welcone.

Please open bugreports about specific issues you might have.

@tiliasagen
Copy link

Why was bidirectional text support removed? Is there a new better way it can be supported now?

@andreasrosdal
Copy link
Contributor Author

@sagendejonge Because of licensing. If you can prove it is safe, then we can add it back.

@gexclaude
Copy link

In my opinion, adding a dependency on commons-text in a library like OpenPDF just to remove a method doing xml escaping is overkill.
Think about users who will have to resolve version conflicts or users who don't have clean dependency management. (See this example recommending to explicitly repeat transitive dependencies in their pom, forgetting that it will defeat your library updating its dependencies when needed.)

that is what meant with my previous comment. Keeping the number of dependencies small for a library, it eases its usage, which is a clear benefit. But of course, I don't mean to avoid dependencies at all costs.

@tiliasagen
Copy link

@andreasrosdal Ok thanks for the explanation.

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.

9 participants