Conversation
mattcasters
left a comment
There was a problem hiding this comment.
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
DatabaseTreeNodeandDatabaseTreeUtilintoorg.apache.hop.ui.core.databasekeeps 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);
}
}- A new
Menuis allocated on every right-click, but the previouswTree.getMenu()is never disposed, leaking native OS handles. - If
node != nullbutnode.isTableLike()isfalse(e.g. right-clicking a Schema or Folder),mTreeis allocated and abandoned without being set or disposed, andwTreeretains its previous menu (showing table actions like Preview/Truncate for a schema).
Suggested Fix: Dispose any existing menu onwTreebefore creating a new one, and only allocate whennode != null && node.isTableLike().
6. SqlEditor progress monitor lifecycle and threading
monitor.done()was removed from thefinallyblock ofrunSqlScriptWithMonitor. Ifdb.connect()or statement prep fails,monitor.done()is never reached, leaving the progress dialog hanging.- In
runScriptStatementsLoop/executeSelectStatement,monitor.done()is called inside the statement loop for query statements and then called again after the loop. EnterTextDialogis opened insiderunScriptStatementsLoopviasyncExecfrom the background worker thread whilepmd.run(true, ...)is waiting on the UI thread.
Suggested Fix: Keep background tasks strictly on background threads: letpmd.run()complete, ensuremonitor.done()is in thefinallyblock, and open any dialogs (ShowRowsDialog,EnterTextDialog) on the UI thread afterpmd.run()finishes.
💡 Suggestions & UI Polish
-
Toolbar Tooltip missing
i18n:::
Inrefresh():toolTip = "System.Button.Refresh"
Needs the
i18n::prefix ("i18n::System.Button.Refresh"), otherwiseGuiToolbarWidgetsdisplays the literal raw key. -
Dialog title vs. size persistence:
The connection name was removed from the title soWindowPropertywould share dimensions. However, not seeing which connection is currently open hurts UX. We can keep the connection name inshell.setText(...)while explicitly passing a fixed ID toWindowProperty/props.setScreen(...)(e.g.new WindowProperty("DatabaseExplorerDialog", ...)). -
Search box keyboard navigation:
- Hitting
EnterinwSearchcurrently does nothing becauseBaseDialog.NO_DEFAULT_HANDLERis set. AddingwSearch.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 focuswSearchon dialog open.
- Hitting
-
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 inDatabaseWorkbench), would make them easily discoverable. -
Layout margin safety:
wTreetop is attached totoolBar, whilewSearchis on the left. IfwSearchis taller than the toolbar on some OS themes,wTreecould overlap the bottom edge ofwSearch. AnchoringwTreebelow a top header composite or below the taller control avoids any overlap. -
Deduplicating
namesForSchemainDatabaseTreeUtil:
namesForSchemais currently duplicated in bothDatabaseWorkbenchandDatabaseExplorerDialog. Moving it toDatabaseTreeUtilkeeps it DRY. (Also, ifschemaName != nullbut not found, falling through tomap.get("")ormap.get(null)can cause schema-less views to be displayed under unrelated schemas). -
Minor typos:
messages_pt_BR.properties:DatabaseExplorerDialog.Menu.OpenSQL=Abrir SQS-> should beAbrir SQL.DatabaseExplorerDialog.java:BaseMessages.getString(PKG, "DatabaseExplorerDialog.Menu.Truncate", table)passestable, but{0}was removed frommessages_*.properties.
|
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. |
c9248f2 to
13800e4
Compare
|
Thanks for the review; I think I've corrected all the issues. However, I didn't understand the comment below: |
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