Bug 456602 - [Cocoa] ArrayIndexOutOfBoundsException in Table._getItem
The problem is that item for cached `selectedRowIndex` can be deleted
between mouse down (where variable is set) and mouse up (where it's used).
I tried to solve the original problem (where macOS sends SWT.Selection
after `[NSEvent doubleClickInterval]` from mouse down) but didn't find a
way to do that. It seems that there are no settings that could disable
double click detection in `-[NSTableView mouseDown:]`. So the workaround
has to stay, it seems.
I also noticed that macOS cancels pending selection event in
`-[NSTableView noteNumberOfRowsChanged]`. SWT already calls that
whenever number of items changes (item is deleted or added). I was
thinking to reset `selectedRowIndex` in it, but then I wasn't sure
if it's the right thing to do if for example an item is added at the
end - surely this doesn't prevent from proceeding with handling click
on the old item? I the end, I decided to not overcomplicate things and
just fix the crash without affecting anything else.
Change-Id: I42cb381d408d105e4df6372a2d69967b9b274a9a
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/192115
Tested-by: Platform Bot <platform-bot@eclipse.org>
Tested-by: Lakshmi P Shanmugam <lshanmug@in.ibm.com>
Reviewed-by: Lakshmi P Shanmugam <lshanmug@in.ibm.com>
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java
index 965fb9d..c49f135 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java
@@ -3636,39 +3636,66 @@
return sendMouseEvent(NSApplication.sharedApplication().currentEvent(), SWT.DragDetect, true);
}
+void handleClickSelected() {
+ /*
+ * When there are multiple selected items and one of them is clicked
+ * without modifiers, macOS supports two cases:
+ * 1) For single click, all other items are deselected
+ * 2) For double-click, selection stays as is, allowing to create
+ * double-click event with multiple items.
+ * In order to distinguish between the two, macOS delays (1) by
+ * [NSEvent doubleClickInterval] in order to see if it's case (2).
+ * This causes SWT.Selection to occur after SWT.MouseUp.
+ *
+ * Bug 289483: For consistent cross-platform behavior, we want
+ * SWT.Selection to occur before SWT.MouseUp. The workaround is to
+ * implement (1) in SWT code and ignore the delayed selection event.
+ */
+
+ int clickedRow = selectedRowIndex;
+ selectedRowIndex = -1;
+
+ if (clickedRow == -1) return;
+ if (dragDetected) return;
+
+ // Deselect all items except the clicked one
+ NSTableView widget = (NSTableView)view;
+ NSIndexSet selectedRows = widget.selectedRowIndexes ();
+ int count = (int)selectedRows.count();
+ long [] indexBuffer = new long [count];
+ selectedRows.getIndexes(indexBuffer, count, 0);
+ for (int i = 0; i < count; i++) {
+ if (indexBuffer[i] == clickedRow) continue;
+ ignoreSelect = true;
+ widget.deselectRow (indexBuffer[i]);
+ ignoreSelect = false;
+ }
+
+ // Bug 456602: It's possible that item is removed between mouse
+ // down (where 'selectedRowIndex' was cached) and mouse up (current
+ // code). In such case, all other items are still deselected, because
+ // 1) without workaround, selection should have happened in mouse down,
+ // where item still existed
+ // 2) clicking empty space deselects all items on macOS
+ // If item is deleted, then pending selection is canceled by macOS, so
+ // there's no need to ignore the next selection event.
+ if (clickedRow >= itemCount) return;
+
+ // Emulate SWT.Selection
+ Event event = new Event ();
+ event.item = _getItem(clickedRow);
+ sendSelectionEvent (SWT.Selection, event, false);
+
+ // Ignore real SWT.Selection that will arrive later
+ ignoreSelect = true;
+}
+
@Override
boolean sendMouseEvent(NSEvent nsEvent, int type, boolean send) {
if (type == SWT.DragDetect) {
dragDetected = true;
} else if (type == SWT.MouseUp) {
- /*
- * This code path handles the case of an unmodified click on an already-selected row.
- * To keep the order of events correct, deselect the other selected items and send the
- * selection event before MouseUp is sent. Ignore the next selection event.
- */
- if (selectedRowIndex != -1) {
- if (dragDetected) {
- selectedRowIndex = -1;
- } else {
- NSTableView widget = (NSTableView)view;
- NSIndexSet selectedRows = widget.selectedRowIndexes ();
- int count = (int)selectedRows.count();
- long [] indexBuffer = new long [count];
- selectedRows.getIndexes(indexBuffer, count, 0);
- for (int i = 0; i < count; i++) {
- if (indexBuffer[i] == selectedRowIndex) continue;
- ignoreSelect = true;
- widget.deselectRow (indexBuffer[i]);
- ignoreSelect = false;
- }
-
- Event event = new Event ();
- event.item = _getItem ((int)selectedRowIndex);
- selectedRowIndex = -1;
- sendSelectionEvent (SWT.Selection, event, false);
- ignoreSelect = true;
- }
- }
+ handleClickSelected();
dragDetected = false;
}
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java
index 0485d22..1c2de57 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java
@@ -2805,11 +2805,7 @@
if (type == SWT.DragDetect) {
dragDetected = true;
} else if (type == SWT.MouseUp) {
- /*
- * This code path handles the case of an unmodified click on an already-selected row.
- * To keep the order of events correct, deselect the other selected items and send the
- * selection event before MouseUp is sent. Ignore the next selection event.
- */
+ // See code comment in Table.handleClickSelected()
if (selectedRowIndex != -1) {
if (dragDetected) {
selectedRowIndex = -1;
@@ -2828,6 +2824,8 @@
Event event = new Event ();
id itemID = widget.itemAtRow (selectedRowIndex);
+ // (itemID = null) means that item was removed after
+ // 'selectedRowIndex' was cached
if (itemID != null) {
Widget item = display.getWidget (itemID.id);
if (item != null && item instanceof TreeItem) {
diff --git a/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug456602_macOS_AIOOBE_Table_selectedRowIndex.java b/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug456602_macOS_AIOOBE_Table_selectedRowIndex.java
new file mode 100644
index 0000000..c161b26
--- /dev/null
+++ b/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug456602_macOS_AIOOBE_Table_selectedRowIndex.java
@@ -0,0 +1,90 @@
+/*******************************************************************************
+ * Copyright (c) 2022 Syntevo and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ * Syntevo - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.swt.tests.manual;
+
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.dnd.*;
+import org.eclipse.swt.layout.GridLayout;
+import org.eclipse.swt.widgets.*;
+
+import java.util.Arrays;
+
+public final class Bug456602_macOS_AIOOBE_Table_selectedRowIndex {
+ public static void main(String[] args) {
+ final Display display = new Display();
+
+ final Shell shell = new Shell(display);
+ shell.setLayout(new GridLayout(1, true));
+
+ Label hint = new Label(shell, 0);
+ hint.setText(
+ "Bug 456602\n" +
+ "----------\n" +
+ "1) Use macOS\n" +
+ "2) Click 'Delete items' button\n" +
+ "3) Press left mouse button on last table item and HOLD the button\n" +
+ "4) When item is deleted, release the button\n" +
+ "5) Bug 456602: SWT will have an internal AIOOBE\n" +
+ "6) Bug 456602: More exceptions are reported if you set focus to\n" +
+ " Text beforehand and move the mouse after\n" +
+ "7) In real world, user would clicks faster, but crashes will\n" +
+ " still happen sometimes\n" +
+ "\n" +
+ "Bug 355200 and Bug 289483\n" +
+ "-------------------------\n" +
+ "Just to ensure that old workarounds are still working fine:\n" +
+ "1) Click any Table item when multiple items are selected\n" +
+ "2) SWT.Selection shall report clicked item\n" +
+ "3) The order shall be: SWT.MouseDown, SWT.Selection, SWT.MouseUp"
+ );
+
+ Table table = new Table(shell, SWT.BORDER | SWT.MULTI);
+ table.addListener(SWT.MouseDown, e -> {
+ System.out.println(System.currentTimeMillis() + " SWT.MouseDown: " + Arrays.toString(table.getSelectionIndices()));
+ });
+ table.addListener(SWT.MouseUp, e -> {
+ System.out.println(System.currentTimeMillis() + " SWT.MouseUp: " + Arrays.toString(table.getSelectionIndices()));
+ });
+ table.addListener(SWT.Selection, e -> {
+ System.out.println(System.currentTimeMillis() + " SWT.Selection: " + Arrays.toString(table.getSelectionIndices()));
+ });
+
+ for (int iItem = 0; iItem < 10; iItem++) {
+ new TableItem(table, 0).setText("Item #" + iItem);
+ }
+ table.selectAll();
+
+ Button button = new Button(shell, SWT.PUSH);
+ button.setText("Delete items after 2000ms");
+ button.addListener(SWT.Selection, e -> {
+ display.timerExec(2000, () -> {
+ table.setItemCount(table.getItemCount() / 2);
+ });
+ });
+
+ new Text(shell, 0).setText("I'm a text field");
+
+ shell.pack();
+ shell.open();
+
+ while (!shell.isDisposed()) {
+ if (!display.readAndDispatch()) {
+ display.sleep();
+ }
+ }
+
+ display.dispose();
+ }
+}
