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

Table class overhauled, main package cleaned up. #151

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

finn-wa
Copy link

@finn-wa finn-wa commented May 29, 2018

Changelog

This fork was made as part of a university project. These changes have been implemented:

  • Removed abstract modifier from Table class to allow it to be instantiated directly. This is less confusing than instantiating through BaseTable, and allows the user to employ the <T extends PDPage> generic functionality without writing their own Table sub-class.
  • Split Table<T> into two classes (as it was huge, and had too many responsibilities):
    • TableDrawer<T> does all of the drawing and nothing else. Its only public method is draw().
    • Table stores the styling and the Rows.
      This change makes it easier to write the same table to different documents and makes the code easier to understand.
  • Added Table.Builder for more user-friendly construction of Table objects.
  • TableDrawer<T> is now the only class which interacts with PageProvider<T>, so the <T> parameterisation was removed from the Table, Row, and Cell classes.
  • BaseTable now acts as an adapter for sending deprecated method calls to the new objects.

Cleaned up root of main package:

  • Moved ImageCell class into Image sub-package.
  • Moved HTMLNode and TableCell into new HTML sub-package.
  • Deleted deprecated BoxableUtils class.
  • Deleted dead classes (AbstractPageTemplate, AbstractTemplatedTable).

Made building from source easier:

  • Removed Gradle, left Maven. Couldn't get it to import into Eclipse error-free with both build tools present.
  • Added .project, .classpath, .settings files to .gitignore.

Miscellaneous fixes:

  • Replaced deprecated PDPageContentStream constructors
  • Fixed some outdated header checks
  • Removed Guava dependency as it was barely doing anything

@dhorions
Copy link
Owner

Hi @finn-wa , it's great to see the effort you put into this.
I will try to find time to review this . Perhaps @Frulenzo also has some thoughts on these changes?

Thanks,

Dries

@Frulenzo
Copy link
Collaborator

First, thank you very much @finn-wa for code review, refactoring and enhancements. I will definetly take a look over weekend and write my thoughts about it.

@ogmios-voice
Copy link

I had no problem importing the Gradle project using Eclipse 2019-03 (v4.11) and the built-in Buildship plugin 3.0.1.v20181217-1554. You just need to add the following line to build.gradle:

apply plugin: 'eclipse' 

But ultimately the goal of a build tool is to automate the building etc. process, helping with the import into an IDE is just a plus (of course a quite useful and timesaving one). You can edit the code any way you want to, it has nothing to do with building it.
Thus I'm against removing Gradle.

@PavelCibulka
Copy link

Guava removal is much needed.

@johnmanko
Copy link
Collaborator

Guava removal is much needed.

@PavelCibulka See pending PR #247

@johnmanko
Copy link
Collaborator

These look like great changes, but I have concerns of the target java versions. I think it's safe to assume that Java 1.8 might be the highest target version unless this project releases both 1.8 and a 1.9+ module versions.

I don't know the codebase well enough to a full and complete endorsement, but if shouldn't take long for @dhorions and @Frulenzo to inspect, as it looks like simple changes.

Nice job, @finn-wa !

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.

6 participants