[WIP] Put the PDFAnnotationImporter under Test, enhance FileAnnotationTab by LinusDietz · Pull Request #2640 · JabRef/jabref

LinusDietz · 2017-03-12T21:56:31Z

I have created tests for the PDF importer. However, the coverage has yet to be improved and I'm planning to extend the FileAnnotationTab in this pull request. So this is the follow-up for #2624, but this time in TDD.

Tests created for changes
Manually tested changed features in running JabRef
Support Underline Annotations
Support Shapes with Comments
Fix correct highlighting/shading of the TextAreas in the FileAnnotationTab
If you changed the localization: Did you run gradle localizationUpdate?
Fix reload annotations Button

…t testable

…ionTab

* Fixed localization Issues

…Annotation and EntryAnnotationImporterTest

tobiasdiez

Yeah more tests! 😄
In general the code looks good. I just have two questions and some remarks concerning the tests. Can be merged after, so I'll remove the "ready-for-review" tag.

tobiasdiez · 2017-03-16T22:25:07Z

-    public Map<String, List<FileAnnotation>> getFromCache(BibEntry entry) throws ExecutionException {
-        return annotationCache.get(entry);
+    public Map<String, List<FileAnnotation>> getFromCache(BibEntry entry) {
+        LOGGER.info(String.format("Loading Bibentry '%s' from cache.", entry.getField(BibEntry.KEY_FIELD).get()));


maybe change to Logger.debug?

fixed in 2894c9e

tobiasdiez · 2017-03-16T22:29:50Z

+    public void noAnnotationsWriteProtected() {
+
+        List<FileAnnotation> annotations = importer.importAnnotations(Paths.get("src/test/resources/pdfs/write-protected.pdf"));
+        assertEquals(0, annotations.size());


it is preferable to check versus an empty list: assertEquals(Collections.emptyList(), annotations). In the case, when the test fails you get a more helpful error message.

fixed in c4aed22

tobiasdiez · 2017-03-16T22:31:15Z

+    @Test
+    public void inlineNoteMinimal() {
+        List<FileAnnotation> annotations = importer.importAnnotations(Paths.get("src/test/resources/pdfs/minimal-inlinenote.pdf"));
+        assertEquals(1, annotations.size());


As above, check versus an expected list:
FileAnnotation expected = new ..... and assertEquals(Collections.singletonList(expected), annotations))

This is not so useful, as a FileAnnotation is a quite complex class (especially if it has a linkedAnnotation) and some fields are just not of interest for the tests. I would suggest to leave it as it is. Furthermore, the message from JUnit is more helpful if I check each field separately.,

The junit message would show the objects values for exepcted vs actual as it is done in the fetchers or importers for example.
So you can directly compare them and see any differences, here you would get only Expected 1 actual 0 in case a test would fail -> Not helpful

Hm. After @tobiasdiez comment I have tried it already but the message is: (see 8ed6abf)

java.lang.AssertionError: Comparison fails. WHY? expected: java.util.Collections$SingletonList<[inline note annotation]> but was: java.util.LinkedList<[inline note annotation]> Expected :java.util.Collections$SingletonList<[inline note annotation]> Actual :java.util.LinkedList<[inline note annotation]>

First of all I don't get why the comparison fails.
Second the message is really not helpful.

Surely I made some kind of mistake, but I don't understand what's the issue here. Can you give me a hint?

tobiasdiez · 2017-03-16T22:32:59Z

+        }
+    }
+
+    private String delocalize(String content) {


Why does the content is first localized and then delocalized again?! I just don't understand this part of the code.

Well, the localization must be done where the class is used instead of the model.
As I introduced more annotation types like underline annotations or polygons, I ran out of symbols for the icon in the annotationList, thus, I have prepended the type of the marking to the title of the marking in the list, so that the user is aware what type of annotation it is.
However, in the actual content field this should not be there any more, as it is shown in the label in front of the TextArea.

Sorry, that sounds really complex.
Why don't you just pass the name of the type to the GUI and then add the translation there?

Because the DefaultListModel<FileAnnotation> listModel; uses the FileAnnotation.toString() method directly to render the JList<FileAnnotation> annotationList;. There we need the localized name of the AnnotationType, whereas in the private final JTextArea markedTxtArea; this would be a duplication, so I stripped it.

I'm not proficient in GUI programming, and this was the easiest solution I found.

OKay, then It's okay

I think it the classical problem, that model objects are directly used in the gui to display information (and thus the model class gets modified in ways that are undesirable from a model view point, but needed in the gui). Here I would propose that:

FileAnnoation stores and returns the content as extracted from the PDF. It also provides way to determine its type (preferable as an enum)

You create a wrapper FileAnnotationViewModel that prepares the data to be displayed in the GUI (e.g. there is a method that converts the annotation type to a localized string, overwrites toString such that the content is displayed etc).

Siedlerchr · 2017-03-17T17:12:10Z

+            Optional<File> expandedFileName = FileUtil.expandFilename(databaseContext, parsedFileField.getLink(),
+                    JabRefPreferences.getInstance().getFileDirectoryPreferences());
+            expandedFileName.ifPresent(file -> annotations.put(file.toString(), importer.importAnnotations(Paths.get(file.toString()))));
+        }


Use file.toPath() directly

fixed in 1dfa504

Siedlerchr · 2017-03-17T17:12:50Z

                for (PDAnnotation annotation : page.getAnnotations()) {
-                    if (annotation.getSubtype().equals(FDFAnnotationHighlight.SUBTYPE)) {
-                        annotationsList.add(createHighlightAnnotations(pageIndex, page, annotation));
+                    if (annotation.getSubtype().equals(FDFAnnotationHighlight.SUBTYPE) ||


You should change the order of equals, the constant first and then the annotation.getsubType as it theoretically could ne null and would result in an NPE.

yes. bcf7516

Siedlerchr · 2017-03-17T17:14:55Z

                case FDFAnnotationHighlight.SUBTYPE:
                    label.setIcon(IconTheme.JabRefIcon.MARKER.getSmallIcon());
                    break;
+                case FDFAnnotationUnderline.SUBTYPE:


Is/Can FileAnnotation be made an enum?

Ah I just checked it's strings..

Siedlerchr · 2017-03-17T17:20:00Z


        try {
-            annotation.setContents(extractHighlightedText(page, annotation));
+            if (annotation.getSubtype().equals(FDFAnnotationHighlight.SUBTYPE) || annotation.getSubtype().equals(FDFAnnotationUnderline.SUBTYPE)) {


same as above

yes. bcf7516

Siedlerchr

See comments

Siedlerchr · 2017-03-17T20:36:35Z

+        // End old test
+
+        FileAnnotation expected = new FileAnnotation("Linus Dietz", LocalDateTime.of(2017, 3, 12, 20, 25), 1, "inline note annotation", "FreeText", Optional.empty());
+        assertEquals("Comparison fails. WHY?", Collections.singletonList(expected), annotations);


Not directly to see, but I did some debuggin and it fails because the FileAnnotation does not provide an equals method

I have added an equals method now

Ah. That makes sense. Thanks a lot!

LinusDietz · 2017-03-18T07:02:32Z

-        assertEquals(1, note.getPage());
+        // End old test style
+
+        assertEquals(expected,


Thanks to @Siedlerchr for pointing out the necessity to override the .equals() method in FileAnnotation, however the reason for this was to actually improve the message which is displayed by jUnit if the test fails.
@tobiasdiez, can you give a hint why the message is better with the Collections.singletonList() variant? (see #2640 (comment))

To see the value of comparison with lists, we need to assume that the method under test returns something unexpected. For example:

It returns two items instead of one. With the "old style" you just get expected 1 but got 2 while comparison with the singleton list displays something like Expected "item [property 1, ...]" but got "item [property 1, ....], item2 [value 1, ... ]". So you get more details about the item that is wrongly returned, which is often helpful in fixing bugs.
(for this you need to overwrite the toStrings method..I normally take the autogenerated variant generated by intellij)

The method still returns only one item but with the wrong values. In this case, the old style gives you expected "value1" but got "wrong value". If you compare against an expected item you directly get also information about the other properties (thus you directly see if just one value is off or a completely different item is returned).

Another important advantage is that, if in the future the class is extended by an additional property, you just need to adapt the equals method and all your tests automatically take this new property in account.

Well, if I change any of the tests in the PdfAnnotationImporterTest such that one attribute of the single list element mismatches I don't get the rich comparison you are describing. Nevertheless I have implemented it as you have proposed.

tobiasdiez

Code looks good to me, so I merge. Thanks for the refactoring / code improvement!

* upstream/master: Localization: General: French: Translation of new entries Localization: Menu: French: Translation of an entry (#2685) Fix #2680 and fix #2667: Swing errors are catched properly and without freezing (#2681) Do not log AND throw Replace misleading error message for fetcher connection error Document CrossRef test Fix subtitle detection for CrossRef fetcher Revert "Invoke LogMessages.add in JavaFX thread" Use global user agent Update mockito from 2.7.17 to 2.7.18 Move GuiAppender to GUI package Invoke LogMessages.add in JavaFX thread [WIP] Put the PDFAnnotationImporter under Test, enhance FileAnnotationTab (#2640) Fix for "Paying off technical debt: almost all utility classes have a private constructor now." (#2672) Revert "Paying off technical debt: almost all utility classes have a private constructor now. (#2649)" (#2670) Paying off technical debt: almost all utility classes have a private constructor now. (#2649) Changed codeformatting for better fxml annotation (#2668) Disalbe Google Scholar tests on all CI environments (#2654) Fix JSONException in Crossref fetcher as mentioned in #2442

LinusDietz added 5 commits March 12, 2017 20:07

updated outdated javadoc

c55b4d8

Refactored the PDF import using absolute paths prior import to make i…

8a4ef50

…t testable

Wrote some unit tests for the extraction of annotations from PDFs

7303738

Added support for annotations with timezone in the date

54d19fe

Fix correct highlighting/shading of the TextAreas in the FileAnnotat…

c648dae

…ionTab

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

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

LinusDietz added 3 commits March 13, 2017 16:55

* Added support for underlined text

605dfbf

* Fixed localization Issues

Add test for polygons, support for different date formats in the File…

0596504

…Annotation and EntryAnnotationImporterTest

Fixed reloading of annotations via the "Reload Annotations" button

347b577

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

tobiasdiez requested changes Mar 16, 2017

View reviewed changes

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

LinusDietz added 2 commits March 17, 2017 17:44

Use Collections.emptyList() to compare empty lists

c4aed22

FileAnnotationCache logger is now on debug level instead of info

2894c9e

Siedlerchr reviewed Mar 17, 2017

View reviewed changes

Siedlerchr requested changes Mar 17, 2017

View reviewed changes

LinusDietz added 3 commits March 17, 2017 20:20

Changed a test using Collections.singletonList(). It fails

8ed6abf

Use file.toPath() instead of Paths.get(file.toString())

1dfa504

Flipped some .equals()

bcf7516

Siedlerchr approved these changes Mar 17, 2017

View reviewed changes

Siedlerchr reviewed Mar 17, 2017

View reviewed changes

Siedlerchr and others added 2 commits March 17, 2017 21:50

Add equals method for test

bf36365

Deciding on the right test paradigm

65b5617

LinusDietz commented Mar 18, 2017

View reviewed changes

Introduced the FileAnnotationViewModel, Changed AnnotationType to Enum

2a2081d

Fixed localization, refactored PdfAnnotationImporterTest

4b9d796

tobiasdiez approved these changes Mar 20, 2017

View reviewed changes

tobiasdiez merged commit 8751cef into master Mar 20, 2017

tobiasdiez deleted the test_pdfimport branch March 20, 2017 14:52

LinusDietz mentioned this pull request Mar 20, 2017

Reload annotations does not seem to work #2673

Closed

Uh oh!

Conversation

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

Uh oh!

Uh oh!

tobiasdiez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Siedlerchr 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

LinusDietz commented Mar 12, 2017 •

edited

Loading

tobiasdiez left a comment •

edited

Loading

LinusDietz Mar 17, 2017 •

edited

Loading

LinusDietz Mar 17, 2017 •

edited

Loading

tobiasdiez Mar 18, 2017 •

edited

Loading