Oobranch d : apply style

[antalk2]

Adds style machinery: given a set of ordered citation groups
produce citation markers and bibliography in OOText format

[Siedlerchr]

did not yet look at all files

[Siedlerchr]

Is there a reason to change this? Won't break this backwards compatibility?

[antalk2]

https://wiki.openoffice.org/wiki/Documentation/DevGuide/Text/Overall_Document_Features#Styles
https://wiki.openoffice.org/wiki/API/Implementation_Details/Programmatic_StyleNames
How To Obtain Default PAra Style Name

Is there a reason to change this?

Styles have two names: one for programming (getName(), setName()) and one for display purposes DisplayName. The DisplayName is localized, meaning it can differ between locales. "Standard" is the programmatic (locale-independent) name for the paragraph style with DisplayName "Default" in some locales. The programmatic names work even if the locale used differs from that used by the author of a jstyle (or in this case jabref).

Won't break this backwards compatibility?

It should improve compatibility across locales and possible changes of DisplayName.

[antalk2]

In OOBibBase2.java/checkStyleExistsInTheDocument
I inserted a warning against non-programmatic style names,
to warn about locale-dependent style names in jstyle files..
That pointed out these builtin default values as well.
It will also warn, I believe about

File: src/main/resources/resource/openoffice/default_authoryear.jstyle
39:25:CitationCharacterFormat="Default"

File: src/main/resources/resource/openoffice/default_numerical.jstyle
27:25:CitationCharacterFormat="Default"

and some styles at jstyles.jabref.org-master

[Siedlerchr]

Is it really necessary to fill the list with Optional.empty? This looks a bit odd.
Can't you just skip them in a later step if there is no entry?

[antalk2]

normalizePageInfos is not used anymore, removing.

Similar representation is still present elsewhere. So the questions remain.

Is it really necessary to fill the list with Optional.empty? This looks a bit odd.

It does look odd. The original API (e.g. getCitationMarkers) used arrays and null values or entries.
Although originally there was only a single pageInfo for a citation group, now
we allow a pageInfo for each citation, and need a list or array to represent them.
When I needed to pass in multiple pageInfo values, I also used null values or null entries.

At this point normalizePageInfos (OOBibStyle.regularizePageInfosForCitations) was used by functions accepting null for "no pageInfo".

Now we pass List<CitationMarkerEntry> and similar for citation marker generation, with entries providing getPageInfo()

After changing getPageInfo() to return Optional, it became confusing to convert its Optional.empty result to null
when collecting, and then back to Optional for the parts expecting one.
So I attempted to standardize on always using Optional for pageInfo values internally.

At the boundaries we are still receiving null, and need to consider OODataModel.JabRef52 which
uses a single pageInfo per citation group (not per citation).

Where do we still use lists or arrays of pageInfo values?

• When creating a new citation group from the GUI,
  we receive a single pageInfo, that is converted to List<Optional<OOText>> in
  insertCitationGroup using OODataModel.fakePageInfos. This list is passed to UpdateCitationMarkers.createAndFillCitationGroup

• OODataModel.fakePageInfos is called in Backend52.combinePageInfosCommon for OODataModel.JabRef52 (to support EditMerge.mergeCitationGroups) (and in EditInsert.insertCitationGroup above)

• UpdateCitationMarkers.createAndFillCitationGroup is called in :
  EditInsert.insertCitationGroup (above), EditMerge.mergeCitationGroups and EditSeparate.separateCitations.
  The latter two are concerned about how to handle multiple pageInfo values during a merge and where to put the single pageInfo when breaking up a citation group (these are only problematic with the old datamodel).

• CitationEntry still corresponds to a citation group (not a citation) (and allows a single pageInfo)

• In the document we still use OODataModel.JabRef52

• OOFrontend.createCitationGroup : called from UpdateCitationMarkers.createAndFillCitationGroup

• Backend52.createCitationGroup (OODataModel.JabRef52 compatibility) called from OOFrontend.createCitationGroup

• OOBibStyleTestHelper.getCitationMarker2 has parameter String[] pageInfo to provide API similar to
  the old one, for easy comparison.

summary

The new API prefers citations to provide their pageInfo. We are passing around lists or arrays of pageInfo
values to handle the old datamodel. This includes the backend, EditMerge.mergeCitationGroups and EditSeparate.separateCitations, as well as interfacing to GUI parts (insertCitation and CitationEntry for ManageCitationsDialog).

We could use null entries instead of Optional, as we did for some time. At least in some cases it is going to
involve Optional -> null on collection and null -> Optional when separating them.
Or storing null values and converting them in getPageInfo to Optional, and back to null when collecting.

Can't you just skip them in a later step if there is no entry?

I am not sure I understand this. When is later?
For normalizePageInfos it is history, but
OODataModel.fakePageInfos is still in use, and produces
a List<Optional<OOText>> of normalized pageInfo values with a single (at most one) real (non-empty) entry.

It could return null or an empty list if the single entry becomes Optional.empty() itself.
Then at the locations using the result we should check and handle the special case.
This is what I was trying to avoid by moving the special cases to OODataModel and Backend.

Where should we handle specially

• insertCitationGroup calls fakePageInfo, uses the result to generate a preliminary citation marker here and also passes it to UpdateCitationMarkers.createAndFillCitationGroup, and ends up Backend52.createCitationGroup
• Backend52.combinePageInfosCommon calls fakePageInfos, called from Backend52.combinePageInfos which is called from mergeCitationGroups and the result is passed to UpdateCitationMarkers.createAndFillCitationGroup, and ends up in
  Backend52.createCitationGroup (same as above)

This is two places, plus documentation on UpdateCitationMarkers.createAndFillCitationGroup, OOFrontend.createCitationGroup and Backend52.createCitationGroup stating that for Backend52 (or OODataModel.JabRef52) null is an accepted value. I would suggest to wait with this.

• Backend60 would either also have to handle the special case, or we will need to ensure that it does not receive
  an abridged input.
• If we do not create / use OODataModel.JabRef52 documents, we will not need the OODataModel.JabRef52 branch in
  BackendXX.createCitationGroup and if we do not work with them, Backend.combinePageInfosCommon is not needed either.
• If [Cite special] in the gui learns to handle a pageInfo for each selected BibEntry, we will not need to go from a single
  nullable entry to a list in insertCitationGroup

[Siedlerchr]

something like this could work:
result = databases.stream().findFirst(database -> database.getEntryByCitationKey())
result.flatMap(...)

[antalk2]

original

        for (BibDatabase database : databases) {
            Optional<BibEntry> entry = database.getEntryByCitationKey(key);
            if (entry.isPresent()) {
                return Optional.of(new CitationLookupResult(entry.get(), database));
            }
        }
        return Optional.empty();

Need database in new CitationLookupResult(entry.get(), database)

I can express it as

        return databases.stream()
            .map(database ->
                 database.getEntryByCitationKey(key)
                 .map(e -> new CitationLookupResult(e, database)))
            .filter(Optional::isPresent)
            .findFirst()
            .flatMap(e -> e);

but it looks more complicated than the original. Did I miss some opportunity to simplify?

[calixtus]

The stream version is ok.
It's maybe a question of taste, I personally prefer the stream version, because I like the fluid interface. I think it can be read and understood quicker.

[antalk2]

I miss the types. Added lookup from a single database.
The nice thing with streams is that usually steps can be written in the order they are performed.
Here findFirst shortcuts two (or three in the version above) steps later.

Changed to

    public static Optional<CitationLookupResult> lookup(BibDatabase database, String key) {
        return (database
                .getEntryByCitationKey(key)
                .map(bibEntry -> new CitationLookupResult(bibEntry, database)));
    }

    public static Optional<CitationLookupResult> lookup(List<BibDatabase> databases, String key) {
        return (databases.stream()
                .map(database -> Citation.lookup(database, key))
                .filter(Optional::isPresent)
                .findFirst()
                .orElse(Optional.empty()));
    }

[antalk2]

Changed again, to avoid Optional<Optional>, Optional::get first

    public static Optional<CitationLookupResult> lookup(List<BibDatabase> databases, String key) {
        return (databases.stream()
                .map(database -> Citation.lookup(database, key))
                .filter(Optional::isPresent)
                .map(Optional::get)
                .findFirst());
    }

[Siedlerchr]

Object.equals(..)

[antalk2]

changed to

return Objects.equals(this.entry, that.entry) && Objects.equals(this.database, that.database);

Is this what you meant to suggest?

[Siedlerchr]

just Optional.of(db.get().entry)?

[antalk2]

No. db may be missing (no database, or not found in any), but we still want to
produce something, even if it is only "Unresolved(citationKey)". db.get() would throw in this case.

Would db.map(e -> e.entry ) better?
Certainly shorter. Changed to db.map(e -> e.entry)

[Siedlerchr]

How is the status here? Can we merge this or will this break something? Or should we rather integrate it in the other branch before?

[antalk2]

How is the status here? Can we merge this or will this break something?

These are additions, they do not affect behavior by themselves.
But they will after integration.

Or should we rather integrate it in the other branch before?

What does "other branch" refer to here?

[Siedlerchr]

Okay, thanks for the clarification!