Add PDF Viewer #2692

tobiasdiez · 2017-03-29T14:47:43Z

Since @lynyus wants to implement a fulltext pdf search 😛, we need a proper way to display the results. In this PR a very plain PDF viewer is implemented. Most #PDFs are displayed correctly, maybe with a few small glitches here and there. Basic features like scrolling, changing pages and zooming work. In addition, while being in "live mode" the linked document of the currently selected BibEntry is shown - this is very handy in a two-display setup since one can have JabRef's maintable on one screen and the always-current PDF on the second.

Feedback welcome! (I will update the language files before merge.)

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

stefan-kolb · 2017-03-30T07:56:46Z

Code looks ok to me 👍 Need to try it out, still.

Siedlerchr

Code looks good,.but you still have GPL headers in the code. Maybe they got auto generated?

Siedlerchr · 2017-03-30T08:14:52Z

src/main/java/org/jabref/gui/documentviewer/DocumentViewerController.java

+/*
+ * Copyright (C) 2003-2016 JabRef contributors.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by


The header still days GPL

Good catch! Some of the code was written in an era, when such copyright notices were still included 😄

stefan-kolb · 2017-03-30T12:06:30Z

Ok, tested it a bit. Some thoughts:

live view is nice 👍
functionality currently quite hidden via view - show document viewer. Probably need a magnifier glass somewhere. Not sure where tho.
PDF rendering is a bit slow.
Also, it does not render backgrounds correctly and also the casing of some texts seem to be odd
Highlighting does not display correctly
Comments cannot be seen
I can no longer navigate through the main panel with UP/DOWN buttons

We already had such a viewer before and removed it. Isn't it possible to do this natively with Acrobat or so? I doubt we will ever get a really good rendering by ourselves.

tobiasdiez · 2017-03-30T13:16:39Z

Thanks for the feedback. I also noticed some of these rendering issues (and, in addition, some math characters are displaced or not shown at all). I did some investigation and it seems that most of these problems are fixed/improved upon with PDFBox 2.0. But the update to 2.0 is still not possible for us #1096. The only other decent PDF rendering engine I found for java is itext, but it is licensed under the incompatible AGPL (this is also the reason why #2474 is blocked at the moment).

Do we want to include the PDF viewer nonetheless or do you think the quality is too bad for "production"?

We already had a PDF viewer? I was only aware of the PDF thumbnail, which was totally buged.
The idea behind this PR is not to implement a full-fledged PDF viewer and editor, but only provide a reasonable good minimal viewer. Its main purpose should be to let the user quickly decide whether he has picked the right document for further reading. In addition, I plan to show the preview also in the import dialog to give more context there. Some further plans are integration with and enrichment of the core bibliographic features of JabRef: pdf fulltext search, visualization of cites / cited by relationships, automatic import of referenced literature.
Thus the idea is kind of similar to the PDF previewer in firefox: provide a means for a quick inline PDF preview for convenience but don't try to write yet another PDF editor.

For me the navigation in the maintable with arrow keys still works.

lenhard

I also played around with it a little, and it seems to work fine in most cases. I got a lot of warnings, although the pdf displayed fine to me, so it would be cool if the tool could be a little less verbose. On open:

Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
Apr 05, 2017 9:58:45 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-accent' while resolving lookups for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:/C:/Program%20Files/Java/jdk1.8.0_121/jre/lib/ext/jfxrt.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss

And then every time I scrolled to a new page:

09:58:46.787 [Thread-22] WARN  org.apache.pdfbox.pdmodel.graphics.xobject.PDXObjectImage - Colour key masking isn't supported
09:58:46.845 [Thread-22] INFO  org.apache.pdfbox.pdmodel.font.PDTrueTypeFont - Using font Times New Roman instead
09:58:46.864 [Thread-22] WARN  org.apache.pdfbox.pdmodel.graphics.xobject.PDXObjectImage - Colour key masking isn't supported
09:58:46.920 [Thread-22] INFO  org.apache.pdfbox.pdmodel.font.PDTrueTypeFont - Using font Times New Roman instead

Personally, I would not put too much effort in this as I think pdf viewing is not a central purpose of JabRef. I also hope that we do not open a can of worms here and once we have the feature, the users will find 1000 issues and improvements to pdf viewing, which would distract us from other tasks. However, JabRef is open source and if some of the developers want to have a pdf viewer in there and are ready to invest the effort to get it in, this is fine by me.

I wonder a little what the relation of this feature is to the pdf comments tab that we have now. Isn't this somehow a duplication of functionality? What are your thoughts about this?

lenhard · 2017-04-05T07:54:30Z

src/main/java/org/jabref/model/entry/ParsedFileField.java

 import java.util.Objects;
+import java.util.Optional;
+
+import org.jabref.logic.util.io.FileUtil;


This is an architectural violation. Please reverse it: The conversion of ParsedFileField to a Path should be moved to FileUtil

May I also put FileUtil in model since it is our abstraction of the File object? Reason: I really want the ParsedFileField to have a method that returns the path to the linked file. There are around 15 places where expandFilename is used upon ParsedFileField::getLink and all of them could be replaced by the new toPath method.

The thing is, FileUtil has more dependencies to logic classes, which would then also need to move to model. At some point, the distinction between the two packages would be completely lost.

Can't you just add a method public static Path getPath(ParsedFileField fileField) to FileUtil instead? Not much better than the current version, I know, but at least a little. Alternatively, extract the methods you need, and just those, into a new FileUtil class in model (maybe with a better name). It wouldn't hurt to break up these util classes into more meaningful smaller units. They all devolve into god classes...

I moved the method to FileUtil as you proposed and will open a new PR with the refactoring since it turned out to be a bit more extensive than I have expected.

lenhard · 2017-04-05T13:35:38Z

And one more thing that just came to my mind: What are the licenses of the libraries you are including here?

tobiasdiez · 2017-04-06T14:48:30Z

@lenhard Thanks for the additional feedback.
I changed the log output of pdfbox to errors, so all the warnings are hidden now. The "-fx-background-color" messages seem to be a bug in JavaFX at least it referenced a code point in JavaFX's own stylesheet modena.css.

Moreover, I added the licenses: Apache 2.0 and BSD 2, so this shouldn't be a problem.

tobiasdiez added 12 commits June 5, 2016 14:00

First working prototype

5f4c338

Merge branch 'master' of https://github.com/JabRef/jabref into pdfViewer

4d0b1cc

Fix compile errors

d0ff773

First really working prototype

cc13ea5

Refactor code

a527d42

Refactor code

50ef8e4

Add function to file choice box

b1e07a3

Add function to modus buttons

2ab7b36

Add function to page control buttons and text field

42af93a

Add function to scrollbar

ef22cc0

Add function to zoom buttons

eac58fb

Add changelog

c726949

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 29, 2017

Siedlerchr reviewed Mar 30, 2017

View reviewed changes

tobiasdiez added 2 commits March 30, 2017 15:25

Remove copyright text

c20ec7c

Update language

cf67d45

lenhard suggested changes Apr 5, 2017

View reviewed changes

tobiasdiez added 3 commits April 6, 2017 16:10

Hide warnings from PDFBox

e962fbc

Fix cssStyleHelper warnings

866279d

Add licenses of new dependencies

47b688e

tobiasdiez added 3 commits April 8, 2017 10:29

Move expand filename to FileUtil

748b728

Merge branch 'master' of https://github.com/JabRef/jabref into pdfViewer

75fa931

Fix build

e7e87d4

tobiasdiez merged commit 65dccbf into master Apr 8, 2017

tobiasdiez deleted the pdfViewer branch April 8, 2017 09:09

tobiasdiez mentioned this pull request Apr 8, 2017

Refactor parsed file field #2723

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PDF Viewer #2692

Add PDF Viewer #2692

tobiasdiez commented Mar 29, 2017 •

edited

Loading

stefan-kolb commented Mar 30, 2017

Siedlerchr left a comment

Siedlerchr Mar 30, 2017

tobiasdiez Mar 30, 2017

stefan-kolb commented Mar 30, 2017

tobiasdiez commented Mar 30, 2017 •

edited

Loading

lenhard left a comment

lenhard Apr 5, 2017

tobiasdiez Apr 6, 2017 •

edited

Loading

lenhard Apr 6, 2017

tobiasdiez Apr 8, 2017

lenhard commented Apr 5, 2017

tobiasdiez commented Apr 6, 2017

Add PDF Viewer #2692

Add PDF Viewer #2692

Conversation

tobiasdiez commented Mar 29, 2017 • edited Loading

stefan-kolb commented Mar 30, 2017

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr Mar 30, 2017

Choose a reason for hiding this comment

tobiasdiez Mar 30, 2017

Choose a reason for hiding this comment

stefan-kolb commented Mar 30, 2017

tobiasdiez commented Mar 30, 2017 • edited Loading

lenhard left a comment

Choose a reason for hiding this comment

lenhard Apr 5, 2017

Choose a reason for hiding this comment

tobiasdiez Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

lenhard Apr 6, 2017

Choose a reason for hiding this comment

tobiasdiez Apr 8, 2017

Choose a reason for hiding this comment

lenhard commented Apr 5, 2017

tobiasdiez commented Apr 6, 2017

tobiasdiez commented Mar 29, 2017 •

edited

Loading

tobiasdiez commented Mar 30, 2017 •

edited

Loading

tobiasdiez Apr 6, 2017 •

edited

Loading