Update KeywordEditor to work with escaping

[BOgdAnSAM-sudo]

Closes #14780
Follow up for #14637

Update KeywordEditor to work with escaping logic throught jablib/src/main/java/org/jabref/model/entry/KeywordList#Parse and jablib/src/main/java/org/jabref/model/entry/KeywordList#Serialize

Steps to test

1. Start JabRef
2. Create new empty library
3. Click on "Add example entry"
4. In example entry locate tab "General"
5. In Keywords field enter paste following keywords: "Keyword \> Keyword", "Keyword > Keyword", "KeywordOne\,KeywordTwo", "KeywordOne,KeywordTwo", "keyword\", "keyword".
6. Keywords field should show: "Keyword > Keyword" and "Keyword > Keyword" as two separate keywords, "KeywordOne,KeywordTwo" as single keyword, "KeywordOne" and "KeywordTwo" as two separate keywords, "KeywordOne", "KeywordOne".
7. Locate tab "BibTeX sorce"
8. Tab "BibTeX sorce" should contain line keywords = {Keyword > Keyword,Keyword > Keyword,KeywordOne,KeywordTwo, KeywordOne,KeywordTwo,keyword\,keyword}

This should work the same way both when pasting and entering manualy. Also, when using suggestions, suggested text should be added as tag and serialized version saved to "BibTeX sorce".

Proposed documntation changes JabRef/user-documentation#615

Checklist

• 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 (if applicable)
• [/] I added screenshots in the PR description (if change is visible to the user)
• I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
• I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
• I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

[qodo-free-for-open-source-projects]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Unclear variable name: Variable substringWithSeparator is misleading as it represents a reversed substring before the separator, not one containing the separator Referred Code String substringWithSeparator = new StringBuilder(keywordString.substring(0, separatorFirstOccurrence)).reverse().toString(); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing boundary validation: Method isSeparatedKeyword does not handle the case where separatorFirstOccurrence is -1 (separator not found), which would cause substring to fail Referred Code int separatorFirstOccurrence = keywordString.lastIndexOf(keywordSeparator); String substringWithSeparator = new StringBuilder(keywordString.substring(0, separatorFirstOccurrence)).reverse().toString(); Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Improve method robustness and readability Suggestion Impact: The commit implements the first part of the suggestion by adding the guard clause to check if separatorLastOccurrence is -1. It also renames the variable from separatorFirstOccurrence to separatorLastOccurrence as suggested. However, the commit does not include the full refactoring of the stream-based logic to a simpler loop. code diff: - if (keywordString.isEmpty()) { + int separatorLastOccurrence = keywordString.lastIndexOf(keywordSeparator); + if (separatorLastOccurrence == -1) { return false; Refactor the isSeparatedKeyword method to prevent a potential StringIndexOutOfBoundsException and improve its readability. Replace the complex stream-based logic with a simpler loop and add a guard clause to handle cases where the separator is not found. jabgui/src/main/java/org/jabref/gui/fieldeditors/KeywordsEditor.java [164-177] private boolean isSeparatedKeyword(String keywordString, String keywordSeparator) { if (keywordString.isEmpty()) { return false; } - int separatorFirstOccurrence = keywordString.lastIndexOf(keywordSeparator); - String substringWithSeparator = new StringBuilder(keywordString.substring(0, separatorFirstOccurrence)).reverse().toString(); + int separatorLastOccurrence = keywordString.lastIndexOf(keywordSeparator); + if (separatorLastOccurrence == -1) { + return false; + } - AtomicBoolean isSeparatedKeyword = new AtomicBoolean(true); - substringWithSeparator.chars().takeWhile(symbol -> symbol == Keyword.DEFAULT_ESCAPE_SYMBOL) - .forEachOrdered(_ -> isSeparatedKeyword.set(!isSeparatedKeyword.get())); - - return isSeparatedKeyword.get(); + String prefix = keywordString.substring(0, separatorLastOccurrence); + int escapeCharCount = 0; + for (int i = prefix.length() - 1; i >= 0; i--) { + if (prefix.charAt(i) == Keyword.DEFAULT_ESCAPE_SYMBOL) { + escapeCharCount++; + } else { + break; + } + } + return escapeCharCount % 2 == 0; } [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential StringIndexOutOfBoundsException and a significant readability issue in the new isSeparatedKeyword method. The proposed refactoring makes the code safer, more efficient, and much easier to understand.	Medium
	✅ Fix inconsistent keyword conversion logic Suggestion Impact: The commit implements the exact changes suggested: adds null check to the isEmpty condition, removes the inconsistent empty keyword creation logic, and replaces the manual list checking with a cleaner stream().findFirst().orElse(null) pattern. code diff: - if (keywordString.isEmpty()) { + if (keywordString == null || keywordString.isEmpty()) { return null; } - KeywordList parsedKeywords = KeywordList.parse(keywordString, keywordSeparator); - Keyword resultKeyword = null; - if (parsedKeywords.isEmpty()) { - return new Keyword(""); - } else { - resultKeyword = parsedKeywords.get(0); - } - return resultKeyword; + return KeywordList.parse(keywordString, keywordSeparator) + .stream() + .findFirst() + .orElse(null); Simplify and fix inconsistent logic in the fromString method of the StringConverter. The current implementation handles empty strings and strings that parse to an empty list differently, which can lead to undesired empty tags. jabgui/src/main/java/org/jabref/gui/fieldeditors/KeywordsEditorViewModel.java [74-88] @Override public Keyword fromString(String keywordString) { - if (keywordString.isEmpty()) { + if (keywordString == null || keywordString.isEmpty()) { return null; } - KeywordList parsedKeywords = KeywordList.parse(keywordString, keywordSeparator); - Keyword resultKeyword = null; - if (parsedKeywords.isEmpty()) { - return new Keyword(""); - } else { - resultKeyword = parsedKeywords.get(0); - } - return resultKeyword; + return KeywordList.parse(keywordString, keywordSeparator) + .stream() + .findFirst() + .orElse(null); } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies inconsistent behavior in the fromString method, where different empty-like inputs produce different results (null vs. new Keyword("")). The proposed change simplifies the logic and makes the behavior more consistent, preventing the creation of unwanted empty keywords.	Low
Update

[calixtus]

Why dont you want to keep the converter static and reusable?

[BOgdAnSAM-sudo]

Why dont you want to keep the converter static and reusable?

Because the converter needs access to the object's data to convert the strings.

[calixtus]

Did it occur to you, that a keyword is different from a keywordlist?

[BOgdAnSAM-sudo]

Did it occur to you, that a keyword is different from a keywordlist?

What do you mean by that?

[calixtus]

The String converter for keywords is not meant to be used to parse a list of keywords, hoping that I ly the first one is the one you are looking for. Only clear defined keywords as strings to be converted are expected. Parsing for shg else must be done beforehand.

[calixtus]

That's why the String converter was static. To keep the code clean.

[BOgdAnSAM-sudo]

To create a Keyword from a string, the string must be parsed first. However, the logic in KeywordList#parse already returns a KeywordList populated with created Keywords. If we introduce a separate method to parse a single Keyword while keeping StringConverter static, the fromString method becomes redundant. This approach seems overcomplicated and leads to unused code.

[github-actions]

Your pull request conflicts with the target branch.

Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line.

[BOgdAnSAM-sudo]

That's why the String converter was static. To keep the code clean.

I understand your concern, but I don't see how to achieve this. I spent some time looking at the code and couldn't find a solution other than making the String converter non static. I would really appreciate it if you could share your thoughts on this.

[calixtus]

Will do in a few days. Busy with day job.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: b3ed3a7
▶️ Tests: 11198 executed
⚪️ Checks: 51/51 completed

---

Learn more about TestLens at testlens.app.

[calixtus]

Looks good, thanks for coming back for this one.