Skip to content

Conversation

@TimeTriggerx
Copy link
Contributor

@TimeTriggerx TimeTriggerx commented Jan 7, 2026

User description

Closes #14556

The "Search Engine URL Templates" table in Preferences > Web Search was consuming excessive vertical space due to a VBox.vgrow="ALWAYS" constraint on the TableView, creating a large empty gap in the UI.

Changes:

  • Replaced the heavy TableView control with a lightweight VBox container.
  • Removed the "Press return to commit" label as it is no longer needed for standard text fields.

Screenshots

Before:

img

After (Fix):

img

Mandatory checks


PR Type

Bug fix, Enhancement


Description

  • Replace TableView with dynamic VBox layout for search engines

  • Remove excessive vertical space in Web Search preferences UI

  • Simplify search engine display with label and text field

  • Remove obsolete localization keys and "Press return" hint


Diagram Walkthrough

flowchart LR
  A["TableView with vgrow ALWAYS"] -- "Replace with" --> B["Dynamic VBox layout"]
  B -- "Render each engine as" --> C["HBox with Label and TextField"]
  D["Remove TableColumn setup"] -- "Simplify to" --> E["Direct property binding"]
  F["Obsolete localization keys"] -- "Delete" --> G["Clean properties file"]
Loading

File Walkthrough

Relevant files
Enhancement
WebSearchTab.java
Replace TableView with dynamic VBox rendering                       

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java

  • Removed TableView and TableColumn imports and FXML bindings
  • Replaced table initialization with dynamic VBox-based rendering
  • Added createSearchEngineNode() method to build HBox containers with
    Label and TextField
  • Implemented InvalidationListener to update search engine list on
    changes
  • Simplified search engine display from editable table cells to bound
    text fields
+30/-15 
WebSearchTab.fxml
Simplify FXML layout from table to VBox                                   

jabgui/src/main/resources/org/jabref/gui/preferences/websearch/WebSearchTab.fxml

  • Removed TableView and TableColumn XML imports
  • Replaced entire TableView element with simple VBox container
  • Removed "Press return to commit" instructional label
  • Removed table column definitions and resize policy configuration
  • Added searchEngineContainer VBox with 10px spacing
+1/-22   
Documentation
JabRef_en.properties
Remove obsolete table column localization keys                     

jablib/src/main/resources/l10n/JabRef_en.properties

  • Removed localization key "Search Engine"
  • Removed localization key "URL Template"
  • Kept "Search Engine URL Templates" section header
+0/-2     

Replaced the large TableView with a dynamic VBox layout to remove unused vertical space.
@TimeTriggerx TimeTriggerx changed the title FIX #14556 Fix: Optimize UI layout for Web Search preferences #14556 Jan 7, 2026
@github-actions github-actions bot added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Jan 7, 2026
@qodo-free-for-open-source-projects
Copy link
Contributor

qodo-free-for-open-source-projects bot commented Jan 7, 2026

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: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

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: 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:
Missing Input Validation: The urlField TextField at line 187 binds directly to item.urlTemplateProperty() without
visible validation or sanitization of URL input.

Referred Code
urlField.textProperty().bindBidirectional(item.urlTemplateProperty());
HBox.setHgrow(urlField, Priority.ALWAYS); // Grow to fill space

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

qodo-free-for-open-source-projects bot commented Jan 7, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent memory leak with weak listener

To prevent a potential memory leak, wrap the searchEngineListener in a
WeakInvalidationListener before adding it to viewModel.getSearchEngines(). This
allows the WebSearchTab to be garbage collected properly.

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java [81-91]

 // Bind search engine list to the UI container
 InvalidationListener searchEngineListener = _ -> searchEngineContainer
         .getChildren()
         .setAll(viewModel.getSearchEngines()
                          .stream()
                          .map(this::createSearchEngineNode)
                          .toList());
 
 // Initialize the list and add listener for future updates
 searchEngineListener.invalidated(null);
-viewModel.getSearchEngines().addListener(searchEngineListener);
+viewModel.getSearchEngines().addListener(new javafx.beans.WeakInvalidationListener(searchEngineListener));
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a potential memory leak caused by a long-lived view model holding a strong reference to a listener in a shorter-lived UI component. Using a WeakInvalidationListener is the standard and correct way to prevent this in JavaFX, making this a critical fix for application stability.

High
Learned
best practice
Extract lambda to method reference

Replace the lambda expression with a method reference for better readability and
conciseness. Extract the logic into a separate method and use a method
reference.

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java [82-87]

-InvalidationListener searchEngineListener = _ -> searchEngineContainer
+InvalidationListener searchEngineListener = _ -> updateSearchEngineContainer();
+
+private void updateSearchEngineContainer() {
+    searchEngineContainer
         .getChildren()
         .setAll(viewModel.getSearchEngines()
                          .stream()
                          .map(this::createSearchEngineNode)
                          .toList());
+}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Use method references instead of lambda expressions when the lambda only calls a single method with the same parameters

Low
  • Update

@github-actions github-actions bot added the status: changes-required Pull requests that are not yet complete label Jan 7, 2026
@TimeTriggerx TimeTriggerx changed the title Fix: Optimize UI layout for Web Search preferences #14556 Fix: Optimize UI layout for Web Search preferences Jan 7, 2026
@koppor
Copy link
Member

koppor commented Jan 7, 2026

  • No screenshot before
  • Headings are lost (which is IMHO OK, BUT ADAPT THE LOCALIZATON AS THE TEST SAYS - **why is there no reaction on the the failing test since 12 hours?)

@TimeTriggerx TimeTriggerx marked this pull request as draft January 7, 2026 15:49
@github-actions github-actions bot removed the status: changes-required Pull requests that are not yet complete label Jan 7, 2026
@TimeTriggerx
Copy link
Contributor Author

Apologies for the delay @koppor ! I missed the notification earlier. Now fault is mended and order restored.

@TimeTriggerx TimeTriggerx marked this pull request as ready for review January 7, 2026 17:32
@qodo-free-for-open-source-projects
Copy link
Contributor

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: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

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: 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

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

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refactor UI update logic for clarity

Refactor the UI update logic by extracting it from the InvalidationListener into
a separate, clearly-named method. This improves readability by avoiding the
unconventional listener.invalidated(null) call for initialization.

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java [81-91]

-// Bind search engine list to the UI container
-InvalidationListener searchEngineListener = _ -> searchEngineContainer
-        .getChildren()
-        .setAll(viewModel.getSearchEngines()
-                         .stream()
-                         .map(this::createSearchEngineNode)
-                         .toList());
+// Create and bind the search engine list to the UI
+viewModel.getSearchEngines().addListener((InvalidationListener) _ -> updateSearchEngineList());
+updateSearchEngineList();
 
-// Initialize the list and add listener for future updates
-searchEngineListener.invalidated(null);
-viewModel.getSearchEngines().addListener(searchEngineListener);
-
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that calling invalidated(null) is an unconventional use of a listener. Extracting the logic into a named method improves code clarity and maintainability, making the developer's intent more explicit.

Low
  • More

@TimeTriggerx TimeTriggerx marked this pull request as draft January 8, 2026 01:43
@TimeTriggerx TimeTriggerx marked this pull request as ready for review January 8, 2026 02:28
@calixtus
Copy link
Member

calixtus commented Jan 8, 2026

A vertical gap usually 10 should be visible between controls

@TimeTriggerx TimeTriggerx marked this pull request as draft January 8, 2026 17:14
@TimeTriggerx TimeTriggerx marked this pull request as ready for review January 8, 2026 17:24
@qodo-free-for-open-source-projects
Copy link
Contributor

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: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

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: 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:
Missing URL validation: The TextField for URL template is bound bidirectionally without visible input validation
or sanitization in the displayed code

Referred Code
TextField urlField = new TextField();
urlField.textProperty().bindBidirectional(item.urlTemplateProperty());
HBox.setHgrow(urlField, Priority.ALWAYS); // Grow to fill space

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refactor UI update logic for clarity

Refactor the UI update logic to use a Runnable for better clarity. This involves
defining the update logic in a Runnable, calling it directly for the initial
setup, and then using it within the listener for subsequent updates.

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java [81-91]

-// Bind search engine list to the UI container
-InvalidationListener searchEngineListener = _ -> searchEngineContainer
+// Define the update logic
+Runnable updateSearchEnginesView = () -> searchEngineContainer
         .getChildren()
         .setAll(viewModel.getSearchEngines()
                          .stream()
                          .map(this::createSearchEngineNode)
                          .toList());
 
-// Initialize the list and add listener for future updates
-searchEngineListener.invalidated(null);
-viewModel.getSearchEngines().addListener(searchEngineListener);
+// Initial population
+updateSearchEnginesView.run();
 
+// Add listener for future updates
+viewModel.getSearchEngines().addListener(obs -> updateSearchEnginesView.run());
+
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies an unconventional use of invalidated(null) to trigger an action and proposes a clearer, more standard pattern using a Runnable, which improves code readability and maintainability.

Low
  • More

@TimeTriggerx TimeTriggerx marked this pull request as draft January 17, 2026 11:14
@TimeTriggerx TimeTriggerx marked this pull request as ready for review January 17, 2026 11:15
@qodo-free-for-open-source-projects
Copy link
Contributor

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: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing null check: The createSearchEngineNode() method does not validate if item parameter is null before
accessing its properties.

Referred Code
private Node createSearchEngineNode(SearchEngineItem item) {
    HBox container = new HBox(10);
    container.setAlignment(Pos.CENTER_LEFT);

    // 1. Label for the Name (e.g., "Google Scholar")
    Label nameLabel = new Label();
    nameLabel.textProperty().bind(item.nameProperty());
    nameLabel.setMinWidth(120); // Keep alignment consistent
    nameLabel.setPrefWidth(120);

    // 2. TextField for the URL
    TextField urlField = new TextField();
    urlField.textProperty().bindBidirectional(item.urlTemplateProperty());
    HBox.setHgrow(urlField, Priority.ALWAYS); // Grow to fill space

    container.getChildren().addAll(nameLabel, urlField);
    return container;
}

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:
No URL validation: The urlField bound to item.urlTemplateProperty() lacks input validation to ensure URLs are
properly formatted and safe.

Referred Code
TextField urlField = new TextField();
urlField.textProperty().bindBidirectional(item.urlTemplateProperty());
HBox.setHgrow(urlField, Priority.ALWAYS); // Grow to fill space

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Unbind properties to prevent memory leaks

To prevent a memory leak, unbind the nameLabel and urlField properties when
their parent node is removed from the scene by adding a listener to the
container's sceneProperty.

jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java [175-192]

 private Node createSearchEngineNode(SearchEngineItem item) {
     HBox container = new HBox(10);
     container.setAlignment(Pos.CENTER_LEFT);
 
     // 1. Label for the Name (e.g., "Google Scholar")
     Label nameLabel = new Label();
     nameLabel.textProperty().bind(item.nameProperty());
     nameLabel.setMinWidth(120); // Keep alignment consistent
     nameLabel.setPrefWidth(120);
 
     // 2. TextField for the URL
     TextField urlField = new TextField();
     urlField.textProperty().bindBidirectional(item.urlTemplateProperty());
     HBox.setHgrow(urlField, Priority.ALWAYS); // Grow to fill space
 
     container.getChildren().addAll(nameLabel, urlField);
+
+    // Unbind properties when the node is removed from the scene to prevent memory leaks
+    container.sceneProperty().addListener((obs, oldScene, newScene) -> {
+        if (newScene == null) {
+            nameLabel.textProperty().unbind();
+            urlField.textProperty().unbindBidirectional(item.urlTemplateProperty());
+        }
+    });
+
     return container;
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a memory leak introduced by the new dynamic UI creation, where old UI nodes are not garbage collected because their property bindings are not removed. This is a critical issue that affects application stability.

High
  • More

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good first issue An issue intended for project-newcomers. Varies in difficulty. Review effort 2/5 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.

Fix UI for configuring custom URLs

3 participants