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 CSL rendering in case of article #8607

Merged
merged 23 commits into from
Aug 30, 2022
Merged

Fix CSL rendering in case of article #8607

merged 23 commits into from
Aug 30, 2022

Conversation

koppor
Copy link
Member

@koppor koppor commented Mar 26, 2022

Fixes #8372
Fixes koppor#514
Fixes https://discourse.jabref.org/t/unable-to-edit-my-bibtex-file-that-i-used-before-vers-5-1/2390/4
Fixes https://discourse.jabref.org/t/jabref-5-6-need-help-with-export-from-jabref-to-microsoft-word-entry-preview-of-apa-7-not-rendering-correctly/3462/6

Currently, only test cases showing the intended output are added. We still need to discuss where to put the translation logic. Since the CSL processor is not aware of bibtex/biblatex, we should do the translation based on the context. The translation logic is shown at #8372 (comment). The horizontal arrows mean: put at that place if not existant. A similar translationlogic (excel table style) is shown in #8607 (comment)

Next steps:

  • Implement translation logic
  • Check if test cases match.
  • If tests do not match, think of the edge cases and fix them

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor koppor marked this pull request as draft March 26, 2022 10:00
@Siedlerchr
Copy link
Member

The logic for "translating" would be needed here: A while back I already passed down the bibdatabasecontext so we can get the mode.:

/**
* Converts the {@link BibEntry} into {@link CSLItemData}.
*/
private static CSLItemData bibEntryToCSLItemData(BibEntry bibEntry, BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager) {
String citeKey = bibEntry.getCitationKey().orElse("");
BibTeXEntry bibTeXEntry = new BibTeXEntry(new Key(bibEntry.getType().getName()), new Key(citeKey));
// Not every field is already generated into latex free fields
RemoveNewlinesFormatter removeNewlinesFormatter = new RemoveNewlinesFormatter();
Optional<BibEntryType> entryType = entryTypesManager.enrich(bibEntry.getType(), bibDatabaseContext.getMode());
Set<Field> fields = entryType.map(BibEntryType::getAllFields).orElse(bibEntry.getFields());
for (Field key : fields) {
bibEntry.getResolvedFieldOrAlias(key, bibDatabaseContext.getDatabase())
.map(removeNewlinesFormatter::format)
.map(LatexToUnicodeAdapter::format)
.ifPresent(value -> {
if (StandardField.MONTH.equals(key)) {
// Change month from #mon# to mon because CSL does not support the former format
value = bibEntry.getMonth().map(Month::getShortName).orElse(value);
}
bibTeXEntry.addField(new Key(key.getName()), new DigitStringValue(value));
});
}
return BIBTEX_CONVERTER.toItemData(bibTeXEntry);

koppor and others added 3 commits March 28, 2022 22:00
Co-authored-by: Christoph <siedlerkiller@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@koppor
Copy link
Member Author

koppor commented Mar 28, 2022

Current state: At test 13, we pass both issue and number to CSL Proc, but not a "page", it is not displayed.

grafik

We tried to put "777" in the pages, it works. "Article 777" in pages has maybe issues...

@ThiloteE ThiloteE added this to the v5.6 milestone Apr 7, 2022
@koppor koppor modified the milestones: v5.6, v5.7 Apr 11, 2022
@ThiloteE ThiloteE added bib(la)tex entry-preview bug Confirmed bugs or reports that are very likely to be bugs and removed bug Confirmed bugs or reports that are very likely to be bugs labels Apr 20, 2022
koppor and others added 5 commits April 25, 2022 22:37
# Conflicts:
#	src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java
Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
@ThiloteE

This comment was marked as duplicate.

* upstream/main: (185 commits)
  Snapcraft and issue template
  Show development information\n\n+semver: minor
  Release v5.7
  New Crowdin updates (#9030)
  New Crowdin updates (#9029)
  [Bot] Update CSL styles (#9027)
  Add missing translations for AutomaticFieldEditor (#9028)
  [Bot] Update Journal abbrev list (#9026)
  Rating in main table (#9023)
  New Crowdin updates (#9024)
  New Crowdin updates (#9016)
  New Crowdin updates (#9013)
  try to gather more output from LO exception (#9002)
  Improve Automatic Field Editor Dialog (#8973)
  Update BST VM to Antlr4 (#8934)
  Support biblatex apa citation for legal entry types (#8966)
  Bump junit-jupiter from 5.8.2 to 5.9.0 (#9012)
  Bump lucene-core from 9.2.0 to 9.3.0 (#9009)
  Bump checkstyle from 10.3.1 to 10.3.2 (#9006)
  Bump appleboy/ssh-action from 0.1.4 to 0.1.5 (#9005)
  ...
@koppor koppor modified the milestones: v5.7, v5.8 Aug 8, 2022
@ThiloteE
Copy link
Member

ThiloteE commented Aug 10, 2022

BibTeX BibLaTeX EntryPreview/CSL proposed logic, conditions and info
volume volume volume
number issue issue For conversion to CSL or BibTeX: BibLaTeX number takes priority and supersedes BibLaTeX issue
number number issue For conversion to CSL or BibTeX: BibLaTeX number takes priority and supersedes BibLaTeX issue
pages eid number Some journals put the article-number (= eid) into the pages field. If BibLaTeX eid exists, provide csl number to the style. If pages exists, provide csl page. If eid WITHIN the pages field exists, detect the eid and provide csl number. If both eid and pages exists, ideally provide both csl number and csl page. Ideally the citationstyle should be able to flexibly choose the rendering. If this is not possible then map biblatex eid to CSL page, since this is sufficient to comply with APA 7th edition, but will break other citationstyles (e.g. IEEE)
pages pages page Some journals put the article-number (= eid) into the pages field. If BibLaTeX eid exists, provide csl number to the style. If pages exists, provide csl page. If eid WITHIN the pages field exists, detect the eid and provide csl number. If both eid and pages exists, ideally provide both csl number and csl page. Ideally the citationstyle should be able to flexibly choose the rendering. If this is not possible then map biblatex eid to CSL page, since this is sufficient to comply with APA 7th edition, but will break other citationstyles (e.g. IEEE)

@Siedlerchr
Copy link
Member

TIL: Apa CSL has no field number

@koppor koppor marked this pull request as ready for review August 10, 2022 22:56
@ThiloteE
Copy link
Member

ThiloteE commented Aug 10, 2022

Known problems of this implementation (edited):

  1. A negative tradeoff between rendering IEEE or APA style correctly. The CSL APA citationstyle is not able to render "number" (even though it should. This is tracked in Issue vs Number APA 7th edition; Number and Issue not shown under certain conditions citation-style-language/styles#5827) How to deal with this? - For JabRef, the first option would be to map eid to the page field, otherwise eid will not get rendered at all. Unfortunately this would mean that the IEEE citationstyle now renders article numbers in the pages field and in the eid field with p. prefixed. Since users have the option to move data from the eid field to the pages field with JabRef's new "automatic entry editor", they will be able to render article numbers even with APA style via workaround. Hence, I went with the second option, mapping Biblatex eid to CSL number, which is the correct thing to do if purely following the Biblatex and CSL standards documentation. Once CSL APA style supports the "number" field, JabRef will only have to update a few test cases, but not change its biblatex to csl translation logic.

  2. The biblatex issue field will only get rendered, if there is no number field present in the entry. If both are present, the biblatex number field takes priority.

Overall though, I would suggest this to be an improvement to the status quo.

@ThiloteE
Copy link
Member

ThiloteE commented Aug 12, 2022

Waiting a little bit for CSL maintainers to potentially refactor the APA Styles. Tracked in citation-style-language/styles#5827. We have time until JabRef 5.8 will be released anyway....

@Siedlerchr Siedlerchr merged commit 554aee1 into main Aug 30, 2022
@Siedlerchr Siedlerchr deleted the fix-8372 branch August 30, 2022 21:02
Siedlerchr added a commit that referenced this pull request Sep 2, 2022
* upstream/main: (387 commits)
  Show a warning in the merge dialog when authors are the same but formatted differently (#9088)
  Fix subdatabase from aux on cli (#9117)
  Visual improvements to LinkedFilesEditor (#9114)
  SLR Remove "last-search-date" (#9116)
  Hide diffs when one of the field values is blank a.k.a no conflict (#9110)
  Squashed 'buildres/csl/csl-locales/' changes from e637746677..b2afeb4d87
  Squashed 'buildres/csl/csl-styles/' changes from c750b6e..8d69f16
  Fix title case capitalization after en-dash characters (#9102)
  Update journal abbrev list (#9109)
  Fix CSL rendering in case of article (#8607)
  [WIP][GSOC22] - C - Improve the external changes resolver dialog (#9021)
  Bump jsoup from 1.15.1 to 1.15.3 (#9103)
  Bump checkstyle from 10.3.2 to 10.3.3 (#9104)
  Bump postgresql from 42.4.2 to 42.5.0 (#9105)
  Bump unirest-java from 3.13.10 to 3.13.11 (#9106)
  Include check for TimeStamp (#9089)
  Close OO connection on JabRef exit (#9076)
  Bump slf4j-tinylog from 2.4.1 to 2.5.0 (#9085)
  Bump bcprov-jdk18on from 1.71 to 1.71.1 (#9079)
  Bump tinylog-impl from 2.4.1 to 2.5.0 (#9086)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java
#	src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
#	src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java
@ThiloteE ThiloteE mentioned this pull request Oct 10, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bib(la)tex entry-preview status: depends-on-external A bug or issue that depends on an update of an external library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entry preview not rendering the citation properly User is unable to edit his BibTeX file
4 participants