Suggestions for changes in caching latex free authors

[k3KAW8Pnf7mkmdSMPHz27]

This PR helps with caching of latex-free last names for PR #7228. It should also improve readability.

The main idea is to cache a latex-free AuthorList rather than a different latex-free String for every different use case.

Some suggestions are based on the discussion in #6552 (comment), where,

1. nameStyle refers to the possibility of having different name parts in biblatex, it won't be addressed in this PR.
2. displayStyle (e.g., getLastOnly) and conjugationStyle (e.g., Natbib) will likely be addressed by streams and collectors Suggestions for changes in caching latex free authors #7301 (comment).

I'll flesh out the to-do list in the coming days.

• Change the previous caching solution for latex-free authors
  • Cache in Author
  • Cache in AuthorList
  • Remove the previous caching?
  • Cache Strings used in maintable?
• "Modernize" the AuthorList construction
  • Add AuthorList.of
  • Add a Collector for AuthorList
  • Remove all public constructors (requires changes in AuthorListParser)
    • Mark all remaining public constructors as deprecated
• "Modernize" the various AuthorList -> String methods
  • Draft using static String andCoordinatedConjunction
  • andCoordinatedConjunction should be using Optional<String> and not String since an author might be lacking the used value (should be done but out of scope for this PR)
• Evaluate performance impact
  • Avoid creating an intermediate list in Collector
• JavaDoc
  • Make sure that no one will use the stream collector on bibEntry.getField().split(" and ").collect(...)
• Update /layout/format/Authors.java to take advantage of AuthorList This will have to be done in follow up PR. It benefits from other changes to Authors.java and Author.java.

• 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. (no new functionality)

[tobiasdiez]

One could use the same strategy as for the Author class here, and introduce a AuthorList latexFree() method. Then one could also remove all the "latexFree" versions of the methods below.

[k3KAW8Pnf7mkmdSMPHz27]

I'll try to implements List<Author> with a cache strategy so that repeated AuthorList.latexFree().latexFree()... calls returns the same AuthorList (I am trying to make it immutable anyway).

[tobiasdiez]

The repeated calls you might be able to deal with as follows:

latexFree() {
  if authorsLatexFree == null
      authorsLatexFree = AuthorList.of(unicode converted...)
      authorsLatexFree.authorsLatexFree = authorsLatexFree 
}
return authorsLatexFree;

but honestly, I wouldn't worry too much about it. In the normal usage, you get the author list from an entry and then use latexFree once if you want to have a latexfree version.

Implementing List<Author> is a good idea but might be a lot of work. For example, you want to make sure that add/remove etc update also the latexfree cache. Maybe it's enough for now to implement AuthorList.stream().

[k3KAW8Pnf7mkmdSMPHz27]

Implementing List<Author> is a good idea but might be a lot of work

I have to admit I have not looked into it in too much detail yet. My "high-level" approach were to make an unmodifiable list based on util.AbstractList. At least the API documentation claims it shouldn't be too hard 😜

you want to make sure that add/remove etc update also the latexfree cache

Regarding immutability

I believe AuthorList should be unmodifiable because,

1. It is non-trivial to add/remove authors while dealing with bibtex. {Barnes and Noble} and Author would need to insert Noble between Barnes and Author if the braces are removed. It is much easier to just re-parse it.
2. There is currently no parts of JabRef that relies on a list of authors being mutable. Barring AuthorListParser I have replaced all uses with Streams. The mutability required by AuthorListParser can be addressed but I haven't decided what is the best solution that does not come at a performance cost. (I am leaning towards making AuthorListParser an inner class of AuthorList to avoid creating an extra list/array through AuthorList.of)
3. Even if I am wrong, all static methods in AuthorList can (and in my opinion should) be made to work with List<Author>. Worst case scenario, someone that must have AuthorList mutability can use any class extending List and use static calls instead.

Regarding mutability

Because I think immutability is the right approach, I haven't really spent as much thought on this option. I think that using an EnumMap (or anything with an iterator) to store cached lists of AuthorLists that are extended by Decorators would allow an easy and less error prone implementation. Adding/removing elements can be overridden to primarily work with indices.

E.g., if a latex free AuthorList wants to remove a specific Author, first make sure that the Author is latex free and then find the index of that Author (if it is in the list), then use an iterator over the cached AuthorLists to remove that specific index. It is really not safe for concurrent modifications but neither is the current one.

I can add a "draft" implementation of one of these decorators if you'd prefer that to this explanation.

"P.S.", if you think mutability is the way to go, feel free to tell me that, and I'll look into it more.

[tobiasdiez]

My first thought would be to have it as a mutable list. This might become handy in the future, although I don't really have a use case right now. But for now I would keep the structure like it is right now with a wrapped List<Author> variable. Of course, if you want to add convient methods like stream() or something like this, you are very welcome to do so. I suggest we keep it growing organically, and if at some point many of the list methods are implemented we can also fully implement the interface.

[k3KAW8Pnf7mkmdSMPHz27]

AuthorList -> str methods in various places

The AuthorList -> ? is needed in various places. Take institute authors, both in

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

Lines 127 to 131 in 54f27e9

	// FIXME: #4152 This is an ugly hack because the latex2unicode formatter kills of all curly braces, so no more corporate author parsing possible
	String authorLatexFree = entry.getLatexFreeField(field).orElse("");
	if (corporate) {
	authorLatexFree = "{" + authorLatexFree + "}";
	}

and

jabref/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java

Lines 497 to 501 in 54f27e9

	String lastName = author.getLast()
	.map(lastPart -> isInstitution(author) ?
	generateInstitutionKey(lastPart) :
	LatexToUnicodeAdapter.format(lastPart))
	.orElse(null);

In both locations they should rely on the parsing of AuthorListParser and only utilize List<Author>.
I even have an Author.isInstitute() method in one of my local branches ^^

getLatexFreeField could be considered the culprit of this.

You can also view #7228 as related to this.

[k3KAW8Pnf7mkmdSMPHz27]

I dunno. Do you think this discussion will lead to anything concrete?
I am happy just implementing public AuthorList latexFree() and killing of quite a few methods 😄

As you might have noticed I have ideas both for dealing with getLatexFreeField and implementing List. Perhaps that is again better for a different PR and I can expand on the idea(s) so they can be criticised in a more structure fashion.

[tobiasdiez]

Yes, these workarounds should actually be moved to the AuthorListParser. There was a dark time in JabRef's history, where there was no Author class and everyone reimplemented different parsing and serialization schemes again and again. Happily it's now 2021 ;-)

[Siedlerchr]

I guess it makes sense to add the "Workarounds" for the Authorlist in one central place. And especially institution handling is important for exporters

[k3KAW8Pnf7mkmdSMPHz27]

I'll add a follow-up PR moving the formatting code of layout/Authors.java to AuthorList. I believe they produce different results when using oxford commas for two authors, which is not ideal.

[calixtus]

Devcall: whats the status here?

[k3KAW8Pnf7mkmdSMPHz27]

@calixtus basically me not having as much time for this as I would have hoped.

The remaining "big picture" parts (in my opinion) are to,

1. make it harder to use lists of authors without the AuthorList class (to discourage writing more author parsers)
2. figure out how to minimize the performance impact

[k3KAW8Pnf7mkmdSMPHz27]

Using the extremely scientific approach of literally measuring the time it takes to open a .bib with 6400+ entries and an AutomatedPersonGroup takes me 47s until the 'AllEntries' count shows up in the current master and 50s with the changes currently in this branch.
Perhaps there is nothing to worry about.

[k3KAW8Pnf7mkmdSMPHz27]

@calixtus @tobiasdiez would it help if I split this into 2-3 PRs? It should make it easier/less tedious to review.

[tobiasdiez]

Sorry, it just didn't had this on the radar any more. I've now went through it and it looks very good. I have one suggestion for the caching...

[k3KAW8Pnf7mkmdSMPHz27]

Sorry, it just didn't had this on the radar any more

No worries. I am trying to learn to scope/limit my PRs better. My apologies that it grew this big and I appreciate that you have stuck with it from the start! ❤️

[tobiasdiez]

LGTM! ❤️

[calixtus]

Went through your changes. Looks good to me too. Nothing more to say. 😄 Merging now.