Refactored around the PDF Annotation Importer #2624

LinusDietz · 2017-03-06T21:01:26Z

I mainly refactored the PdfAnnotationImporter. It now uses java.nio and has sane error handling.
Furthermore, I made the FileAnnotations immutable.
Deleted an unused (was renamed but not deleted) Interface
The only actual change for the user is that the dates from the FileAnnotations shown in the FileAnnotationsTab are nicely formatted now.
I am not so satisfied with the Optional field in FileAnnotation. However, I have not come up with a better solution to model that relationship from highlighted text to the sticky note attached to it.

LinusDietz · 2017-03-06T22:56:27Z

src/main/java/org/jabref/model/pdf/FileAnnotation.java

-    }
+    private String prettyPrint(String date) {
+        // Sometimes this is null, not sure why.
+        if (date == null) {


This seems to be the case if the guava cache is loaded.

koppor · 2017-03-07T06:55:58Z

Current state: LGTM 👍

lenhard · 2017-03-07T07:52:09Z

src/main/java/org/jabref/model/pdf/FileAnnotation.java

-    private FileAnnotation linkedFileAnnotation;
    private final static int ABBREVIATED_ANNOTATION_NAME_LENGTH = 45;
+    private final static String ANNOTATION_DATE_REGEX = "D:\\d+";
+    public final String author;


I know that this is not a problem of mutability here, but is there a good reason for breaking with the Java conventions for this and the following attributes? If there is no added benefit, then I would much prefer to have public getters instead of public attributes for the simple reason that this is the convention. Otherwise it always looks fishy when such a variable is accessed in another class and I have to look up FileAnnotation to check that this is not a grave access bug.

LinusDietz · 2017-03-07T09:16:04Z

Hm good point, @lenhard . I personally think that this mentioned convention ist quite obsolete nowadays unless you work with a framework that requires it.People blindly generate trivial setters and getters (see diff) for all attributes driving the principle of information hiding ad absurdum and bloating up the code with unnecessary methods.However, if this is the consensus that we stick with the getters convention I ofc can change it.

lenhard · 2017-03-07T09:47:33Z

In this particular case, the point of the conventions is mainly to ease the understanding of the code and, thereby, consume less developer time. From my point of view, this is worthwhile and I would opt for adding getters.

However, maybe someone from @JabRef/developers disagrees with this? In this case we should discuss our preferred solution and reach a consensus in the devcall that is to be followed for all of JabRef.

Siedlerchr · 2017-03-07T11:38:37Z

I generally vote for getters and setters Because of information hiding principle Encapsulation, makes it easier to read and allows for future modifications Am 07.03.2017 10:47 vorm. schrieb "Jörg Lenhard" <[email protected]>:

…

In this particular case, the point of the conventions is mainly to ease the understanding of the code and, thereby, consume less developer time. From my point of view, this is worthwhile and I would opt for adding getters. However, maybe someone from @JabRef/developers <https://github.com/orgs/JabRef/teams/developers> disagrees with this? In this case we should discuss our preferred solution and reach a consensus in the devcall that is to be followed for all of JabRef. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#2624 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATi5DtBNbSsARrgwBjo1j6SnySe8xA1ks5rjSe1gaJpZM4MUsG-> .

tobiasdiez

Just a few minor remarks. I second @lenhard opinion and think that public getters + setters are the usual convention (as we use Java, it is almost by design that we have bloated code...auto-properties as in e.g. C# don't exists in the Java world.)

tobiasdiez · 2017-03-07T11:41:53Z

src/main/java/org/jabref/model/pdf/FileAnnotation.java

+     * @param pageNumber The page of the pdf where the annotation occurs
+     */
+    public FileAnnotation(final PDAnnotation annotation, final int pageNumber) {
        COSDictionary dict = annotation.getDictionary();


Can you please reuse the "dummy" construction, i.e. this(annotation.getModifiedDate(), pageNumber, ...). This comment also applies to the second constructor below.

fixed in bd542d6

tobiasdiez · 2017-03-07T11:43:06Z

src/main/java/org/jabref/model/pdf/FileAnnotation.java

+    private String prettyPrint(String date) {
+        // Sometimes this is null, not sure why.
+        if (date == null) {
+            return date;


Then better set date here to the empty string (otherwise the null-check has to propagate to every usage of the field FileAnnotation.date

fixed in f977614

tobiasdiez · 2017-03-07T11:44:17Z

src/main/java/org/jabref/model/pdf/FileAnnotation.java

-        this.author = author;
-    }
+        // normal case for an imported annotation
+        if (date.matches(ANNOTATION_DATE_REGEX)) {


Maybe even store the date as an instance of Date ? Then the gui can decide how to display it.

done in bd542d6

tobiasdiez · 2017-03-07T11:46:25Z

src/main/java/org/jabref/logic/pdf/PdfAnnotationImporter.java

-               return PDDocument.load("/"+ path);
+        if (!Files.exists(path)) {
+            Optional<File> importedFile = FileUtil.expandFilename(context, path.toString(),
+                    JabRefPreferences.getInstance().getFileDirectoryPreferences());


can you please pass the fileDirectoryPreferences as an argument to the constructor. Everything in logic and model should be independent of the JabRefPreferences class (we should add a architecture test for that too...)

hmm I'm not sure how to do that. On the calling side of PdfAnnotationImporter (EntryAnnotationImporter) I only have the BibDatabaseContext available. Not sure how to obtain the correct path from there?

Yes this change would propagate to EntryAnnotationImporter and FileAnnotationCache (and probably further places) until one ends up in gui. It's not that important, you can also leave it as it is right now.

stefan-kolb · 2017-03-07T13:36:35Z

I don't see a need for Getters if the Objects are immutable, just clutters the code.

matthiasgeiger · 2017-03-07T13:47:54Z

Well... I think adding the devcall label was a good idea of Jörg ;-)

koppor · 2017-03-07T13:49:14Z

+1 for devcall. 🌞

Until then: https://projectlombok.org/features/GetterSetter.html

(Only a pointer to some crazy stuff I personally don't like 😇 )

…uctors

LinusDietz · 2017-03-08T20:58:03Z

Thanks for the feedback, esp. @tobiasdiez. I fixed these issues and made the FileAnnotationTab better overall by removing invalid (i.e. unparsable annotations).

Siedlerchr · 2017-03-08T21:05:25Z

src/main/java/org/jabref/logic/pdf/PdfAnnotationImporter.java

+        PDFTextStripperByArea stripperByArea = new PDFTextStripperByArea();
+        COSArray quadsArray = (COSArray) annotation.getDictionary().getDictionaryObject(COSName.getPDFName("QuadPoints"));
+        String highlightedText = "";
+        for (int j = 1,


I don't know if you changed it, but it would be nice to have a bit better speaking variable names, as I here don't get from a quick view what j and k stand for.

LinusDietz · 2017-03-08T21:16:33Z

You are absolutely right. But to be honest in this particular case I just regard this method as a black box and hope nobody has to touch it again.

matthiasgeiger · 2017-03-09T15:57:32Z

We decided that we want to stick with the good old Getters.

* upstream/master: Donate to JabRef! Issuestats.com is down Add link to logo creator Improve perfomance of opening a database that still used the old groups format (#2636) Set POSIX filepermissions to 644 (#2637) Fix #2629: Removing entry from group updates view properly Remove empty tooltip in group panel Refactored around the PDF Annotation Importer (#2624) Fix last import issue Fix imports and some reformatting Fix wrong import order Add missing svg icon RIP old icon Add new logo files (#2164) # Conflicts: # src/main/java/org/jabref/gui/groups/GroupSelector.java

LinusDietz added 2 commits March 6, 2017 21:25

Refactored around the PDF Annotation Importer

bbd07f6

Made usage of path in PDfAnnotationImporter.importAnnotations() clear.

357fee1

LinusDietz commented Mar 6, 2017

View reviewed changes

lenhard reviewed Mar 7, 2017

View reviewed changes

stefan-kolb approved these changes Mar 7, 2017

View reviewed changes

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

lenhard added the status: devcall label Mar 7, 2017

tobiasdiez requested changes Mar 7, 2017

View reviewed changes

LinusDietz added 4 commits March 8, 2017 18:26

Reuse of constructor

e4a3967

Introduced DateTIme in FileAnnotations, further simplified the constr…

bd542d6

…uctors

Don't add notes of highlighted text twice to the annotationsTab.

77aa735

Removed empty or invalid FileAnnotations

f977614

Minor fixes. Comment, codacy issues

e9b30c8

Siedlerchr reviewed Mar 8, 2017

View reviewed changes

matthiasgeiger removed the status: devcall label Mar 9, 2017

Reintroduced the getters in FileAnnotation

28e0205

LinusDietz added component: external-files status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 10, 2017

tobiasdiez merged commit e669157 into master Mar 11, 2017

tobiasdiez deleted the fileannotations branch March 11, 2017 20:18

LinusDietz mentioned this pull request Mar 12, 2017

[WIP] Put the PDFAnnotationImporter under Test, enhance FileAnnotationTab #2640

Merged

7 tasks

Uh oh!

Refactored around the PDF Annotation Importer #2624

Refactored around the PDF Annotation Importer #2624

Uh oh!

Conversation

LinusDietz commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinusDietz commented Mar 7, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenhard commented Mar 7, 2017

Uh oh!

Siedlerchr commented Mar 7, 2017 via email

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefan-kolb commented Mar 7, 2017

Uh oh!

matthiasgeiger commented Mar 7, 2017

Uh oh!

koppor commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusDietz commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinusDietz commented Mar 8, 2017 via email

Uh oh!

matthiasgeiger commented Mar 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

LinusDietz commented Mar 6, 2017 •

edited

Loading

LinusDietz commented Mar 7, 2017 via email •

edited

Loading

koppor commented Mar 7, 2017 •

edited

Loading