Skip to content
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

Fix warning display issue for entering a title made of two words #6054

Merged
merged 10 commits into from
Mar 17, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where the group and the link column were not updated after changing the entry in the main table. [#5985](https://github.com/JabRef/jabref/issues/5985)
- We fixed an issue where reordering the groups was not possible after inserting an article. [#6008](https://github.com/JabRef/jabref/issues/6008)
- We fixed an issue where citation styles except the default "Preview" could not be used. [#56220](https://github.com/JabRef/jabref/issues/5622)
- We fixed an issue where a warning was displayed when the title content is made up of two sentences. [#5832](https://github.com/JabRef/jabref/issues/5832)
- We fixed an issue where an exception was thrown when adding a save action without a selected formatter in the library properties [#6069](https://github.com/JabRef/jabref/issues/6069)

### Removed
Expand Down
28 changes: 17 additions & 11 deletions src/main/java/org/jabref/logic/integrity/TitleChecker.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
public class TitleChecker implements ValueChecker {

private static final Pattern INSIDE_CURLY_BRAKETS = Pattern.compile("\\{[^}\\{]*\\}");
private static final Pattern DELIMITERS = Pattern.compile("\\.|\\!|\\?|\\;|\\:");
private static final Predicate<String> HAS_CAPITAL_LETTERS = Pattern.compile("[\\p{Lu}\\p{Lt}]").asPredicate();

private final BibDatabaseContext databaseContext;
Expand All @@ -22,10 +23,13 @@ public TitleChecker(BibDatabaseContext databaseContext) {

/**
* Algorithm:
* - remove trailing whitespaces
* - ignore first letter as this can always be written in caps
* - remove everything that is in brackets
* - check if at least one capital letter is in the title
* - split the title into sub titles based on the delimiters
* (defined in the local variable DELIMITERS, currently . ! ? ; :)
* - for each sub title:
* - remove trailing whitespaces
* - ignore first letter as this can always be written in caps
* - check if at least one capital letter is in the sub title
*/
@Override
public Optional<String> checkValue(String value) {
Expand All @@ -37,9 +41,7 @@ public Optional<String> checkValue(String value) {
return Optional.empty();
}

String valueTrimmed = value.trim();
String valueIgnoringFirstLetter = valueTrimmed.startsWith("{") ? valueTrimmed : valueTrimmed.substring(1);
String valueOnlySpacesWithinCurlyBraces = valueIgnoringFirstLetter;
String valueOnlySpacesWithinCurlyBraces = value;
while (true) {
Matcher matcher = INSIDE_CURLY_BRAKETS.matcher(valueOnlySpacesWithinCurlyBraces);
if (!matcher.find()) {
Expand All @@ -48,11 +50,15 @@ public Optional<String> checkValue(String value) {
valueOnlySpacesWithinCurlyBraces = matcher.replaceAll("");
}

boolean hasCapitalLettersThatBibtexWillConvertToSmallerOnes = HAS_CAPITAL_LETTERS
.test(valueOnlySpacesWithinCurlyBraces);

if (hasCapitalLettersThatBibtexWillConvertToSmallerOnes) {
return Optional.of(Localization.lang("capital letters are not masked using curly brackets {}"));
String[] splitTitle = DELIMITERS.split(valueOnlySpacesWithinCurlyBraces);
for (String subTitle : splitTitle) {
subTitle = subTitle.trim();
if (!subTitle.isEmpty()) {
subTitle = subTitle.substring(1);
if (HAS_CAPITAL_LETTERS.test(subTitle)) {
return Optional.of(Localization.lang("capital letters are not masked using curly brackets {}"));
}
}
}

return Optional.empty();
Expand Down
22 changes: 22 additions & 0 deletions src/test/java/org/jabref/logic/integrity/IntegrityCheckTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,17 @@ void testTitleChecks() {
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a {Title}"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "{C}urrent {C}hronicle"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "{A Model-Driven Approach for Monitoring {ebBP} BusinessTransactions}"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is a sub title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is a sub title 2"), BibDatabaseMode.BIBTEX));
assertWrong(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is A sub title 2"), BibDatabaseMode.BIBTEX));
assertWrong(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is A sub title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is {A} sub title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is {A} sub title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1...This is a sub title 2"), BibDatabaseMode.BIBTEX));
assertWrong(withMode(createContext(StandardField.TITLE, "This is a sub title 1... this is a sub Title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is; A sub title 1.... This is a sub title 2"), BibDatabaseMode.BIBTEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This!is!!A!Title??"), BibDatabaseMode.BIBTEX));
assertWrong(withMode(createContext(StandardField.TITLE, "This!is!!A!TitlE??"), BibDatabaseMode.BIBTEX));

assertCorrect(withMode(createContext(StandardField.TITLE, "This is a title"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a Title"), BibDatabaseMode.BIBLATEX));
Expand All @@ -194,6 +205,17 @@ void testTitleChecks() {
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a {Title}"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "{C}urrent {C}hronicle"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "{A Model-Driven Approach for Monitoring {ebBP} BusinessTransactions}"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is a sub title 2"), BibDatabaseMode.BIBLATEX));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good to have tests here. These are more a end-to-end-test. Would you mind to add separate tests to TitleCheckerTest.java. In IntelliJ, you can create this file using Ctrl+Shift+T (and pressing Enter in the dialog). It would really help to have the tests for the TitleChecker in a dedicated class, which helps to focus on the implementation. -- This should be very easy to do, but will help future work on the title checker (maybe in one, two years) to ensure that the functionality is still correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanation! As suggested, I have added separate tests for titles having delimiters in the new file. Those method names may not be perfect. If there is a better way to rename it, please let me know.

assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is a sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is A sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is A sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: This is {A} sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1: this is {A} sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1...This is a sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is a sub title 1... this is a sub Title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This is; A sub title 1.... This is a sub title 2"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This!is!!A!Title??"), BibDatabaseMode.BIBLATEX));
assertCorrect(withMode(createContext(StandardField.TITLE, "This!is!!A!TitlE??"), BibDatabaseMode.BIBLATEX));
}

@Test
Expand Down
79 changes: 79 additions & 0 deletions src/test/java/org/jabref/logic/integrity/TitleCheckerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package org.jabref.logic.integrity;

import java.util.Optional;

import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.database.BibDatabaseMode;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;

public class TitleCheckerTest {

private TitleChecker checker;

@BeforeEach
public void setUp() {
BibDatabaseContext databaseContext = new BibDatabaseContext();
databaseContext.setMode(BibDatabaseMode.BIBTEX);
checker = new TitleChecker(databaseContext);
}

@Test
public void FirstLetterAsOnlyCapitalLetterInSubTitle2() {
assertEquals(Optional.empty(), checker.checkValue("This is a sub title 1: This is a sub title 2"));
}

@Test
public void NoCapitalLetterInSubTitle2() {
assertEquals(Optional.empty(), checker.checkValue("This is a sub title 1: this is a sub title 2"));
}

@Test
public void TwoCapitalLettersInSubTitle2() {
assertNotEquals(Optional.empty(), checker.checkValue("This is a sub title 1: This is A sub title 2"));
}

@Test
public void MiddleLetterAsOnlyCapitalLetterInSubTitle2() {
assertNotEquals(Optional.empty(), checker.checkValue("This is a sub title 1: this is A sub title 2"));
}

@Test
public void TwoCapitalLettersInSubTitle2WithCurlyBrackets() {
assertEquals(Optional.empty(), checker.checkValue("This is a sub title 1: This is {A} sub title 2"));
}

@Test
public void MiddleLetterAsOnlyCapitalLetterInSubTitle2WithCurlyBrackets() {
assertEquals(Optional.empty(), checker.checkValue("This is a sub title 1: this is {A} sub title 2"));
}

@Test
public void FirstLetterAsOnlyCapitalLetterInSubTitle2AfterContinuousDelimiters() {
assertEquals(Optional.empty(), checker.checkValue("This is a sub title 1...This is a sub title 2"));
}

@Test
public void MiddleLetterAsOnlyCapitalLetterInSubTitle2AfterContinuousDelimiters() {
assertNotEquals(Optional.empty(), checker.checkValue("This is a sub title 1... this is a sub Title 2"));
}

@Test
public void FirstLetterAsOnlyCapitalLetterInEverySubTitleWithContinuousDelimiters() {
assertEquals(Optional.empty(), checker.checkValue("This is; A sub title 1.... This is a sub title 2"));
}

@Test
public void FirstLetterAsOnlyCapitalLetterInEverySubTitleWithRandomDelimiters() {
assertEquals(Optional.empty(), checker.checkValue("This!is!!A!Title??"));
}

@Test
public void MoreThanOneCapitalLetterInSubTitleWithoutCurlyBrackets() {
assertNotEquals(Optional.empty(), checker.checkValue("This!is!!A!TitlE??"));
}
}