Skip to content

Web Import: Add New Entries to 'Imported Entries' Group #12998

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

paudelritij
Copy link
Contributor

@paudelritij paudelritij commented Apr 25, 2025

Closes #12548
This PR introduce feature to add new group "Imported Entries"

  • Introduce Smart Group
  • Create configurable field for "Imported entries" group (under web search preferences)
  • Sync configuration with group-tree (for enable/disable or name-change of group)
  • Configure entries (web search / PDF import /web fetchers) to be auto-import
  • Add a few test cases
image image

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • 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.

@paudelritij paudelritij closed this May 4, 2025
@paudelritij paudelritij force-pushed the fix-for-issue-12548 branch from 2732800 to d9ea011 Compare May 4, 2025 04:20
@paudelritij paudelritij reopened this May 4, 2025
@@ -61,6 +62,10 @@ public static String getShortDescriptionAllEntriesGroup() {
return Localization.lang("<b>All Entries</b> (this group cannot be edited or removed)");
}

public static String getShortDescriptionSmartGroup(SmartGroup smartGroup) {
Copy link

Choose a reason for hiding this comment

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

The method getShortDescriptionSmartGroup returns a String, which could potentially be null. New public methods should not return null and should use java.util.Optional instead.

@@ -61,6 +62,10 @@ public static String getShortDescriptionAllEntriesGroup() {
return Localization.lang("<b>All Entries</b> (this group cannot be edited or removed)");
}

public static String getShortDescriptionSmartGroup(SmartGroup smartGroup) {
return Localization.lang("<b>Smart Group</b> (Import Entries)");
Copy link

Choose a reason for hiding this comment

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

The string 'Smart Group' should be in sentence case as per the guidelines, so it should be 'Smart group'.

@@ -17,6 +17,7 @@
import org.jabref.model.groups.GroupTreeNode;
import org.jabref.model.groups.KeywordGroup;
import org.jabref.model.groups.SearchGroup;
import org.jabref.model.groups.SmartGroup;
Copy link

Choose a reason for hiding this comment

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

The code uses Java SWING imports, which contradicts the instruction to use only JavaFX for UI technology. This could lead to inconsistencies in the UI framework used.

@@ -26,6 +27,19 @@ private static String serializeAllEntriesGroup() {
return MetadataSerializationConfiguration.ALL_ENTRIES_GROUP_ID;
}

private String serializeSmartGroup(SmartGroup group) {
Copy link

Choose a reason for hiding this comment

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

The method serializeSmartGroup is private and returns a String. If this method is intended to be used externally, it should return an Optional instead of null, following modern Java practices.

@@ -206,6 +210,24 @@ private static KeywordGroup keywordGroupFromString(String s, Character keywordSe
return newGroup;
}

private static SmartGroup smartGroupFromString(String input, Character keywordSeparator) throws ParseException {
if (!input.startsWith(MetadataSerializationConfiguration.SMART_GROUP_ID)) {
Copy link

Choose a reason for hiding this comment

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

Using exceptions for control flow is not recommended. Instead of throwing an IllegalArgumentException, consider using a different approach to handle this condition.

@@ -26,6 +27,19 @@ private static String serializeAllEntriesGroup() {
return MetadataSerializationConfiguration.ALL_ENTRIES_GROUP_ID;
}

private String serializeSmartGroup(SmartGroup group) {
Copy link

Choose a reason for hiding this comment

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

The method serializeSmartGroup is private and returns a String. According to the special instructions, new public methods should not return null and should use java.util.Optional. Consider making this method public and returning Optional.

@@ -206,6 +210,24 @@ private static KeywordGroup keywordGroupFromString(String s, Character keywordSe
return newGroup;
}

private static SmartGroup smartGroupFromString(String input, Character keywordSeparator) throws ParseException {
Copy link

Choose a reason for hiding this comment

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

The method 'smartGroupFromString' throws a ParseException for normal control flow when parsing context fails. Exceptions should be used for exceptional states, not for normal control flow.

@paudelritij paudelritij marked this pull request as ready for review May 7, 2025 16:17
this.addImportedEntries.set(addImportedEntries);
}

public String getAddImportedEntriesGroupName() {
Copy link

Choose a reason for hiding this comment

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

The method should return an Optional instead of a potentially null String to prevent null-related issues and align with modern Java practices.

@@ -206,6 +210,24 @@ private static KeywordGroup keywordGroupFromString(String s, Character keywordSe
return newGroup;
}

private static SmartGroup smartGroupFromString(String input, Character keywordSeparator) throws ParseException {
if (!input.startsWith(MetadataSerializationConfiguration.SMART_GROUP_ID)) {
throw new IllegalArgumentException("SmartGroup cannot be created from \"" + input + "\".");
Copy link

Choose a reason for hiding this comment

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

Avoid using exceptions for normal control flow. Consider using a different approach to handle this condition.

Copy link

trag-bot bot commented May 10, 2025

@trag-bot didn't find any issues in the code! ✅✨

@koppor koppor added component: groups status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: groups status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web import: New entry should be added to group "Imported entries"
2 participants