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

Preserve casing of field names for customize entry types #9993

Merged
merged 13 commits into from
Jun 12, 2023

Conversation

koppor
Copy link
Member

@koppor koppor commented Jun 9, 2023

Follow-up to #9977, refs #9840.

This PR especially (tries) to adhere that Field is constant and that the ViewModel is the thing collecting data and at the end converts it to a "real" data object.

Result of a coding session started with @calixtus and @Siedlerchr.


image

@Comment{jabref-entrytype: person: req[GoogleScholar;ORCID;name] opt[]}

image

@Person{,
  googlescholar = {test1},
  name          = {test3},
  orcid         = {test2},
}

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.

@JabRef JabRef deleted a comment from github-actions bot Jun 9, 2023
@Siedlerchr
Copy link
Member

BibtexDatabaseWriterTest > writeCustomizedTypesInAlphabeticalOrder() FAILED

", requiredFields=" + requiredFields +
", fields=" + fields +
'}';
return "type = " + type + "," + OS.NEWLINE +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will break many tests! Rather add a new method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced org.jabref.gui.importer.BibEntryTypePrefsAndFileViewModel to really fix that comment.

image

@@ -395,7 +395,7 @@ void writeCustomizedTypesInAlphabeticalOrder() throws Exception {
database.insertEntry(entry);
bibtexContext.setMode(BibDatabaseMode.BIBTEX);

databaseWriter.savePartOfDatabase(bibtexContext, Arrays.asList(entry, otherEntry));
databaseWriter.savePartOfDatabase(bibtexContext, List.of(entry, otherEntry));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was a reason to pass a modifiable list

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked the code - no modification of the list inside. I added a JavaDoc comment.

@koppor koppor marked this pull request as draft June 9, 2023 19:50
@Siedlerchr
Copy link
Member

Siedlerchr commented Jun 9, 2023

The problem you know have is that the tests don't distinguish between custom field names and normal field names if they have the same name. This is hella complex now...

@koppor
Copy link
Member Author

koppor commented Jun 9, 2023

The problem you know have is that the tests don't distinguish between custom field names and normal field names if they have the same name. This is hella complex now...

Please unzip. 😅 MetaDataParsing: Yesterday, we opted to use org.jabref.model.entry.field.FieldFactory#parseField(T, java.lang.String), which never produces custom fields (UnknownField) in case the name is known. Thus, there cannot be any "custom field name" with a name available in standard field names. - The only issue is at the UI when generating the field. This place also uses the parseField (via org.jabref.gui.util.FieldsUtil#FIELD_STRING_CONVERTER).

@Siedlerchr
Copy link
Member

Please take a look at the failng tests :-P

@Siedlerchr
Copy link
Member

Only problem I now see is migration of existing custom entry types/fields.
that will break existing bib files

@koppor
Copy link
Member Author

koppor commented Jun 12, 2023

Only problem I now see is migration of existing custom entry types/fields.
that will break existing bib files

Why? When parsing the bib file, the casing is stored in JabRef. That casing is written out again. Thus, the bib file are kept.as is..IMHO also covered by tests.

What does break is the UI: The first letter will be lower case.

@Siedlerchr
Copy link
Member

Yeah, seems fine but after we resave it in the UI the casing is changed?

@koppor
Copy link
Member Author

koppor commented Jun 12, 2023

Yeah, seems fine but after we resave it in the UI the casing is changed?

The UI displays the casing found in the bib file..

User modifies casing: bib modified

User does not modify casing: bib not modified

IMHO as exected.

@Siedlerchr Siedlerchr marked this pull request as ready for review June 12, 2023 09:26
@Siedlerchr
Copy link
Member

no risk no fun!

@Siedlerchr Siedlerchr merged commit cd2d816 into main Jun 12, 2023
@Siedlerchr Siedlerchr deleted the preserve-casing branch June 12, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants