Skip to content

Conversation

@rahulkarru
Copy link
Contributor

Closes #14416
I have extracted the NewEntryPreferences logic from the main JabRefPreferences class into its own dedicated class. This reduces the size of the main preferences class and groups related settings together.

Specific implementation details:

  • Created NewEntryPreferences with a private constructor for hardcoded defaults.
  • Updated JabRefPreferences to read/write from this new object.
  • Imports & Types: In the new class constructor, I switched to using StandardEntryType objects directly (instead of parsing Strings) to ensure better type safety for the default values. This required adding the necessary imports for EntryType and NewEntryDialogTab.

Steps to test

  1. Run JabRef.
  2. Go to Options > Preferences.
  3. Check the Entry or New Entry settings (e.g., change "Generate key on import" or "Default entry type").
  4. Save and Close JabRef.
  5. Reopen JabRef and verify that the settings were remembered.
  6. Try resetting the preferences to default to ensure the hardcoded defaults work correctly.

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (not applicable, pure refactoring)
  • [/] I added screenshots in the PR description (UI did not change)
  • [/] I described the change in CHANGELOG.md (Internal refactoring, not visible to user)
  • [/] I checked the user documentation (No behavior change)

@github-actions
Copy link
Contributor

Hey @rahulkarru!

Thank you for contributing to JabRef! Your help is truly appreciated ❤️

We have automated checks in place, based on which you will soon get feedback if any of them are failing. In a while, maintainers will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs.

Please re-check our contribution guide in case of any other doubts related to our contribution workflow.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Why is so much code moved? Is this AI-generated?

Comment on lines 27 to 41
NewEntryDialogTab.CHOOSE_ENTRY_TYPE,

true,

false,

true,

StandardEntryType.Article,

true,

DoiFetcher.NAME,

PlainCitationParserChoice.RULE_BASED_GENERAL.getLocalizedName()
Copy link
Member

Choose a reason for hiding this comment

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

Fix empty lines - and add comments - as outlined in the issue description

@github-actions github-actions bot added the status: changes-required Pull requests that are not yet complete label Nov 25, 2025
getStringList(SELECTED_SLR_CATALOGS));
}

private NewEntryPreferences getNewEntryPreferencesFromLowLevelApi(NewEntryPreferences defaults) {
Copy link
Member

Choose a reason for hiding this comment

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

Rename ti XXXFromBackingStore

getStringList(SELECTED_SLR_CATALOGS));
}

private NewEntryPreferences getNewEntryPreferencesFromLowLevelApi(NewEntryPreferences defaults) {
Copy link
Member

Choose a reason for hiding this comment

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

Please move below getNewEntryPreferences

Comment on lines 384 to 407
private static List<String> getColumnNamesAsStringList(ColumnPreferences columnPreferences) {
return columnPreferences.getColumns().stream()
.map(MainTableColumnModel::getName)
.toList();
}

private static List<String> getColumnWidthsAsStringList(ColumnPreferences columnPreferences) {
return columnPreferences.getColumns().stream()
.map(column -> column.widthProperty().getValue().toString())
.toList();
}

private static List<String> getColumnSortTypesAsStringList(ColumnPreferences columnPreferences) {
return columnPreferences.getColumns().stream()
.map(column -> column.sortTypeProperty().getValue().toString())
.toList();
}

private static List<String> getColumnSortOrderAsStringList(ColumnPreferences columnPreferences) {
return columnPreferences.getColumnSortOrder().stream()
.map(MainTableColumnModel::getName)
.collect(Collectors.toList());
}

Copy link
Member

Choose a reason for hiding this comment

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

Please move back to original position in code. Reason: Keep together what belongs together in code. Makes understanding easier.


// endregion

// region: Main table, main table column, and search dialog column preferences
Copy link
Member

Choose a reason for hiding this comment

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

Please keep region comments

@github-actions github-actions bot removed the status: changes-required Pull requests that are not yet complete label Nov 26, 2025
@calixtus
Copy link
Member

You forgot to add to clear and import methods

@koppor koppor added the status: changes-required Pull requests that are not yet complete label Nov 26, 2025
@rahulkarru
Copy link
Contributor Author

rgot to add to clear and import methods

I guess I had added right could you specify that?

@rahulkarru rahulkarru requested a review from koppor November 26, 2025 16:11
Comment on lines 1198 to 1204
int approachIndex = getInt("createEntryApproach", defaults.getLatestApproach().ordinal());
NewEntryDialogTab approach;
if (approachIndex >= 0 && approachIndex < NewEntryDialogTab.values().length) {
approach = NewEntryDialogTab.values()[approachIndex];
} else {
approach = defaults.getLatestApproach();
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not the original logic? (which is btw now dead code in the getNewEntryPreferences() method)

Comment on lines 1206 to 1207
String typeName = get("createEntryImmediateType", defaults.getLatestImmediateType().getDisplayName());
EntryType immediateType = EntryTypeFactory.parse(typeName);
Copy link
Member

Choose a reason for hiding this comment

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

In the original logic, this was testing only for StandardTypes. Please provide at least some reasoning why this should also be tested for other types.

Comment on lines 1169 to 1181
final int approachIndex = getInt(CREATE_ENTRY_APPROACH);
NewEntryDialogTab approach = NewEntryDialogTab.values().length > approachIndex
? NewEntryDialogTab.values()[approachIndex]
: NewEntryDialogTab.values()[0];

final String immediateTypeName = get(CREATE_ENTRY_IMMEDIATE_TYPE);
EntryType immediateType = StandardEntryType.Article;
for (StandardEntryType type : StandardEntryType.values()) {
if (type.getDisplayName().equals(immediateTypeName)) {
immediateType = type;
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Now dead code.

approach = defaults.getLatestApproach();
}

String typeName = get("createEntryImmediateType", defaults.getLatestImmediateType().getDisplayName());
Copy link
Member

Choose a reason for hiding this comment

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

Please use the constants

}

private NewEntryPreferences getNewEntryPreferencesFromBackingStore(NewEntryPreferences defaults) {
int approachIndex = getInt("createEntryApproach", defaults.getLatestApproach().ordinal());
Copy link
Member

Choose a reason for hiding this comment

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

Please use the constants

@calixtus
Copy link
Member

rgot to add to clear and import methods

I guess I had added right could you specify that?

i overlooked that change. my fault.

rahulkarru added 2 commits November 27, 2025 08:47
…ferences

# Conflicts:
#	jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java
@github-actions github-actions bot removed the status: changes-required Pull requests that are not yet complete label Nov 27, 2025
koppor
koppor previously requested changes Nov 28, 2025
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

There is no removal of defaults.put things - and some strange donation changes are in... Maybe, wrong PR conflict resolution

image

Comment on lines 366 to 369
// region donation defaults
defaults.put(DONATION_NEVER_SHOW, Boolean.FALSE);
defaults.put(DONATION_LAST_SHOWN_EPOCH_DAY, -1);
// endregion
Copy link
Member

Choose a reason for hiding this comment

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

Something is broken here -- this PR does not touch donations.

Copy link
Member

Choose a reason for hiding this comment

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

Donations were handled at #14439

@github-actions github-actions bot added the status: changes-required Pull requests that are not yet complete label Nov 28, 2025
rahulkarru and others added 3 commits November 28, 2025 08:41
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@koppor koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes-required Pull requests that are not yet complete labels Nov 28, 2025

private NewEntryPreferences getNewEntryPreferencesFromBackingStore(NewEntryPreferences defaults) {
// moved the same snippet form public to private
final int approachIndex = getInt(CREATE_ENTRY_APPROACH);
Copy link
Member

Choose a reason for hiding this comment

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

What about the default value?

? NewEntryDialogTab.values()[approachIndex]
: NewEntryDialogTab.values()[0];

final String immediateTypeName = get(CREATE_ENTRY_IMMEDIATE_TYPE);
Copy link
Member

Choose a reason for hiding this comment

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

What about the default value?

@github-actions github-actions bot added status: changes-required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Nov 28, 2025
@github-actions github-actions bot removed the status: changes-required Pull requests that are not yet complete label Nov 29, 2025
}

private NewEntryPreferences getNewEntryPreferencesFromBackingStore(NewEntryPreferences defaults) {
// moved the same snippet form public to private
Copy link
Member

Choose a reason for hiding this comment

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

ai comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it' wasn't extremely an ai comment it was the comment written by myself to under stand for reviewer

Copy link
Member

Choose a reason for hiding this comment

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

but why would you put a comment like this into the production code. nobody would understand it, who does not know how the code looked previously.

Comment on lines 1184 to 1185
// if any values not match it assign this value
: defaults.getLatestApproach();
Copy link
Member

Choose a reason for hiding this comment

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

why change this?

: defaults.getLatestApproach();

final String immediateTypeName = get(CREATE_ENTRY_IMMEDIATE_TYPE, defaults.getLatestImmediateType().getDisplayName());
// logic change to assign default values at the start
Copy link
Member

Choose a reason for hiding this comment

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

another ai comment

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Remove ai comments, reason for logic change

@github-actions github-actions bot added the status: changes-required Pull requests that are not yet complete label Nov 30, 2025
@github-actions github-actions bot removed the status: changes-required Pull requests that are not yet complete label Dec 1, 2025
@koppor koppor requested a review from calixtus December 1, 2025 13:39
@calixtus calixtus enabled auto-merge December 1, 2025 22:21
@calixtus calixtus added this pull request to the merge queue Dec 1, 2025
Merged via the queue into JabRef:main with commit 9331991 Dec 1, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable resetting of NewEntryPreferences

3 participants