Fixes generated bibtex key and display of institute authors

[k3KAW8Pnf7mkmdSMPHz27]

Fixes #6459. Fixes #6465 .

There are two parts of this issue,

1. A null is prepended to an abbreviated BibTeX key.
2. Curly brackets are not respected (in the maintable's author column?).

1. Fix to the prepended null
What is going on?
BracketedPattern.generateInstitutionKey gets called for any author enclosed in curly brackets (e.g., "{The School of Life}"). The method expects an institute of technology or university and appends its name to the key (e.g., null if there is no name).

Why is it going on?
Academic institutions can have long generated BibTeX keys unless abbreviated, e.g., "Royal Institute of Technology: The School of Electrical Engineering and Computer Science", which generateInstitutionKey shortens to RITEECS.

Fix
Replace a null valued university with an empty string. The drawback is potentially very short BibTeX keys (e.g., The School of Life -> L).

What are alternatives?

1. Regex matching universities and technological institutions, which is hard to implement correctly. On the other hand, the drawback is an unexpected BibTeX key in a corner case. In the case of The School of Life would be abbreviated to SL instead of L.

2. Fix to the author column
When the list of authors gets converted to a latex-free version, all curly brackets are removed since the whole string is parsed as latex. When the latex-free string is used to create/fetch an AuthorList it will no longer contain any brackets, and the information needed to format the string is gone.

3. What I think is left to do

• Find out why {The School of Life} isn't respected in the author field of the GUI
• Attempt to match universities etc. with regex Assume that names that have comma separated parts are universities
• Update BibtexKeyGeneratorTest as it makes heavy use of deprecated methods the deprecated methods are essentially convenience methods so they have been moved inside the test file
• See if the readability of generateInstitutionKey can be improved
• Change the key generator for institution/corporate names to a Formatter? generateInstitutionKey should not be a separate Formatter, it is only called by normalize.

and

• Change in CHANGELOG.md described (if applicable)
• Tests created for changes (if applicable)
• Manually tested changed features in running JabRef (always required)
• Screenshots added in PR description (for UI changes)
• Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

[Siedlerchr]

Find out why {The School of Life} isn't respected in the author field of the GUI

I think it could be that the latex2unicode convert is called for the author field. It kills the extra braces. I had a similar problem in the MSOffice Exporter. I think I implemented a workaround there.

[k3KAW8Pnf7mkmdSMPHz27]

Ah, nice, thank you! I will take a look!

[k3KAW8Pnf7mkmdSMPHz27]

I believe this is fixable in BibEntry.getLatexFreeField by splitting it into BibTeX/LaTeX according to whichever BibTeX version are supported, but the problems are,

1. I can't find which BibTeX version we are supporting in ADR nor anywhere else
2. BibEntry.getLatexFreeField appears to be used both for BibTeX in UTF-8 (e.g., "{LâTëX}") and for fully "expanded" text (e.g., "LâTëX"). As those are incompatible perhaps it makes sense to split the method and track down what method wants what?
3. Should I do this?
4. I can attempt to do the same workaround but it doesn't really solve the problem (as I understand it)?

I can also look at it a bit more and see if I have missed something, anyway, any suggestion/hint is greatly appreciated @Siedlerchr

[Siedlerchr]

Hi,
thanks for investigating. The underlying problem is the latex2unicode (external library) kills the braces, it has of course no understanding of an author. Refs #4152 and #6155
I fear there is no easy solution. We use the Latexfree field method to not have latex code displayed in the main table #6329
I would treat this as a secondary issue.

Regarding the second issue:

Replace a null valued university with an empty string. The drawback is potentially very short BibTeX keys (e.g., The School of Life -> L).

What about using a formatter that uses the Capital letters? e.g. JabRef would then become JR
and orgs with abbreviations e.g. UNO or WHO would stay as is. Similar to the title formatter.

[k3KAW8Pnf7mkmdSMPHz27]

Regarding the first issue, first of all, as you know, I am not a regular contributor to JabRef. If you'd like me to drop this topic for any reason, please say so. I am just hoping that sorting this out will save someone else time in the future, but I am not familiar enough with JabRef to know if I have missed something important.

If I understand things correctly, there are currently three different cases relevant to #4337 and #4152.

Assuming we are using the BibTeX format described at BibTeX.org (special symbols)

1. Plain BibTeX, e.g., Kurt {G\"OD}el: Used as the internal representation?
2. BibTeX "with unicode", e.g., Kurt {GÖD}el: Used for exporting to a unicode-aware file format and editing in unicode?
3. Formatted text, e.g., K. GÖDel: Used to display to a user or export to a LaTeX/BibTeX unaware environment.

BibEntry.getLatexFreeField handles case 3 while, to the best of my knowledge, no function/method handles case 2.

The issue with case 2 is that LaTeX and BibTeX are two incompatible "languages" (since some reserved words are the same). Therefore, if we are using the format described at BibTeX.org (format), the "correct" way of dealing with case 2 using BibEntry.getLatexFreeField is to either split out the LaTeX components and "translate" them to unicode separately or to escape the parts that are in BibTeX format. If I am using the BibTeX.org format, at least all non-nested curly bracketsl, not preceded by a keyword, should be escaped

• {JabRef} -> \{JabRef\}
• Proceedings of the {IEEE} -> Proceedings of the \{IEEE\}

which is then dealt with correctly in BibEntry.getLatexFreeField.

What I was trying to argue is that case 2 and case 3 must be dealt with differently. I can deal with this after issue #6459 if that is of interest?

However, I just realized that issue #6459 is case 3, and can most likely be solved by changing the order of method calls.

Please point out if something is unclear or incomprehensible. I am trying to improve my writing skills, but I am well aware that I have some practice ahead of me. My only excuse is that it is still morning here X)

[k3KAW8Pnf7mkmdSMPHz27]

Regarding the second issue. That does sound better. I can't think of any case where keeping all capitalized letters isn't good and it should be compatible with the current implementation.

[Siedlerchr]

Regarding the first issue, first of all, as you know, I am not a regular contributor to JabRef. If you'd like me to drop this topic for any reason, please say so. I am just hoping that sorting this out will save someone else time in the future, but I am not familiar enough with JabRef to know if I have missed something important.

No problem :) We are happy for every contributor. It's just a complex issue. It's just complicated issue because it involves Corporate authors and Unicode.
I try to provide some background on this:

Unicode:
Originally, bibtex and Latex did not support unicode. That's why you have to use those Latex-Escaping of umlauts and other characters.
Biblatex, the successor of bibtex, supports Unicode, but many journals still require bibtex.
And many citations from online resources still contain Latex-escaping of characters.
Originally JabRef maintained a bidirectional mapping between latex escaped characters and their Unicode equivalent. Some time ago we switched to the latex2unicode library.
Of course JavaFX has no idea of latex, therefore having a title or an author encoded in latex must obviously converted to Unicode for display.
e.g. => Kurt {G\"OD}el becomes Kurt Gödel

Corporate Authors:
See the Biblatex manual Section 2.3.3 Corporate Authors and Editors:

Corporate authors and editors are given in the author or editor field, respectively.Note that they must be wrapped in an extra pair of curly braces to prevent data parsing from treating them as personal names which are to be dissected into their components.
Example:
author= {{National Aeronautics and Space Administration}}

In JabRef the latex2unicode formatter is called for every field and the formatter now receives the string toConvert = {National Aeronautics and Space Administration}
The latex2unicode formatter now kills the curly braces as they could indicate some latex commands and returns string converted = National Aeronautics and Space Administration
JabRef now splits the author according to it rules and thinks it's in this case two authors separated by authors.
The only solution I see is to check if it's a corporate authors, convert it to Unicode and add the braces again. One really difficult edge case is for example author = {\L{}}ukasz Micha\l{}
Could be easily interpreted as corporate authors....

I hope my long explanation helps you a bit to understand the problem. Maybe you come up with an idea.

[rolandog]

Hello @k3KAW8Pnf7mkmdSMPHz27 , thank you for your work in fixing this issue! And thanks for that informative context @Siedlerchr ; that's a very interesting edge case... I think there could be a test where braces are counted to corroborate that the brace at the very beginning is not closed earlier on (that would be the case where formatting as a Corporate Author would apply, I think).

I have some code in Python that does something related... (though the opposite: it matches the most ancient parenthesis first...); I'll adapt it when I return home.

def par_count2(text: Letters, opener: str = "(", closer: str = ")") -> Numbers:
    """Base algorithm to count matching parentheses.

    Parameters
    ----------
    `text` : `str`
        The string to be parsed against matching opener and closer
    `opener` : `str`
        The character(s) to be considered as the 'opener' of a sequence
    `closer` : `str`
        The character(s) to be considered as the 'end' of a sequence

    Yields
    ------
    `count` : `int`
        A sequence of `int` that are the number of matches for each character

    Examples
    --------
    Here are some base examples of expected output, and actual output.

        ``(((((((((``

        ``123456789``

    >>> [c for c in par_count2("(((((((((")]
    [1, 2, 3, 4, 5, 6, 7, 8, 9]

        ``)))))))))``

        ``123456789``

    >>> [c for c in par_count2(")))))))))")]
    [1, 2, 3, 4, 5, 6, 7, 8, 9]

        ``()()()()()()()()()``

        ``112233445566778899``

    >>> [c for c in par_count2("()()()()()()()()()")]
    [1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8, 9, 9]

        ``((((((((()))))))))``

        ``123456789123456789``

    >>> [c for c in par_count2("((((((((()))))))))")]
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5, 6, 7, 8, 9]

        ``))()())(())``

        ``12334456767``

    >>> [c for c in par_count2("))()())(())")]
    [1, 2, 3, 3, 4, 4, 5, 6, 7, 6, 7]

    ``(A)()(A)()(B)()(A)()(A)``

    ``11122323445156673788949``

    >>> [c for c in par_count2("(A)()(A)()(B)()(A)()(A)")]
    [1, 1, 1, 2, 2, 3, 2, 3, 4, 4, 5, 1, 5, 6, 6, 7, 3, 7, 8, 8, 9, 4, 9]
    """

    # type declarations for local variables
    count: int
    character: str
    pending_from: int
    pending_to: int
    others: Counter[str]

    count = 0
    pending_from = 0
    pending_to = 0
    others = Counter()

    # logging recommends using %s substitutions, instead of f-strings or
    # string interpolation with brackets
    logger = logging.getLogger(__name__)
    logger.debug("About to parse %s with %s and %s", text, opener, closer)

    for character in text:
        if character == closer:
            if pending_from != pending_to:
                pending_from += 1
                yield pending_from
            else:
                count += 1
                pending_from = count
                pending_to = count
                yield count
        elif character == opener:
            count += 1
            pending_to = count
            yield count
        else:
            others.update(character)
            yield others.get(character, 0)

    logger.info("Parsed %s with %s and %s", text, opener, closer)

[Siedlerchr]

@rolandog Your hint with the braces gave me the idea and I think we maybe have already a solution for this problem. I can't believe it's been hidden in plain sight 🤦

This method is used in the BracketedPattern class to define if it's an insituation (corporate author or not)

jabref/src/main/java/org/jabref/model/strings/StringUtil.java

Lines 418 to 446 in 4e220f6

	/**
	* Checks if the given String has exactly one pair of surrounding curly braces <br>
	* Strings with escaped characters in curly braces at the beginning and end are respected, too
	* @param toCheck The string to check
	* @return True, if the check was succesful. False otherwise.
	*/
	public static boolean isInCurlyBrackets(String toCheck) {
	int count = 0;
	int brackets = 0;
	if ((toCheck == null) || toCheck.isEmpty()) {
	return false;
	} else {
	if ((toCheck.charAt(0) == '{') && (toCheck.charAt(toCheck.length() - 1) == '}')) {
	for (char c : toCheck.toCharArray()) {
	if (c == '{') {
	if (brackets == 0) {
	count++;
	}
	brackets++;
	} else if (c == '}') {
	brackets--;
	}
	}
	return count == 1;
	}
	return false;
	}
	}

And the test:

jabref/src/test/java/org/jabref/model/strings/StringUtilTest.java

Lines 181 to 193 in 862078a

	@Test
	void testIsInCurlyBrackets() {
	assertFalse(StringUtil.isInCurlyBrackets(""));
	assertFalse(StringUtil.isInCurlyBrackets(null));
	assertTrue(StringUtil.isInCurlyBrackets("{}"));
	assertTrue(StringUtil.isInCurlyBrackets("{a}"));
	assertTrue(StringUtil.isInCurlyBrackets("{a{a}}"));
	assertTrue(StringUtil.isInCurlyBrackets("{{\\AA}sa {\\AA}Stor{\\aa}}"));
	assertFalse(StringUtil.isInCurlyBrackets("{"));
	assertFalse(StringUtil.isInCurlyBrackets("}"));
	assertFalse(StringUtil.isInCurlyBrackets("a{}a"));
	assertFalse(StringUtil.isInCurlyBrackets("{\\AA}sa {\\AA}Stor{\\aa}"));
	}

[k3KAW8Pnf7mkmdSMPHz27]

Hi @rolandog ! Thank you for the very detailed issue!! It makes things a lot easier :P

I have done a "bad" code update to demonstrate what I think is the issue with displaying names in the maintable (i.e., the order of method calls). @Siedlerchr and @rolandog you are both (of course) most welcome to add any comment/change any code and if there is any way I can make life easier for you do tell. I am currently quite new to Github and my todo-list, notes and links are currently kept in a offline jupyter document and I have no idea how to do it differently X)

@Siedlerchr thank you for the overview and the biblatex manual link! I have been looking for that one o.O
I am going to stay out of the biblatex discussion for now, because I believe Kurt {G\"OD}el should be Kurt GÖDel. It doesn't mean I am not interested in the issue, just that I have a lot of reading ahead of me, apparently a lot have changed since 1994 and BibTeX 1.0 ;)

[rolandog]

@k3KAW8Pnf7mkmdSMPHz27, you're welcome, I'm glad to have helped pinpoint this, and thank you for taking on this issue!

And, that's great @Siedlerchr, I'm happy that it seems that this may not need extreme refactoring; that's a very clever function!

However, I think I found an edge case where isInCurlyBrackets may present a false positive in its current form (inspired by the Corporate Author 'Kurt Gödel Society', but instead using 'Łukasz Michał' as part of a Corporate Author name).

I'm not familiar with JabRef's whole codebase, so mismatched braces may be caught earlier on, as per:

jabref/src/main/java/org/jabref/logic/bibtex/FieldWriter.java

Lines 41 to 74 in 4e220f6

	private static void checkBraces(String text) throws InvalidFieldValueException {
	int left = 0;
	int right = 0;
	// First we collect all occurrences:
	for (int i = 0; i < text.length(); i++) {
	char item = text.charAt(i);
	boolean charBeforeIsEscape = false;
	if ((i > 0) && (text.charAt(i - 1) == '\\')) {
	charBeforeIsEscape = true;
	}
	if (!charBeforeIsEscape && (item == '{')) {
	left++;
	} else if (!charBeforeIsEscape && (item == '}')) {
	right++;
	}
	}
	// Then we throw an exception if the error criteria are met.
	if (!(right == 0) && (left == 0)) {
	LOGGER.error("Unescaped '}' character without opening bracket ends string prematurely. Field value: {}", text);
	throw new InvalidFieldValueException("Unescaped '}' character without opening bracket ends string prematurely. Field value: " + text);
	}
	if (!(right == 0) && (right < left)) {
	LOGGER.error("Unescaped '}' character without opening bracket ends string prematurely. Field value: {}", text);
	throw new InvalidFieldValueException("Unescaped '}' character without opening bracket ends string prematurely. Field value: " + text);
	}
	if (left != right) {
	LOGGER.error("Braces don't match. Field value: {}", text);
	throw new InvalidFieldValueException("Braces don't match. Field value: " + text);
	}
	}

Here is an example, displayed as a test, where one of the entries would throw an error (the last one):

@Test 
void testIsInCurlyBrackets() {
    /** correct
     * c        : {\L{}}ukasz Micha\l{}
     * brackets : 1  210             10
     * count    : 1  112             22
     */
    assertFalse(StringUtil.isInCurlyBrackets("{\L{}}ukasz Micha\l{}"));

    /** correct
     * c        : {{\L{}}ukasz Micha\l{} Society}
     * brackets : 12  321             21        0
     * count    : 11  111             11        1
     */
    assertTrue(StringUtil.isInCurlyBrackets("{{\L{}}ukasz Micha\l{} Society}"));

    /** mismatched braces, should return false?
     * c        : {{\L{}}ukasz Micha\l{} {Society}
     * brackets : 12  321             21 2       1
     * count    : 11  111             11 1       1
     */
    assertFalse(StringUtil.isInCurlyBrackets("{{\L{}}ukasz Micha\l{} {Society}"));
}

In case this isn't filtered by CheckBraces, then checking the final value of brackets == 0 could prevent that edge case:

/** 
 * Checks if the given String has exactly one pair of surrounding curly braces <br> 
 * Strings with escaped characters in curly braces at the beginning and end are respected, too 
 * @param toCheck The string to check 
 * @return True, if the check was succesful. False otherwise. 
 */ 
public static boolean isInCurlyBrackets(String toCheck) { 
    int count = 0; 
    int brackets = 0; 
    if ((toCheck == null) || toCheck.isEmpty()) { 
        return false; 
    } else { 
        if ((toCheck.charAt(0) == '{') && (toCheck.charAt(toCheck.length() - 1) == '}')) { 
            for (char c : toCheck.toCharArray()) { 
                if (c == '{') { 
                    if (brackets == 0) { 
                        count++; 
                    } 
                    brackets++; 
                } else if (c == '}') { 
                    brackets--; 
                } 
            } 
            return count == 1 && brackets == 0; 
        } 
        return false; 
    } 
}

[Siedlerchr]

@rolandog Thanks for testing and checking that edge case. Feel free to submit a new PR and in that way you can also fix the workaround I used in the MSbibAuthor for the MS Office exporter

jabref/src/main/java/org/jabref/logic/msbib/MSBibConverter.java

Line 119 in 4e220f6

private static List<MsBibAuthor> getAuthors(BibEntry entry, String authors, Field field) {

[k3KAW8Pnf7mkmdSMPHz27]

"Curly brackets are not respected (in the maintable's author column?)." is the same as issue #6465.
Should I leave it alone in this PR or update this one to close both?

[Siedlerchr]

Since they are related you can update your PR to close both. Just add it also to the changelog then.

[tobiasdiez]

Thanks! Looks very good now, so I'll merge. We hope you enjoyed contributing to JabRef and re looking forward to your next PR 😸

[calixtus]

@k3KAW8Pnf7mkmdSMPHz27 Big thanks for your work on this.

I just noticed some latex commands are not cleared in the now-master-build.
Has this been an issue before / is this still an issue with Latex2UnicodeAdapter?

Test library is JabRefAuthors in src/test/resources/testbib

[k3KAW8Pnf7mkmdSMPHz27]

Which file in particular and which settings entry table "Format of author and editor name"?

[k3KAW8Pnf7mkmdSMPHz27]

Hum... I am not sure what is going on, I'll have a look

[k3KAW8Pnf7mkmdSMPHz27]

Most of those should be "cleared" and are cleared when I build locally, but that might be something wrong with my setup.

[Siedlerchr]

Looks fine for me as well:

Firstname lastname option and do not abbreviate
Edit// @calixtus Do you have "Show names unchanged" activated?

[k3KAW8Pnf7mkmdSMPHz27]

I think I could reproduce @calixtus screenshot in JabRef 5.1--2020-05-28--ffa07cd but right now I seem unable to do so again (and I have tried).

[Siedlerchr]

@k3KAW8Pnf7mkmdSMPHz27 If you have "Show names unchanged" in the prefs activated then no conversion is happening

[calixtus]

I see, so the solution would be to change return nameToFormat to return Latex2UnicodeAdapter.format(nameToFormat) or similar in MainTableNameFormatter...

[k3KAW8Pnf7mkmdSMPHz27]

@calixtus I thought the intent of that option was to leave the names completely unchanged?
Also, if you change that, don't forget to add a cache

EDIT
With cache I mean something along the line of private String authorsNatbibLatexFree in AuthorList.

[calixtus]

Uh im not going to change anything today 😄
I was just wondering about that...

[k3KAW8Pnf7mkmdSMPHz27]

Hum... actually that is my bad. JabRef 5.0 does indeed "clear" the latex, even when Show names unchanged is set... Should I open up an issue and fix this then?

[tobiasdiez]

Yes, it would be nice if you could provide a fix (no need to create an issue before).

[k3KAW8Pnf7mkmdSMPHz27]

Ok, I apparently don't know how to do this. I created #6552

[k3KAW8Pnf7mkmdSMPHz27]

Well, thank you all for reviewing this PR, it is very appreciated! ❤️ 🎉 🎈

I'll take the reviews and results to heart, and make sure the next PR will create fewer issues 🤦