Keep enclosing braces of authors #11034

koppor · 2024-03-15T11:00:02Z

Note that this PR also keeps braces inside the data model. A "better" data model would have a Boolean property with "protect", but we don't have. Thus, this is as good as it can get to fix the issue.

Mandatory checks

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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
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.

Siedlerchr · 2024-03-15T13:15:48Z

Plaease check the MsBibExporter as well there is a manual workaround for such a case

koppor · 2024-03-15T13:24:29Z

I am working on it! Therefore no label "ready for review". Have to suspend work for now, will resume later.

calixtus · 2024-03-15T14:44:22Z

src/main/java/org/jabref/model/entry/Author.java

+     * @param firstPart     the first name of the author (may consist of several tokens, like "Charles Louis Xavier Joseph" in "Charles Louis Xavier Joseph de la Vall{\'e}e Poussin")
+     * @param firstAbbr the abbreviated first name of the author (may consist of several tokens, like "C. L. X. J." in "Charles Louis Xavier Joseph de la Vall{\'e}e Poussin"). It is a responsibility of the caller to create a reasonable abbreviation of the first name.
+     * @param vonPart       the von part of the author's name (may consist of several tokens, like "de la" in "Charles Louis Xavier Joseph de la Vall{\'e}e Poussin")
+     * @param lastPart      the last name of the author (may consist of several tokens, like "Vall{\'e}e Poussin" in "Charles Louis Xavier Joseph de la Vall{\'e}e Poussin")
+     * @param jrPart        the junior part of the author's name (may consist of several tokens, like "Jr. III" in "Smith, Jr. III, John")


How about
givenName for firstPart
prefix for vonPart
suffix for jrPart
familyName for lastPart?
biblatex switched just a few years ago with the names. Was quite some effort to bugfix my styles.
See https://markov.htwsaar.de/tex-archive/macros/latex/contrib/biblatex/doc/biblatex.pdf#subsubsection.2.2.1

... When a small fix leads to a huge Refactoring...

Let's keep it for bibtex compat

Its only about variable names inside the code, not about parsing logic.

In some cultures, the family name comes first. Biblatex changed the var names to avoid confusion of the developers as biblatex grew.

Renameings done in 92a2024

Siedlerchr · 2024-03-15T14:54:33Z

src/main/java/org/jabref/logic/importer/AuthorListParser.java

-    public AuthorList parse(String listOfNames) {
-        Objects.requireNonNull(listOfNames);
-
+    public AuthorList parse(@NonNull String listOfNames) {


As I told you before I am not a fan of this NonNull stuff, It servers the same purpose as the Objects.requiresNonNull No benefit here

Yeah, I know. We always have that debate :)

I'll collect pros and cons in an ADR.

TL;DR for me. It enables static code analysis for nulls - not some runtime exception stuff. JabRef should not die because of NPEs.

Links - it was approved twice and did not cause any issues yet

Fix closing of JabRef in case of issues with index writer #10840

Add auto group colour assignment #10521 (comment)

koppor · 2024-03-17T01:28:28Z

Working on MSBib makes it a follow-up to #9729

Siedlerchr · 2024-03-17T12:46:23Z

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

@@ -120,47 +128,36 @@ public class MSBibMapping {
        BIBLATEX_TO_MS_BIB.put(new UnknownField(MSBIB_PREFIX + "productioncompany"), "ProductionCompany");
    }

+    static {
+        BIB_ENTRYTYPE_MAPPING.put(StandardEntryType.Book, MSBibEntryType.Book);


Inconsistent usage, for the others you use Map.of...

Map.of does not support that much parameters. I tried ,and. Checked jdk source

I moved the code and added a comment - so that others don't wonder, too.

github-actions · 2024-03-18T11:00:58Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

calixtus · 2024-03-18T11:03:39Z

src/main/java/org/jabref/logic/exporter/MSBibExporter.java

+    public void export(@NonNull BibDatabaseContext databaseContext,
+                       @NonNull Path file,
+                       @NonNull List<BibEntry> entries) throws SaveException {
+        Objects.requireNonNull(databaseContext); // required by test case


This is somekind of silly, isn't it?

Future work would be to introduce JSPecify more globally - but I did not want to escalate the PR here.

calixtus · 2024-03-18T11:08:35Z

src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java

 public class MsBibImporter extends Importer {

    private static final Logger LOGGER = LoggerFactory.getLogger(MsBibImporter.class);
    private static final String DISABLEDTD = "http://apache.org/xml/features/disallow-doctype-decl";
    private static final String DISABLEEXTERNALDTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
+    private static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = makeSafeDocBuilderFactory(DocumentBuilderFactory.newInstance());


The DocumentBuilderFactory as a constant? This smells funny, but also suspicious...

This has performance reasons. A Factory is instantiated once, but the produced things are instantiated more often (by the factory). - It takes time to setup the factory.

See https://community.sap.com/t5/technology-blogs-by-members/performance-improvements-in-nw-java-applications-with-xml-processing/ba-p/12894611 for more information.

Solution - Make XML Factories Static

calixtus · 2024-03-18T11:11:28Z

src/main/java/org/jabref/logic/openoffice/style/OOBibStyleGetCitationMarker.java

 class OOBibStyleGetCitationMarker {

+    private static final RemoveBracesFormatter REMOVE_BRACES_FORMATTER = new RemoveBracesFormatter();


Again, looks weird, but I have no better idea...

IIRC, we have this at other places, too. #Performance

* upstream/main: (40 commits) Improve citation relations (JabRef#11016) Keep enclosing braces of authors (JabRef#11034) Bump org.apache.pdfbox:fontbox from 3.0.1 to 3.0.2 (JabRef#11042) Bump com.dlsc.gemsfx:gemsfx from 2.2.0 to 2.4.0 (JabRef#11044) Bump org.apache.pdfbox:xmpbox from 3.0.1 to 3.0.2 (JabRef#11041) Bump com.googlecode.plist:dd-plist from 1.23 to 1.28 (JabRef#11040) Bump gittools/actions from 0.13.4 to 1.1.1 (JabRef#11039) Change copy-paste function to handle string constants (follow up PR) (JabRef#11037) Fixes Zotero file handling for absolute paths (JabRef#11038) Speed up failure reporting (JabRef#11030) Importing of BibDesk Groups and Linked Files (JabRef#10968) Convert RemoveBracesFormatterTest to @ParameterizedTest (JabRef#11033) Update teaching.md Remove non-existing recipe (JabRef#11029) Update CSL styles (JabRef#11031) Clean up defintions of entry types (JabRef#11013) Fix log file path on Windows (JabRef#11028) Change to rolling logs (JabRef#11023) chore: remove repetitive words (JabRef#11015) Fix test names (JabRef#11014) ...

* issue #10993 - feat: added ability to parse preferred-citation field to CffImporter * issue #10993 - feat: added all fields of JabRef/CITATION.cff to CffImporter * issue #10993 - feat: rewrote CffExporter to parse Software, Dataset types and authors names correctly * issue #10993 - feat: added keywords and unknown fields support * issue #10993 - feat: added round-trip test * issue #10993 - doc: updated CHANGELOG.md * Convert RemoveBracesFormatterTest to @ParameterizedTest (#11033) * Convert to @ParameterizedTest * Convert to csvsource --------- Co-authored-by: Carl Christian Snethlage <[email protected]> * Importing of BibDesk Groups and Linked Files (#10968) * Add test to check parsing of BibDesk Static Groups * Add test to check parsing of BibDesk Static Groups * Change isExpanded attribute to false in expected groups * remove extra blank line * Add tests to check parsing of BibDesk Smart and mixed groups * Add parsing of BibDesk Files * Attempts at plist * Now parses bdsk-file and shows it as a file in JabRef * Add test for parsing a bdsk-file field * Fix formatting * Add dd-plist library to documentation --------- Co-authored-by: Tian0602 <[email protected]> * Add creation of static JabRef group from a BibDesk file * Creates an empty ExplicitGroup from BibDesk comment * Adds citations to new groups modifies group creations to support multiple groups in the same BibDeskFile * Fix requested changes Refactor imports since they did not match with main Add safety check in addBibDeskGroupEntriesToJabRefGroups --------- Co-authored-by: Filippa Nilsson <[email protected]> * Refactor newline to match main branch Co-authored-by: Filippa Nilsson <[email protected]> * Add changes to CHANGELOG.md * Reformat indentation to match previous * Revert external libraries Adjust groups serializing * checkstyle and optional magic * fix * fix tests * fix * fix dangling do * better group tree metadata setting * merge group trees, prevent duplicate group assignment in entry Add new BibDesk group Fix IOB for change listeing * fix tests, and extract constant * return early * fixtest and checkstyle --------- Co-authored-by: Anna Maartensson <[email protected]> Co-authored-by: Tian0602 <[email protected]> Co-authored-by: LottaJohnsson <[email protected]> Co-authored-by: Filippa Nilsson <[email protected]> Co-authored-by: Filippa Nilsson <[email protected]> Co-authored-by: Oliver Kopp <[email protected]> Co-authored-by: Siedlerchr <[email protected]> * Speed up failure reporting (#11030) * Fixes Zotero file handling for absolute paths (#11038) * Fixes Zotero file handling for absolute paths Fixes #10959 * checkstyle mimiimm * fix changelog * cannot fix * Change copy-paste function to handle string constants (follow up PR) (#11037) * [Copy] Include string constants in copy (#11) Signed-off-by: Anders Blomqvist <[email protected]> * [Copy] New method for serializing string constants (#12) Signed-off-by: Anders Blomqvist <[email protected]> * Add a sanity check for null for clipboard content Currenlty, the clipboard content can be null since the database does not seem to be updating. This is a sanity check to prevent the program from adding null to the clipboard. Link to DD2480-Group1#13 * [Fix] Add parsed serilization when save settings When loading from existing files or libraries, the parser will set the serilization of the string constant to the correct value. However, when editing via the GUI, the serilization was not set and a new string constant list will be created without the serilization. This result in the serilization being null and when copying with the clipboard. Link to DD2480-Group1#13 * feat: import string constants when pasting #9 Add functionality to import string constants in the paste function Should add functionality to handle colliding string constants. Should also check that the constants are valid using the ConstantsItemModel class. * feat: Add string constant validity checker and dialog messages #9 Check that a pasted string constant is valid using the ConstantsItemModel class. Add diagnostic messages notifying users when adding a string constant fails while pasting. * [Copy] Copy referenced constant strings to clipboard (#16) * feat: Add parsed serialized string when cloning * feat: Add sanity check for null in ClipBoardManager * closes #15 * feat: new unit tests Add 4 new unit tests, testing the new features added for issue-10872. Specifically the tests are for the `storeSettings` method in the ConstantsPropertiesViewModel.java, and `setContent` in the ClipBaordManager.java. Closes #6 * Update CHANGELOG with copy and paste function * Fix Checkstyle failing by reformat the code * Fix OpenRewrite failing by running rewriteRun * Refactor by extract methods in setContent * collet failures * changelog and use os.newline * checkstyle * use real bibentrytypes manager * Fix CHANGELOG.md * Swap if branches * Code cleanup * Use List for getUsedStringValues * Fix submodule * Collection is better * Fix csl-styles * Remove empty line * Group BibTeX string l10n together --------- Signed-off-by: Anders Blomqvist <[email protected]> Co-authored-by: Anders Blomqvist <[email protected]> Co-authored-by: ZOU Hetai <[email protected]> Co-authored-by: Hannes Stig <[email protected]> Co-authored-by: Elliot <[email protected]> Co-authored-by: Oliver Kopp <[email protected]> * Bump gittools/actions from 0.13.4 to 1.1.1 (#11039) Bumps [gittools/actions](https://github.com/gittools/actions) from 0.13.4 to 1.1.1. - [Release notes](https://github.com/gittools/actions/releases) - [Commits](GitTools/actions@v0.13.4...v1.1.1) --- updated-dependencies: - dependency-name: gittools/actions dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump com.googlecode.plist:dd-plist from 1.23 to 1.28 (#11040) Bumps [com.googlecode.plist:dd-plist](https://github.com/3breadt/dd-plist) from 1.23 to 1.28. - [Release notes](https://github.com/3breadt/dd-plist/releases) - [Commits](3breadt/dd-plist@dd-plist-1.23...v1.28.0) --- updated-dependencies: - dependency-name: com.googlecode.plist:dd-plist dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.apache.pdfbox:xmpbox from 3.0.1 to 3.0.2 (#11041) Bumps org.apache.pdfbox:xmpbox from 3.0.1 to 3.0.2. --- updated-dependencies: - dependency-name: org.apache.pdfbox:xmpbox dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump com.dlsc.gemsfx:gemsfx from 2.2.0 to 2.4.0 (#11044) Bumps [com.dlsc.gemsfx:gemsfx](https://github.com/dlsc-software-consulting-gmbh/GemsFX) from 2.2.0 to 2.4.0. - [Release notes](https://github.com/dlsc-software-consulting-gmbh/GemsFX/releases) - [Changelog](https://github.com/dlsc-software-consulting-gmbh/GemsFX/blob/master/CHANGELOG.md) - [Commits](dlsc-software-consulting-gmbh/GemsFX@v2.2.0...v2.4.0) --- updated-dependencies: - dependency-name: com.dlsc.gemsfx:gemsfx dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.apache.pdfbox:fontbox from 3.0.1 to 3.0.2 (#11042) Bumps org.apache.pdfbox:fontbox from 3.0.1 to 3.0.2. --- updated-dependencies: - dependency-name: org.apache.pdfbox:fontbox dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Keep enclosing braces of authors (#11034) * Add test cases * Add test cases * Keep braces for last part * Refine method description * Adapt test to new braces keeping * Add CHANGELOG.md entry * Adapt tests * More edge cases * Minor code beautification * Simplify code * Fix braces removing * Extract static fields, refactor code * Fix removal of {} for export * Re-add Objects.requireNonNull * Fix typo * Re-add NPE throwing * Rename to modern terms * Consistent initialization * Improve citation relations (#11016) * Collect DOI and publication type from semantich scholar to be able to expand the information of the new entries later by search through DOI * Include abstract in the request. This lets the GUI show the abstract since that was implemented already. Refactor api request string since most of it is shared * Add button to open the relation paper's DOI URL. Fix DOI for some ArXiv entries. * Don't show the open link button if there is no link to open. * Make field value null error a bit more useful * Include SemanticScholar url in the request and use it as the URL field. * Add changes to changelog * Change tooltip text to an existing, more informative one * Run rewriter to fix pull request * improve url optional handling --------- Co-authored-by: Siedlerchr <[email protected]> * issue #10993 - doc: updated CHANGELOG.md * fix: fixed unit tests not passing due to name changes in Author interface (#10995) * feat: changed CFFExporter to use YAML library snakeyaml instead (#10995) * feat: added support for references and ALL possible CFF fields in importer (#10995) * fix: added requested changes (#10995) + updated CHANGELOG.md + removed useless comments + refactored both CffImporter and CffExporter to use more specific methods + used a BiMap to avoid repeating mappings between CffImporter and CffExporter + copied entryMap in exporter to avoid side-effects * fix: task rewriteDryRun fixed to pass by removing test in BibEntryTest * refactor: deleted useless methods in CffImporter (#10995) * doc: added decision MADR document for cff export (#10995) * feat: add a cites or related relationship between imported entries in CffImporter (#10995) * doc: updated MADR decision document for cff export to pass markdownlint (#10995) * fix: fixed round-trip test to use mock citatioKeyPatternPreferences correctly (#10995) * fix: fixed MADR document for CFF export decision to pass Jekyll CI check (#10995) * fix: fixed requested changes (#10995) + fixed typo in CHANGELOG.md + tested multiline abstract in CFFImporter * feat: finished CFFExporter logic and crafted working round-trip test (#10995) * fix: fixed typos in MADR decision doc for CFF export and refactore ImportFormatReader signature (#10995) * Some code beautification * Use existing method getEntryLinkList * Use getEntryLinkList * Use JabRef's Date class for parsing --------- Signed-off-by: Anders Blomqvist <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Oliver Kopp <[email protected]> Co-authored-by: Carl Christian Snethlage <[email protected]> Co-authored-by: Emil Hultcrantz <[email protected]> Co-authored-by: Anna Maartensson <[email protected]> Co-authored-by: Tian0602 <[email protected]> Co-authored-by: LottaJohnsson <[email protected]> Co-authored-by: Filippa Nilsson <[email protected]> Co-authored-by: Filippa Nilsson <[email protected]> Co-authored-by: Siedlerchr <[email protected]> Co-authored-by: Anders Blomqvist <[email protected]> Co-authored-by: ZOU Hetai <[email protected]> Co-authored-by: Hannes Stig <[email protected]> Co-authored-by: Elliot <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Roc <[email protected]>

koppor added 11 commits March 15, 2024 11:14

Add test cases

45cfdba

Add test cases

71b2440

Keep braces for last part

f5f2151

Refine method description

b0d490c

Adapt test to new braces keeping

4b81e61

Add CHANGELOG.md entry

9187381

Adapt tests

2750512

More edge cases

d7ad9da

Minor code beautification

aa48654

Simplify code

1690fe5

Fix braces removing

40c4ca7

calixtus reviewed Mar 15, 2024

View reviewed changes

Siedlerchr reviewed Mar 15, 2024

View reviewed changes

koppor added 5 commits March 17, 2024 03:01

Extract static fields, refactor code

dec0186

Fix removal of {} for export

ed16b78

Re-add Objects.requireNonNull

b588668

Fix typo

c54d006

Re-add NPE throwing

856cf83

koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 17, 2024

Rename to modern terms

92a2024

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 17, 2024

Siedlerchr reviewed Mar 17, 2024

View reviewed changes

Consistent initialization

9098099

calixtus reviewed Mar 18, 2024

View reviewed changes

Siedlerchr approved these changes Mar 18, 2024

View reviewed changes

koppor added this pull request to the merge queue Mar 18, 2024

Merged via the queue into main with commit a902524 Mar 18, 2024
20 of 21 checks passed

koppor deleted the fix-10031 branch March 18, 2024 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep enclosing braces of authors #11034

Keep enclosing braces of authors #11034

koppor commented Mar 15, 2024

Siedlerchr commented Mar 15, 2024

koppor commented Mar 15, 2024

calixtus Mar 15, 2024

koppor Mar 15, 2024

Siedlerchr Mar 15, 2024

koppor Mar 15, 2024

calixtus Mar 16, 2024 •

edited

Loading

koppor Mar 17, 2024

Siedlerchr Mar 15, 2024 •

edited

Loading

koppor Mar 17, 2024

koppor commented Mar 17, 2024

Siedlerchr Mar 17, 2024

koppor Mar 17, 2024

koppor Mar 18, 2024

github-actions bot commented Mar 18, 2024 •

edited

Loading

calixtus Mar 18, 2024

koppor Mar 18, 2024

calixtus Mar 18, 2024

koppor Mar 18, 2024

calixtus Mar 18, 2024

koppor Mar 18, 2024

		class OOBibStyleGetCitationMarker {

		private static final RemoveBracesFormatter REMOVE_BRACES_FORMATTER = new RemoveBracesFormatter();

Keep enclosing braces of authors #11034

Keep enclosing braces of authors #11034

Conversation

koppor commented Mar 15, 2024

Mandatory checks

Siedlerchr commented Mar 15, 2024

koppor commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixtus Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixtus Mar 16, 2024 •

edited

Loading

Siedlerchr Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading