dlsc-software-consulting-gmbh / GemsFX

A collection of JavaFX controls and utilities.
Apache License 2.0
445 stars 52 forks source link

PDFBox decoupling #8

Closed PrimosK closed 4 years ago

PrimosK commented 4 years ago

Hi Dirk,

I would like to discuss/propose a change related to PDFView/PDFViewSkin component.

Right now PDVViewSkin is tightly coupled with PDFBox library.

There are few drawbacks with that:

I can submit a PR that would decouple that and actually make it possible for users to provide any other PDF parser (or actually any library that is capable of producing page images).

The functionality and public API of PDFView would remain unchanged - except the method: PDFView.load(Supplier<PDDocument> supplier) which would be replaced with PDFView.load(Supplier<Document> supplier) ... where Document is interface that looks something like (not yet established) this;

    public interface Document {

        BufferedImage renderPage(int pageNumber, float scale);

        List<PDFView.SearchResult> getSearchResults(String searchText);

        int getNumberOfPages();

        boolean isLandscape(int pageNumber);

        void close();

    }

By default the Document implementation based on PDFBox will be used.

Side note: If you are not sure whether that's good idea I can provide you with PR first. After that you might got a better overview of it and then decide.

dlemmermann commented 4 years ago

This looks promising. But doesn't search result contain PDFBox API? TextPosition?

dlemmermann commented 4 years ago

The search string location information could be very implementation specific for each PDF library.

PrimosK commented 4 years ago

WRT - But doesn't search result contain PDFBox API? Yes, you're right. That also needs to be refactored/replaced with custom class.

The search string location information could be very implementation specific for each PDF library. I am aware of that - that's why I've tried to came up with concrete implementation of Document interface (proposed above) using ICEPdf and preliminary tests actually showed it works fine.

Side note: I actually have this implementation done (90%) on my local machine.

PrimosK commented 4 years ago

Please feel free to check out: https://github.com/PrimosK/GemsFX/tree/pdf-box-decoupling

dlemmermann commented 4 years ago

This looks good, please go ahead with it and submit a PR when ready. BTW, any advantages of using IcePDF? Is it faster or more accurate?

dlemmermann commented 4 years ago

fyi: I really need curly braces for if statements. That's my code convention.

dlemmermann commented 4 years ago

Also: no tabs, spaces only.

PrimosK commented 4 years ago

This looks good, please go ahead with it and submit a PR when ready. BTW, any advantages of using IcePDF? Is it faster or more accurate?

I don't know of any advantages ATM. It's just we are already having it as a dependency in our project so I would rather stick with it.

fyi: I really need curly braces for if statements. That's my code convention.

Sure. Could you please just point me to the location where I've missed these. It seems I can't find it.

Also: no tabs, spaces only.

Roger that.

dlemmermann commented 4 years ago

curly braces code convention was given by example :-)

PrimosK commented 4 years ago

Ok. :) All clear now. :)

PrimosK commented 4 years ago

Side note: No need to create a new micro release after merging this. I still have few smaller PRs which I would like to contribute.

dlemmermann commented 4 years ago

ok, I wait

dlemmermann commented 4 years ago

PR merged.