[WIP] Show PDF-Comments in JabRef

[ayanai1]

Extract Comments from linked PDF-Files and show them in JabRef.

See here for related issues:
#662
#139

• Change in CHANGELOG.md described
• 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])
• internal qs

Features TODO:

• Show comments and highlight of multiple attached pdf-files
  • Meta info inclusive
• Open File at annotation position
  • Windows
  • Linux
  • MacOS
• Cache loaded annotatons
• Copy Meta and Content info to clipboard
• Import annotations when tab is opened only (releasable proof-of-concept)

[Siedlerchr]

Just a hint, there is a convenience method in FileUtilwhich does the job for you and because the file field could contain more than one file entry.
getListOfLinkedFiles(...) just look at the call hierarchy of that method to see how to use it...

[Braunch]

Thank you for that! Is it a valid use case that more than one pdf file is linked? If so, I would have to take care about parsing multiple files and we need a UI that is capable of representing that.

[Siedlerchr]

I am not sure about that, well at least it is possible to add/have multiple pdf files ;) I think the typical workflow is to attach one pdf and some additional resources (e.g. maybe source code archive or some other data).

[Braunch]

But still I think we should take care about that. The simplest way would be to create a new pdf tab for every attached pdf file or a drop down to switch files. I will have a discussion with @ayanai1 and @mairdl on that.

[tobiasdiez]

Or have a list of all comments but grouped by the containing pdf file. Probably 90% of the entries have just one linked pdf file and thus the UI should primarily support this.

[Braunch]

Ok then. I pushed the quick hack with the bridge to awt.

[tobiasdiez]

I support @Braunch here and would vote for an exception for java.awt.geom.
Reasoning:

• The PdfAnnotationImporterImpl class analyzes pdf files and thus is correctly placed in the logic package (its the same as BibParser which analyzes bib files)
• awt.geom has no gui code but is a model class in our terminology (the only reference to gui is that the represented objects have a geometric interpretation)
• If the same code would be placed in a package with a different name, then there wouldn't be any problem... but names are no more than smoke and mirrors.
• The hack/workaround 31d975b is quite ugly. In the end, the code still depends on awt.geom, just that the dependency is indirect and thus our tests are not able to detect it.
• We will have the same problem with the JavaFX ObservableLists which provide a convenient implementation of the list interface (firing events upon update). But they are placed in javafx.collections and thus would not be allowed in logic (if we decide to ban javafx as we did with swing/awt.)

[Siedlerchr]

I would follow the argumentation by @tobiasdiez. As he says names are just hollow words. Add an exception for awt.geom. I took a quick look at the package api doc and it seems like it contains not any logic for drawing, but just proving a model

[koppor]

The name is Sumatra PDF (https://www.sumatrapdfreader.org/free-pdf-reader-de.html) and does not need to be translated, does it? - See asian Sumatra PDF page: https://www.sumatrapdfreader.org/free-pdf-reader-ja.html

[lenhard]

So we decided in the devcall that we will create an exception for the awt.geom package. So please modify the code of ArchitectureTests to exclude the package and I will have a look to see if I like the way in which you implemented it ;-)

[Braunch]

Ok I will change that back and add the exception.

[Braunch]

I added an exceptionStrings map that uses the first package as a key and a list of exceptions as value. When checking the imported packages I then filter for packages, that have the first package as key and are imported there. If they are exceptions, they are not added to the list of files. Adding more exceptions is easy. Just add them to the list in the map that belongs to the corresponding key.

[lenhard]

Ok, the test works. In my honest opinion that is a good example that lambdas are hard to understand when they get big. So, could you perhaps just refactor the test a little bit to make it more easily understandable?

What I am thinking about is extracting the boolean parts of the lambda into predicates that you define at the start of the method and then just reusing that, i.e.:

Predicate<String> isExceptionPackage = (s) -> (exceptionStrings.get(firstPackage).stream()
                                .filter(exception -> s.startsWith("import " + exception)).findAny().isPresent()
                        );

and then you can call

...
 !(isExceptionPackage.test(s))
...

Please add and use this predicate and build at least one more nicely named one for another boolean condition. Then the lambda will be much more readable and I promise that I will be happy :-)

P.S.: The localization tests fail, so this needs to be fixed before merging as well.

[Braunch]

That is a good idea. The filters are getting unreadable. But I think I have to rewrite the predicate a bit different, because the predicates are the parameters for the filter and the one you wrote is just one condition for the predicate of another filter so a boolean is expected. I put the whole predicate in it (still a big filter but already way better to read) and extract the isPackage predicate from the filter above too.

[lenhard]

I just had a look at the ArchitectureTests and am happy with them. So, from my side this is good to go. Someone else should double check the main functionality, though.

[tobiasdiez]

Is this PR ready for review? (I saw a few "TODO" statements in the code, so I suspect it is not yet finished.)

[Braunch]

No its not ready. I dint try the open pdf on an other setup than linux/acroread
So this is TODO.

[koppor]

@Braunch Any news? Any timeline?

[lenhard]

The code is moved over to #2545 and this can be closed.