Skip to content

Preserve casing of field names for customize entry types#9993

Merged
Siedlerchr merged 13 commits into
mainfrom
preserve-casing
Jun 12, 2023
Merged

Preserve casing of field names for customize entry types#9993
Siedlerchr merged 13 commits into
mainfrom
preserve-casing

Conversation

@koppor

@koppor koppor commented Jun 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

github-actions[bot]
github-actions Bot reviewed Jun 9, 2023
View reviewed changes
Comment thread src/main/java/org/jabref/gui/preferences/customentrytypes/FieldViewModel.java Outdated
Comment thread src/main/java/org/jabref/gui/preferences/customentrytypes/CustomEntryTypesTab.java Outdated
Comment thread src/test/java/org/jabref/logic/exporter/MetaDataSerializerTest.java Outdated
@JabRef JabRef deleted a comment from github-actions Bot Jun 9, 2023
@koppor koppor mentioned this pull request Jun 9, 2023
Merged
6 tasks
@Siedlerchr

Siedlerchr commented Jun 9, 2023

Copy link
Copy Markdown
Member

BibtexDatabaseWriterTest > writeCustomizedTypesInAlphabeticalOrder() FAILED

Siedlerchr
Siedlerchr reviewed Jun 9, 2023
View reviewed changes
Comment thread src/main/java/org/jabref/model/entry/BibEntryType.java Outdated
", requiredFields=" + requiredFields +
", fields=" + fields +
'}';
return "type = " + type + "," + OS.NEWLINE +

@Siedlerchr Siedlerchr Jun 9, 2023

Copy link
Copy Markdown
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

@koppor koppor Jun 9, 2023

Copy link
Copy Markdown
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

Siedlerchr
Siedlerchr reviewed Jun 9, 2023
View reviewed changes
Comment thread src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java
bibtexContext.setMode(BibDatabaseMode.BIBTEX);

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

@Siedlerchr Siedlerchr Jun 9, 2023

Copy link
Copy Markdown
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

@koppor koppor Jun 9, 2023

Copy link
Copy Markdown
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

Siedlerchr commented Jun 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

koppor commented Jun 9, 2023

Copy link
Copy Markdown
Member Author

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

Siedlerchr commented Jun 10, 2023

Copy link
Copy Markdown
Member

Please take a look at the failng tests :-P

@Siedlerchr

Siedlerchr commented Jun 12, 2023

Copy link
Copy Markdown
Member

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

@koppor

koppor commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

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

Siedlerchr commented Jun 12, 2023

Copy link
Copy Markdown
Member

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

@koppor

koppor commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

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

Siedlerchr commented Jun 12, 2023

Copy link
Copy Markdown
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
@koppor koppor mentioned this pull request Jun 12, 2023
Closed
3 tasks
@myshevchuk myshevchuk mentioned this pull request Oct 27, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@koppor @Siedlerchr