Skip to content

Polishing the database exploration dialog #5376 - #8353

Open
nadment wants to merge 1 commit into
apache:mainfrom
nadment:5376
Open

nadment wants to merge 1 commit into
apache:mainfrom
nadment:5376

Conversation

@nadment

@nadment nadment commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Add a search filter that optionally supports wildcards.
Add icon display based on the database type.
Move the Refresh action to the toolbar.
Remove the large buttons and keep only the context menu and toolbar
Do not include the connection name in the title (to avoid having to save/restore the dialog box dimensions for each connection)
Expand/Collapse by double-clicking
Fix SqlEditor

@nadment nadment linked an issue Sep 13, 2026 that may be closed by this pull request

@mattcasters mattcasters left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @nadment,

Thank you for tackling this! The database explorer dialog was indeed in need of a refresh, and this PR brings some great improvements:

  • Full-width tree layout: Removing the vertical button column makes the tree so much easier to navigate, especially with long schema and table names.
  • Dynamic database icons: Showing the dialect-specific database icon and distinct icons for Views and Synonyms is a really nice visual upgrade.
  • Search filter: The debounced search filter with wildcard/regex support is fantastic for large databases.
  • Shared tree model: Extracting DatabaseTreeNode and DatabaseTreeUtil into org.apache.hop.ui.core.database keeps things consistent with the Database perspective.
  • Double-click behavior: Expanding/collapsing containers and confirming/previewing tables on double click feels natural.

While reviewing the implementation, I spotted several critical bugs and a few areas where we can polish the UX and maintainability:


🚨 Critical Issues & Regressions

1. Cancel / Escape returns true when a table was pre-selected

In DatabaseExplorerDialog:

public boolean open() {
  ...
  BaseDialog.defaultShellHandling(shell, c -> ok(), c -> cancel());
  return selectedTableName != null;
}

Almost all callers (e.g. TableOutputDialog, InsertUpdateDialog, etc.) pre-populate the dialog before opening:

std.setSelectedSchemaAndTable(wSchema.getText(), wTable.getText());
if (std.open()) { ... }

When cancel() is called:

public void cancel() {
  dispose();
}

selectedTableName is never cleared! So if the user hits Cancel, presses Escape, or closes the shell with (X), open() returns true, causing caller dialogs to treat it as an OK confirmation.
Suggested Fix: Keep the pre-selection input distinct from the dialog result, or use a boolean okPressed = false; flag set only in ok() and return okPressed && tableName != null.


2. updateTreeCatalogs() always returns false (duplicate folders created)

In DatabaseExplorerDialog:

private boolean updateTreeCatalogs() {
  Catalog[] catalogs = databaseMetaInformation.getCatalogs();
  if (catalogs != null && catalogs.length > 0) {
    ...
  }
  return false; // Always returns false
}

In updateTree():

if (!updateTreeSchemas()) {
  if (!updateTreeCatalogs()) {
    addFolder(connectionItem, connectionName, STRING_TABLES, ...);
    addFolder(connectionItem, connectionName, STRING_VIEWS, ...);
    addFolder(connectionItem, connectionName, STRING_SYNONYMS, ...);
  }
}

Because updateTreeCatalogs() returns false, for databases with catalogs (e.g., MySQL, Snowflake, SQL Server), it populates the catalogs and then also adds the flat Tables, Views, and Synonyms folders.
Suggested Fix: Return catalogs != null && catalogs.length > 0;. Also, check whether the catalog or its children match the filter (catalogOrChildMatches) so empty/non-matching catalogs are filtered out like schemas are.


3. Unsafe selection access in openTreeItem()

public void openTreeItem(Event e) {
  TreeItem item = wTree.getSelection()[0];
  DatabaseTreeNode node = (DatabaseTreeNode) item.getData();
  ...

If wTree.getSelection() is empty (e.g. keyboard navigation / focus shifts triggering DefaultSelection), this throws ArrayIndexOutOfBoundsException. Also, if item.getData() is null, casting causes an NPE or ClassCastException.
Suggested Fix: Reuse getSelectedNode() or safely check selection.length == 1 and data instanceof DatabaseTreeNode.


4. Filter matches connectionName, preventing filtering

In matchesFilter:

private boolean matchesFilter(String name, String schemaName, String connectionName) {
  if (filter == null) {
    return true;
  }
  return filter.matches(name) || filter.matches(schemaName) || filter.matches(connectionName);
}

Unlike DatabaseWorkbench (which contains multiple connections), DatabaseExplorerDialog is scoped to a single connection. If the user types any search substring that happens to appear in the connection name (e.g. searching "pg" when the connection is "pg_prod", or "test" on "test_db"), filter.matches(connectionName) evaluates to true for every table in the database, so no filtering occurs.
Suggested Fix: Remove || filter.matches(connectionName).


5. Native OS Menu handle leak on right-click

In setTreeMenu():

public void setTreeMenu() {
  DatabaseTreeNode node = getSelectedNode();
  if (node != null) {
    Menu mTree = new Menu(shell, SWT.POP_UP);
    if (node.isTableLike()) {
      ...
      wTree.setMenu(mTree);
    }
  } else {
    wTree.setMenu(null);
  }
}
  1. A new Menu is allocated on every right-click, but the previous wTree.getMenu() is never disposed, leaking native OS handles.
  2. If node != null but node.isTableLike() is false (e.g. right-clicking a Schema or Folder), mTree is allocated and abandoned without being set or disposed, and wTree retains its previous menu (showing table actions like Preview/Truncate for a schema).
    Suggested Fix: Dispose any existing menu on wTree before creating a new one, and only allocate when node != null && node.isTableLike().

6. SqlEditor progress monitor lifecycle and threading

  1. monitor.done() was removed from the finally block of runSqlScriptWithMonitor. If db.connect() or statement prep fails, monitor.done() is never reached, leaving the progress dialog hanging.
  2. In runScriptStatementsLoop / executeSelectStatement, monitor.done() is called inside the statement loop for query statements and then called again after the loop.
  3. EnterTextDialog is opened inside runScriptStatementsLoop via syncExec from the background worker thread while pmd.run(true, ...) is waiting on the UI thread.
    Suggested Fix: Keep background tasks strictly on background threads: let pmd.run() complete, ensure monitor.done() is in the finally block, and open any dialogs (ShowRowsDialog, EnterTextDialog) on the UI thread after pmd.run() finishes.

💡 Suggestions & UI Polish

  1. Toolbar Tooltip missing i18n:::
    In refresh():

    toolTip = "System.Button.Refresh"

    Needs the i18n:: prefix ("i18n::System.Button.Refresh"), otherwise GuiToolbarWidgets displays the literal raw key.

  2. Dialog title vs. size persistence:
    The connection name was removed from the title so WindowProperty would share dimensions. However, not seeing which connection is currently open hurts UX. We can keep the connection name in shell.setText(...) while explicitly passing a fixed ID to WindowProperty / props.setScreen(...) (e.g. new WindowProperty("DatabaseExplorerDialog", ...)).

  3. Search box keyboard navigation:

    • Hitting Enter in wSearch currently does nothing because BaseDialog.NO_DEFAULT_HANDLER is set. Adding wSearch.addListener(SWT.DefaultSelection, e -> updateTree()); allows immediate filtering without waiting for the 250ms debounce.
    • At the end of updateTree(), wSearch.setFocus() is called. If the user is navigating the tree with arrow keys, toggling regex, or pressing Refresh, focus gets yanked back to the search box. It would be better to only focus wSearch on dialog open.
  4. Action discoverability for non-right-click users:
    With all 8 right-side buttons removed, actions like Preview, Layout, DDL, and SQL are only discoverable via right-click. Adding toolbar buttons for the primary table actions (Preview, Layout, SQL Editor), enabled when a table node is selected (like in DatabaseWorkbench), would make them easily discoverable.

  5. Layout margin safety:
    wTree top is attached to toolBar, while wSearch is on the left. If wSearch is taller than the toolbar on some OS themes, wTree could overlap the bottom edge of wSearch. Anchoring wTree below a top header composite or below the taller control avoids any overlap.

  6. Deduplicating namesForSchema in DatabaseTreeUtil:
    namesForSchema is currently duplicated in both DatabaseWorkbench and DatabaseExplorerDialog. Moving it to DatabaseTreeUtil keeps it DRY. (Also, if schemaName != null but not found, falling through to map.get("") or map.get(null) can cause schema-less views to be displayed under unrelated schemas).

  7. Minor typos:

    • messages_pt_BR.properties: DatabaseExplorerDialog.Menu.OpenSQL=Abrir SQS -> should be Abrir SQL.
    • DatabaseExplorerDialog.java: BaseMessages.getString(PKG, "DatabaseExplorerDialog.Menu.Truncate", table) passes table, but {0} was removed from messages_*.properties.

@mattcasters

Copy link
Copy Markdown
Contributor

Besides the technical glitches I really love the new compact look. When filtering it would be cool to automatically expand the found table(s) to make it visible. I would also add a toolbar above the tree with the same functionality as the right click menu.

@nadment
nadment force-pushed the 5376 branch 3 times, most recently from c9248f2 to 13800e4 Compare September 14, 2026 21:59
@nadment

nadment commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review; I think I've corrected all the issues.

However, I didn't understand the comment below:
When filtering, it would be nice if the table(s) found were displayed automatically so we could see them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task]: Polishing the database exploration dialog

2 participants