Solved #3823 File annotation by upupming · Pull Request #4246 · JabRef/jabref

upupming · 2018-08-02T12:47:20Z

This pull request solved #3823, and improved the new line remover.

Removed hyphens and newlines of highlighted text

I removed all new lines which don't have preceded . or : becaues I think . or : is often used to start a new paragraph.

Test text:

Test result:

There is another problem with the regular expression (\r?\n|\r) when I try to use (?<![.|:])(\r?\n|\r) for excluding the new line end with . or :.

If it's on Windows, the \r\n will be replaced with a whitespace, and it is OK. However if we encounter .\r\n, the .\r will be reserved and the \n will be replaced by a whitespace. In this way, the .\r will not be shown as new line int the field, and we don't need this whitespace. So instead we should change .\r\n to \r\n directly.

So I use java's %n to distinguish new line wisely.

Font size of summary on the left changed to 10px

Summary text wrapped

The Final example:

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

…ap summary

tobiasdiez

Thanks a lot for your contribution. It is a real joy to review a PR that is so well documented and explained! The code is also fine and I've only a few minor comments. It would be nice if you could add a unit test, too.

tobiasdiez · 2018-08-02T13:17:00Z

 public class RemoveHyphenatedNewlinesFormatter extends Formatter {
-    private static final Pattern HYPHENATED_WORDS = Pattern.compile("(-\r\n|-\n|-\r)");
+    private static final String newLine = String.format("%n");
+    private static final Pattern HYPHENATED_WORDS = Pattern.compile("(-" + newLine + ")");


You can also use \R to match any line breaks, see https://docs.oracle.com/javase/9/docs/api/java/util/regex/Pattern.html.

Thanks, it is what I didn't know before. But it seems \R will match any of \r, \n and \r\n.

In this example, you can see \r is matched on Windows. But we need only match \r\n on Windows, so I think %n is the right choice.

tobiasdiez · 2018-08-02T13:18:15Z

 - Window state is saved on close and restored on start.
 - Files without a defined external file type are now directly opened with the default aplication of the operating system
 - We streamlined the process to rename and move files by removing the confirmation dialogs.
+- We removed the redundant new lines of marking and wrapped the summary in File annotation tab.


Slightly better: We removed the redundant new lines of markings and wrapped the summary in the File annotation tab.

Please also add a reference to the issue #3823 (cf. e.g. some of the changelog entries under "fixed").

tobiasdiez · 2018-08-02T13:20:15Z

        Label page = new Label(Localization.lang("Page") + ": " + annotation.getPage());

-        marking.setStyle("-fx-font-weight: bold");
+        marking.setStyle("-fx-font-size: 10px; -fx-font-weight: bold");


Please specify the font-size in em so that it scales properly if the base font size is changed (e.g. on high res displays).

Changed to 0.75em

tobiasdiez · 2018-08-02T13:23:22Z

-            this.marking.set(annotationContent.isEmpty() ? illegibleTextMessage : annotationContent);
+            String markingContent = (annotationContent.isEmpty() ? illegibleTextMessage : annotationContent);
+            // remove newlines && hyphens before linebreaks
+            markingContent = new RemoveHyphenatedNewlinesFormatter().format(markingContent);


It is good that you try to reuse existing logic, i.e. the existing formatters. However, these are also used to generate the bibtex key (and file name). For these use cases, it is still desired to remove all line breaks. Thus, I would prefer if you define the modified regex pattern to remove the linebreaks here in this class and leave the RemoveNewlinesFormatter untouched (except changing the pattern to use \R).

Now I leave the two formatter untouched and only use the pattern in this scope.

tobiasdiez · 2018-08-02T13:28:03Z

+            // remove newlines && hyphens before linebreaks
+            markingContent = new RemoveHyphenatedNewlinesFormatter().format(markingContent);
+            markingContent = new RemoveNewlinesFormatter().format(markingContent);
+            this.marking.set(markingContent);


It would be nice if you could add some tests verifying the new behavior: create a FileAnnotion object by hand (not by parsing a pdf) which contains such problematic hyphens/newlines and check that they are removed, indeed.

Thank you for your advice, I have added a unit test!

upupming · 2018-08-02T13:45:21Z

@tobiasdiez Thanks for your review! I will fix them soon.

This reverts commit ae2956b.

upupming · 2018-08-03T01:32:46Z

Hi, I reverted the change for RemoveHyphenatedNewlinesFormatter and RemoveNewlinesFormatter. I have also fixed the tooltip for marking summary.

Before

After

Now I'm going to add some unit tests!

upupming · 2018-08-03T03:05:04Z

All done~ 😃

Siedlerchr

Thank you very much for your contribution! It's really nice to see such a wonderful documented PR. Code wise lgtm!

tobiasdiez · 2018-08-03T09:04:43Z

-        assertEquals("water", formatter.format("wa-\nter"));
-        assertEquals("water", formatter.format("wa-\r\nter"));
-        assertEquals("water", formatter.format("wa-\rter"));
+        String newLine = String.format("%n");


Please keep these tests (here and below). The text that we want to format might contain linux line endings on Windows and vice versa. But you can of course also add a tests for ̀%n.
Please change also all regex patterns to use \R instead of %n for newlines.

Finally leaving original formatter and tests untouched.

In summary, Pattern.compile("...") changed from (\r?\n|\r) to \\R, but the effects are same.

tobiasdiez · 2018-08-03T10:07:25Z

Thanks for the quick follow-up! Looking forward to your next PR ;)

…tive * upstream/master: Update dependencies (#4251) Fix author list parser (#4169) (#4248) Solved #3823 File annotation (#4246) Fix importer vulnerability (#4240)

upupming added 3 commits August 2, 2018 20:39

Solved JabRef#3823 File annotation: delete hyphens and new lines & wr…

ce78e7f

…ap summary

Fixed: l10n for remove line breaks

ae2956b

Fixed: final variable name

9c39633

tobiasdiez requested changes Aug 2, 2018

View reviewed changes

upupming added 2 commits August 3, 2018 08:48

Revert "Fixed: l10n for remove line breaks"

8015eba

This reverts commit ae2956b.

Fixed: revert changed formatter & wrap marking tooltip

6370f72

Added: test for FileAnnotation

6389c30

Siedlerchr approved these changes Aug 3, 2018

View reviewed changes

tobiasdiez reviewed Aug 3, 2018

View reviewed changes

Fixed: keep newline&hyphen formatter using \R

d8c369f

tobiasdiez merged commit 611ac55 into JabRef:master Aug 3, 2018

Uh oh!

Conversation

upupming commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Removed hyphens and newlines of highlighted text

Font size of summary on the left changed to 10px

Summary text wrapped

The Final example:

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!

Choose a reason for hiding this comment

Uh oh!

upupming commented Aug 2, 2018

Uh oh!

upupming commented Aug 3, 2018

Uh oh!

upupming commented Aug 3, 2018

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Aug 3, 2018 • 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!

upupming Aug 3, 2018 • 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!

tobiasdiez commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

upupming commented Aug 2, 2018 •

edited

Loading

tobiasdiez Aug 3, 2018 •

edited

Loading

upupming Aug 3, 2018 •

edited

Loading