-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solved #3823 File annotation #4246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -10,7 +10,8 @@ | |||
* Removes all hyphenated line breaks in the string. | |||
*/ | |||
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 + ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also use \R
to match any line breaks, see https://docs.oracle.com/javase/9/docs/api/java/util/regex/Pattern.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHANGELOG.md
Outdated
@@ -27,6 +27,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
- 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -94,12 +94,14 @@ private Node createFileAnnotationNode(FileAnnotationViewModel annotation) { | |||
Label date = new Label(annotation.getDate()); | |||
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to 0.75em
this.marking.set(annotationContent.isEmpty() ? illegibleTextMessage : annotationContent); | ||
String markingContent = (annotationContent.isEmpty() ? illegibleTextMessage : annotationContent); | ||
// remove newlines && hyphens before linebreaks | ||
markingContent = new RemoveHyphenatedNewlinesFormatter().format(markingContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I leave the two formatter untouched and only use the pattern in this scope.
// remove newlines && hyphens before linebreaks | ||
markingContent = new RemoveHyphenatedNewlinesFormatter().format(markingContent); | ||
markingContent = new RemoveNewlinesFormatter().format(markingContent); | ||
this.marking.set(markingContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your advice, I have added a unit test!
@tobiasdiez Thanks for your review! I will fix them soon. |
All done~ 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your contribution! It's really nice to see such a wonderful documented PR. Code wise lgtm!
assertEquals("water", formatter.format("wa-\nter")); | ||
assertEquals("water", formatter.format("wa-\r\nter")); | ||
assertEquals("water", formatter.format("wa-\rter")); | ||
String newLine = String.format("%n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally leaving original formatter and tests untouched.
In summary, Pattern.compile("...") changed from (\r?\n|\r)
to \\R
, but the effects are same.
Thanks for the quick follow-up! Looking forward to your next PR ;) |
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: