mattcasters commented on code in PR #8530:
URL: https://github.com/apache/hop/pull/8530#discussion_r4262847694
##########
ui/src/main/java/org/apache/hop/ui/core/widget/TableView.java:
##########
@@ -2162,6 +2210,11 @@ private void editSelected() {
return;
}
+ if (readonly) {
+ selectRows(activeTableRow, activeTableRow);
Review Comment:
**[bug]** This read-only branch selects the row and returns without
`enableToolbarButtons()`. `selectRows` ends in `Table.select` / `deselectAll`,
which do not emit `SWT.Selection` (GTK blocks the selection signals for the
duration of `select`; `selectAll`, `unselectAll`, and `edit` already refresh
the toolbar themselves for that reason). The listener runs from `mouseDown`
before the native click handler, and once this call has selected the row the
native handler usually does not emit a second change either. Copy on the
toolbar is enabled only when `getSelectionCount() > 0` and starts disabled, so
the first click on a result grid (database results, execution data, parquet
preview) selects the row but leaves Copy disabled. The context-menu item is
unaffected because it is always enabled.
**Suggestion:** Call `enableToolbarButtons()` after `selectRows` in this
branch.
##########
ui/src/main/java/org/apache/hop/ui/core/widget/TableView.java:
##########
@@ -2912,6 +2965,22 @@ public void clipSelected() {
clipboard.setContents(new String[] {clip}, new Transfer[] {tran});
}
+ public void clipCell() {
Review Comment:
**[suggestion]** There is no test for the new read-only behavior. `rcp`
already drives `TableView` on the UI thread (`TableViewFindRevealTest` sets
`readonly`). `clipCell()` is public, so a readonly grid can assert the
clipboard text for a value stored with `setCellValue` (full text, not the
shortened display) and for a blue `<null>` placeholder. The same harness can
assert that a programmatic select through this click path enables the Copy
toolbar item.
**Suggestion:** Add a UI-thread test next to the existing `TableView*Test`
classes for those two clipboard cases and for toolbar enablement after the
read-only selection.
##########
ui/src/main/java/org/apache/hop/ui/core/widget/TableView.java:
##########
@@ -778,6 +808,8 @@ public void mouseDown(MouseEvent event) {
boolean shift = (event.stateMask & SWT.SHIFT) != 0;
boolean control = (event.stateMask & SWT.MOD1) != 0;
if (!shift && !control) {
+ // Cleared below as soon as the click turns out to be on a cell.
+ contextMenuOffCell = rightClick;
Review Comment:
**[suggestion]** `contextMenuOffCell` is updated only for an unmodified left
or right click, but `hasCellToCopy()` is applied on every menu `Show`.
Shift+right-click and Ctrl+right-click still open this menu and never touch the
flag, and a later Shift+F10 or menu-key open reuses whatever the last plain
click stored. An off-cell right-click sets the flag, then Shift+F10 with a real
active cell keeps Copy cell value disabled; a plain click on a cell followed by
Shift+right-click below the rows leaves it enabled and copies the old cell.
That is the case the flag was added to prevent.
**Suggestion:** Update the flag on every button-3 `mouseDown`, modifiers
included, or decide it from the menu-detect point. Treat a keyboard-opened menu
as on-cell when `activeTableColumn > 0`.
##########
ui/src/main/java/org/apache/hop/ui/core/widget/TableView.java:
##########
@@ -2912,6 +2965,22 @@ public void clipSelected() {
clipboard.setContents(new String[] {clip}, new Transfer[] {tran});
}
+ public void clipCell() {
+ if (!hasCellToCopy()) {
+ return;
+ }
+ GuiResource.getInstance().toClipboard(getCellValue(activeTableItem,
activeTableColumn));
Review Comment:
**[bug]** `clipCell` copies `getCellValue` unchanged. A null in a result
grid is not stored with `setCellValue`; the cell text is the blue placeholder
`<null>` (`DatabaseResultsPanel` and the other preview grids). `getCellValue`
therefore returns `"<null>"`, and Copy cell value puts that literal on the
clipboard. `getSelectedText()` already drops a cell whose foreground is
`nullTextColor` and whose text is `"<null>"`, so Copy selected lines and Copy
cell value disagree on the same NULL. Pasting a SQL NULL into a spreadsheet
yields the text `<null>`.
**Suggestion:** When
`nullTextColor.equals(activeTableItem.getForeground(activeTableColumn))` and
the value is `"<null>"`, pass an empty string to `toClipboard`. Leave a real
`"<null>"` that is not blue unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
