fix: resolve static analysis issues in code #13270

ungerts · 2025-06-06T11:42:16Z

Description

This pull request addresses various static analysis warnings to improve overall code quality and maintainability. The changes include:

Using string format placeholders ({}) in log messages instead of string concatenation
Correcting keyword import ordering for consistency
Refining the use of generic types for better type safety
Optimizing lambda expressions for readability and performance
Applying consistent code style across the modified files

These are non-functional changes, purely aimed at cleanup and improvement.

Steps to test

No functional behavior has changed, so no specific testing steps are required. However, reviewers may:

Run a full build and ensure all tests pass
Open and use the modified parts of the application to confirm there are no regressions
Use static analysis tools (e.g., CheckStyle, SpotBugs, IDE inspections) to confirm that previous warnings are now resolved

No UI changes have been made, so no screenshots are necessary.

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.

Fix various static analysis warnings including - Use string format placeholders in log messages - Fix keyword import ordering - Improve generic type usage - Optimize lambda expressions - Apply consistent code style These changes improve code quality and maintainability without changing functionality.

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

jabgui/src/main/java/org/jabref/gui/newentry/NewEntryViewModel.java

jabref-machine · 2025-06-06T14:09:39Z

Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of [x] (done), [ ] (not done yet) or [/] (not applicable).

calixtus · 2025-06-06T14:17:33Z

jabgui/src/main/java/org/jabref/gui/commonfxcontrols/SaveOrderConfigPanel.java

@@ -136,7 +135,7 @@ private List<Node> createRowButtons(SortCriterionViewModel criterionViewModel) {
    private void clearCriterionRow(int row) {
        List<Node> criterionRow = sortCriterionList.getChildren().stream()
                                                   .filter(item -> GridPane.getRowIndex(item) == row)
-                                                   .collect(Collectors.toList());
+                                                   .toList();


These to list changes must be checked in context. .toList() is not a drop in replacement, as if I recall correctly, Collectors.toList creates a mutable list and .toList an immutable one. Or am I wrong? @Siedlerchr

These changes are safe only if these are covered by tests, or it is manually ensured that the list is not being changed down the line

SaveOrder is a tricky one: See https://discourse.jabref.org/t/reasons-to-use-utf-8-in-bib-files-for-bibtex/5595/6?u=koppor

On a moderately-focused look I took last time, this instance was probably the only one that needed to be reverted.
There are about only 10 instances, so we can also revert them for safety and merge.

Yep, checked the rest - should not cause issues. reverted this one.

subhramit

Good to go from me

Edit: After a messy merge.

trag-bot · 2025-06-18T03:36:25Z

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

trag-bot · 2025-06-24T10:35:24Z

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

@@ -194,11 +194,11 @@ private Optional<Path> askForSavePath() {
            // Workaround for linux systems not adding file extension
            if (!savePath.getFileName().toString().toLowerCase().endsWith(".bib")) {
                savePath = Path.of(savePath.toString() + ".bib");
-                if (!Files.notExists(savePath)) {
-                    if (!dialogService.showConfirmationDialogAndWait(Localization.lang("Overwrite file"), Localization.lang("'%0' exists. Overwrite file?", savePath.getFileName()))) {
+                if (!Files.notExists(savePath) && !dialogService.showConfirmationDialogAndWait(Localization.lang("Overwrite file"),


Double negative 'notExists' makes the code harder to read and understand. Should use 'Files.exists()' instead for better readability and maintainability.

jabgui/src/main/java/org/jabref/gui/search/Highlighter.java

calixtus · 2025-06-25T13:10:37Z

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

+            Optional<Charset> newEncoding = dialogService.showChoiceDialogAndWait(Localization.lang("Save library"), Localization.lang("Select new encoding"),
+                    Localization.lang("Save library"), encoding, Encodings.getCharsets());


Convention is to put every argument in a single line.

calixtus · 2025-06-25T13:19:46Z

jabgui/src/main/java/org/jabref/gui/externalfiles/ChainedFilters.java

    private DirectoryStream.Filter<Path>[] filters;

+    @SafeVarargs
    public ChainedFilters(DirectoryStream.Filter<Path>... filters) {
        this.filters = filters;
    }


SafeVarArgs annotation is like SuppressWarnings. But ignoring warnings is imho not the best idea.
Can we refactor this to use proper list and do sthg like this?

for (DirectoryStream.Filter<Path> filter : filters) { this.filters.add(filter); }

Can we use List.of on this?
Can the method signature be refactored to use a List?

calixtus · 2025-06-25T13:35:15Z

jabgui/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

@@ -176,7 +176,7 @@ private void setupValidation() {
        keywordRegexValidator = new FunctionBasedValidator<>(
                keywordGroupSearchTermProperty,
                input -> {
-                    if (!keywordGroupRegexProperty.getValue()) {
+                    if (Boolean.FALSE.equals(keywordGroupRegexProperty.getValue())) {


keywordGroupRegexProperty.getValue() looks like it is returning a boxed version of ObservableBooleanValue#get which returns a native boolean. Or am i missing something? Is it maybe better to use get() instead

Yeah something like a .getValue().get

calixtus · 2025-06-25T13:37:07Z

jabgui/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

+        typeTexProperty.addListener((_, _, isSelected) -> {
+            if (Boolean.TRUE.equals(isSelected)) {


here the boxed version seems correct.

calixtus · 2025-06-25T13:46:46Z

jabgui/src/main/java/org/jabref/gui/texparser/ParseLatexDialogViewModel.java

@@ -162,7 +162,7 @@ private FileNodeViewModel searchDirectory(Path directory) throws IOException {
        try (Stream<Path> filesStream = Files.list(directory)) {
            fileListPartition = filesStream.collect(Collectors.partitioningBy(path -> path.toFile().isDirectory()));
        } catch (IOException e) {
-            LOGGER.error("%s while searching files: %s".formatted(e.getClass().getName(), e.getMessage()), e);
+            LOGGER.error("{} while searching files:", e.getClass().getName(), e);


Is the name of the error even neccessary? Wouldn't it suffice to just put "Error searching file." and let the logger handle the exception e?

calixtus · 2025-06-25T14:07:56Z

jablib/src/main/java/org/jabref/logic/citationkeypattern/AbstractCitationKeyPatterns.java

-        sb.append(", data=").append(data);
-        sb.append('}');
-        return sb.toString();
+        String sb = "AbstractCitationKeyPattern{defaultPattern=" + defaultPattern + ", data=" + data + '}';


Maybe with java textblock and linebreaks easier to read?

calixtus · 2025-06-25T14:30:25Z

jablib/src/main/java/org/jabref/logic/l10n/Encodings.java

This whole class is superfluous and can be removed imho.
ENCODINGS and ENCODINGS_DISPLAYNAMES are only used in tests, which can be removed too. Especially these. Not null and size not 0. Useless testing of standard jdk methods.
Im not even sure that distinct() is neccessary on available charsets. But that can either be inlined or moved to another class, maybe OS or sthg. Wdyt @Siedlerchr @koppor ?

calixtus · 2025-06-25T14:31:26Z

jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java

@@ -447,8 +447,9 @@ public class JabRefCliPreferences implements CliPreferences {
     */
    public JabRefCliPreferences() {
        try {
-            if (Files.exists(Path.of("jabref.xml"))) {
-                importPreferences(Path.of("jabref.xml"));
+            Path preferebcesPath = Path.of("jabref.xml");


Suggested change

Path preferebcesPath = Path.of("jabref.xml");

Path preferencesPath = Path.of("jabref.xml");

calixtus · 2025-06-25T14:37:11Z

jablib/src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java

                case "." -> {
-                    continue;  // Stay in current directory
-                }
-                case ".." -> {
-                    currentDirectory = currentDirectory.getParent();
-                    continue;
+                    // Stay in current directory
                }
+                case ".." -> currentDirectory = currentDirectory.getParent();
                case "*" -> { // for all direct subdirs


May be functionally correct, but I think it was more easily understandable before.
Now you have three cases with a statement block and one in the middle without.
Before you know, with continue, do nothing else, next dir, now you look for the end of the swith an look, if there is any code.

calixtus · 2025-06-25T14:39:22Z

jablib/src/main/java/org/jabref/model/entry/identifier/DOI.java


    // Regex
    // (see http://www.doi.org/doi_handbook/2_Numbering.html)
-    private static final String DOI_EXP = ""
-            + "(?:urn:)?"                       // optional urn
+    private static final String DOI_EXP = "(?:urn:)?"                       // optional urn
            + "(?:doi:)?"                       // optional doi
            + "("                               // begin group \1
            + "10"                              // directory indicator
            + "(?:\\.[0-9]+)+"                  // registrant codes
            + "[/:%]"                           // divider
            + "(?:.+)"                          // suffix alphanumeric string
            + ")";                              // end group \1
-    private static final String FIND_DOI_EXP = ""
-            + "(?:urn:)?"                       // optional urn
+    private static final String FIND_DOI_EXP = "(?:urn:)?"                       // optional urn
            + "(?:doi:)?"                       // optional doi
            + "("                               // begin group \1
            + "10"                              // directory indicator


Please adapt all the tabs to keep the comments in one column

Siedlerchr · 2025-06-25T14:48:55Z

jabgui/src/main/java/org/jabref/gui/externalfiles/UnlinkedFilesCrawler.java

@@ -91,7 +92,8 @@ FileNodeViewModel searchDirectory(Path directory, UnlinkedPDFFileFilter unlinked
        //   2. GitIgnoreFilter
        ChainedFilters filters = new ChainedFilters(unlinkedPDFFileFilter, new GitIgnoreFileFilter(directory));
        Map<Boolean, List<Path>> directoryAndFilePartition;
-        try (Stream<Path> filesStream = StreamSupport.stream(Files.newDirectoryStream(directory, filters).spliterator(), false)) {


Why is it split in two ?

Siedlerchr · 2025-06-25T14:49:23Z

jabgui/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

@@ -164,7 +164,7 @@ private void setupValidation() {
                        }

                        // Edit group, changed name to something that is already present
-                        return (editedGroup == null) || editedGroup.getName().equals(name) || (groupsWithSameName <= 0);
+                        return (editedGroup == null) || editedGroup.getName().equals(name) || (groupsWithSameName == 0);


This changes semantics!

trag-bot bot reviewed Jun 6, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java Show resolved Hide resolved

fix: resolve checkstyle violations and improve code quality

0ff44ac

Siedlerchr reviewed Jun 6, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/newentry/NewEntryViewModel.java Outdated Show resolved Hide resolved

fix: resolve issues in LocalizationConsistencyCheck

b844e0f

This comment was marked as off-topic.

Sign in to view

calixtus reviewed Jun 6, 2025

View reviewed changes

subhramit added 4 commits June 18, 2025 08:01

Revert, misc

ff17d38

More underscore

4d80b22

Fix raw use of list

b6946a1

Use peek

f37e553

subhramit previously approved these changes Jun 18, 2025

View reviewed changes

Merge branch 'main' into feature/static-analysis-fixes

a4e3b92

subhramit dismissed their stale review via a4e3b92 June 18, 2025 03:07

subhramit previously approved these changes Jun 18, 2025

View reviewed changes

Missing import

521164a

subhramit dismissed their stale review via 521164a June 18, 2025 03:10

subhramit previously approved these changes Jun 18, 2025

View reviewed changes

Lost commit

0ef1159

subhramit dismissed their stale review via 0ef1159 June 18, 2025 03:20

Overlap error code changes with changes of this branch

6681865

subhramit approved these changes Jun 18, 2025

View reviewed changes

koppor marked this pull request as ready for review June 24, 2025 10:27

trag-bot bot reviewed Jun 24, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/search/Highlighter.java Show resolved Hide resolved

subhramit added dev: code-quality Issues related to code or architecture decisions status: awaiting-second-review For non-trivial changes labels Jun 24, 2025

calixtus requested changes Jun 25, 2025

View reviewed changes

Siedlerchr reviewed Jun 25, 2025

View reviewed changes

subhramit added status: changes-required Pull requests that are not yet complete and removed status: awaiting-second-review For non-trivial changes labels Jun 28, 2025

		Optional<Charset> newEncoding = dialogService.showChoiceDialogAndWait(Localization.lang("Save library"), Localization.lang("Select new encoding"),
		Localization.lang("Save library"), encoding, Encodings.getCharsets());

		typeTexProperty.addListener((_, _, isSelected) -> {
		if (Boolean.TRUE.equals(isSelected)) {

	Path preferebcesPath = Path.of("jabref.xml");
	Path preferencesPath = Path.of("jabref.xml");

Uh oh!

fix: resolve static analysis issues in code #13270

Are you sure you want to change the base?

fix: resolve static analysis issues in code #13270

Uh oh!

Conversation

ungerts commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to test

Mandatory checks

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

jabref-machine commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trag-bot bot commented Jun 18, 2025

Uh oh!

trag-bot bot Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ungerts commented Jun 6, 2025 •

edited

Loading

subhramit Jun 6, 2025 •

edited

Loading

subhramit Jun 18, 2025 •

edited

Loading

subhramit left a comment •

edited

Loading