[GTK] Speed up Combo setItems/removeAll/remove for large item counts

bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os.c

@@ -4198,6 +4198,16 @@ JNIEXPORT void JNICALL GTK_NATIVE(gtk_1combo_1box_1set_1active)
 }
 #endif
 
+#ifndef NO_gtk_1combo_1box_1set_1model
+JNIEXPORT void JNICALL GTK_NATIVE(gtk_1combo_1box_1set_1model)
+	(JNIEnv *env, jclass that, jlong arg0, jlong arg1)
+{
+	GTK_NATIVE_ENTER(env, that, gtk_1combo_1box_1set_1model_FUNC);
+	gtk_combo_box_set_model((GtkComboBox *)arg0, (GtkTreeModel *)arg1);
+	GTK_NATIVE_EXIT(env, that, gtk_1combo_1box_1set_1model_FUNC);
+}
+#endif
+
 #ifndef NO_gtk_1combo_1box_1text_1insert
 JNIEXPORT void JNICALL GTK_NATIVE(gtk_1combo_1box_1text_1insert)
 	(JNIEnv *env, jclass that, jlong arg0, jint arg1, jbyteArray arg2, jbyteArray arg3)
@@ -4249,16 +4259,6 @@ JNIEXPORT void JNICALL GTK_NATIVE(gtk_1combo_1box_1text_1remove)
 }
 #endif
 
-#ifndef NO_gtk_1combo_1box_1text_1remove_1all
-JNIEXPORT void JNICALL GTK_NATIVE(gtk_1combo_1box_1text_1remove_1all)
-	(JNIEnv *env, jclass that, jlong arg0)
-{
-	GTK_NATIVE_ENTER(env, that, gtk_1combo_1box_1text_1remove_1all_FUNC);
-	gtk_combo_box_text_remove_all((GtkComboBoxText *)arg0);
-	GTK_NATIVE_EXIT(env, that, gtk_1combo_1box_1text_1remove_1all_FUNC);
-}
-#endif
-
 #ifndef NO_gtk_1css_1provider_1new
 JNIEXPORT jlong JNICALL GTK_NATIVE(gtk_1css_1provider_1new)
 	(JNIEnv *env, jclass that)

bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os_stats.h

@@ -329,11 +329,11 @@ typedef enum {
 	gtk_1combo_1box_1popdown_FUNC,
 	gtk_1combo_1box_1popup_FUNC,
 	gtk_1combo_1box_1set_1active_FUNC,
+	gtk_1combo_1box_1set_1model_FUNC,
 	gtk_1combo_1box_1text_1insert_FUNC,
 	gtk_1combo_1box_1text_1new_FUNC,
 	gtk_1combo_1box_1text_1new_1with_1entry_FUNC,
 	gtk_1combo_1box_1text_1remove_FUNC,
-	gtk_1combo_1box_1text_1remove_1all_FUNC,
 	gtk_1css_1provider_1new_FUNC,
 	gtk_1css_1provider_1to_1string_FUNC,
 	gtk_1dialog_1add_1button_FUNC,

bundles/org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/GTK.java

@@ -529,11 +529,6 @@ public class GTK extends OS {
 	public static final native void gtk_combo_box_text_insert(long combo_box, int position, byte[] id, byte[] text);
 	/** @param combo_box cast=(GtkComboBoxText *) */
 	public static final native void gtk_combo_box_text_remove(long combo_box, int position);
-	/**
-	 * @param combo_box cast=(GtkComboBoxText *)
-	 */
-	/* Do not call directly. Call Combo.gtk_combo_box_text_remove_all(..) instead). */
-	public static final native void gtk_combo_box_text_remove_all(long combo_box);
 	/**
 	* @param combo_box cast=(GtkComboBox *)
 	*/
@@ -544,6 +539,11 @@ public class GTK extends OS {
 	public static final native long gtk_combo_box_get_model(long combo_box);
 	/**
 	* @param combo_box cast=(GtkComboBox *)
+	* @param model cast=(GtkTreeModel *)
+	*/
+	public static final native void gtk_combo_box_set_model(long combo_box, long model);
+	/**
+	* @param combo_box cast=(GtkComboBox *)
 	* @param index cast=(gint)
 	*/
 	public static final native void gtk_combo_box_set_active(long combo_box, int index);

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java

@@ -249,6 +249,64 @@ private void gtk_combo_box_toggle_wrap (boolean wrap) {
 	}
 }
 
+/**
+ * <p>Bug 506. Bulk updates of the item list.</p>
+ *
+ * <p>Editing the GtkListStore a combo is showing is expensive per row. On GTK3,
+ *   while the wrap width is above 0, row_inserted_cb and row_deleted_cb in
+ *   gtktreemenu.c rebuild the whole popup menu on every row change, so filling
+ *   or clearing a large combo row by row is quadratic. Detaching the model with
+ *   gtk_combo_box_set_model(handle, 0) does not help, because the popup keeps
+ *   the model even when the combo drops it.</p>
+ *
+ * <p>Solution: never bulk-edit an attached store. Build a new GtkListStore, fill
+ *   it while nothing observes it, and hand it to the combo in one step, so the
+ *   popup is rebuilt once per bulk update instead of once per row. Building the
+ *   popup itself is still superlinear inside GTK.</p>
+ *
+ * @param newItems the items the combo shows afterwards
+ * @param activeIndex the item to select afterwards, or -1 for no selection
+ */
+private void setModelItems (String [] newItems, int activeIndex) {
+	if (handle == 0) return;
+	long [] types = new long [] {OS.G_TYPE_STRING (), OS.G_TYPE_STRING ()};
+	long model = GTK.gtk_list_store_newv (types.length, types);
+	if (model == 0) error (SWT.ERROR_NO_HANDLES);
+	long iter = OS.g_malloc (GTK.GtkTreeIter_sizeof ());
+	if (iter == 0) {
+		OS.g_object_unref (model);
+		error (SWT.ERROR_NO_HANDLES);
+	}
+	for (String item : newItems) {
+		GTK.gtk_list_store_append (model, iter);
+		GTK.gtk_list_store_set (model, iter, 0, Converter.wcsToMbcs (item, true), -1);
+	}
+	OS.g_free (iter);
+
+	// On GTK4 swapping the model emits "changed" for the dropped active item, which
+	// would send spurious Modify/Selection events, so keep the combo quiet meanwhile.
+	OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+	GTK.gtk_combo_box_set_model (handle, model);
+	OS.g_object_unref (model);
+	if (activeIndex != -1) {
+		// On a combo with an entry, block every "changed" handler, including GTK's own that rewrites
+		// the entry from the model row, so the entry keeps its text (possibly altered by a Verify
+		// listener) and caret. This also blocks GTK3's GtkComboBoxAccessible, whose index can go stale.
+		int changedId = entryHandle != 0 ? OS.g_signal_lookup (OS.changed, OS.G_OBJECT_TYPE (handle)) : 0;
+		if (changedId != 0) OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_ID, changedId, 0, 0, 0, 0);
+		GTK.gtk_combo_box_set_active (handle, activeIndex);
+		if (changedId != 0) OS.g_signal_handlers_unblock_matched (handle, OS.G_SIGNAL_MATCH_ID, changedId, 0, 0, 0, 0);
+	}
+	OS.g_signal_handlers_unblock_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+	// Editable combos only get wrap enabled on insert, which bulk updates no longer go through.
+	if (newItems.length > 0) gtk_combo_box_toggle_wrap (true);
+
+	// The swap rebuilds the popup, so its children lost the direction set for them before.
+	if ((style & SWT.RIGHT_TO_LEFT) != 0 && popupHandle != 0) {
+		GTK3.gtk_container_forall (popupHandle, display.setDirectionProc, GTK.GTK_TEXT_DIR_RTL);
+	}
+}
+
 /**
  * Adds the listener to the collection of listeners who will
  * be notified when the receiver's text is modified, by sending
@@ -2073,13 +2131,14 @@ public void remove (int start, int end) {
 	System.arraycopy (oldItems, end + 1, newItems, start, oldItems.length - end - 1);
 	items = newItems;
 	int index = GTK.gtk_combo_box_get_active (handle);
-	if (start <= index && index <= end) clearText();
-
-	gtk_combo_box_toggle_wrap(false);
-	for (int i = end; i >= start; i--) {
-		if (handle != 0) GTK.gtk_combo_box_text_remove(handle, i);
+	boolean selectionRemoved = start <= index && index <= end;
+	if (selectionRemoved) clearText();
+	// Rebuilding the model drops the active item, so remember where it moves to.
+	int newIndex = -1;
+	if (index != -1 && !selectionRemoved) {
+		newIndex = index > end ? index - (end - start + 1) : index;
 	}
-	gtk_combo_box_toggle_wrap(true);
+	setModelItems (items, newIndex);
 }
 
 /**
@@ -2119,7 +2178,7 @@ public void removeAll () {
 
 	items = new String[0];
 	clearText();
-	gtk_combo_box_text_remove_all();
+	setModelItems (items, -1);
 }
 
 /**
@@ -2408,20 +2467,7 @@ public void setItems (String... items) {
 	System.arraycopy (items, 0, this.items, 0, items.length);
 	clearText ();
 
-	gtk_combo_box_text_remove_all();
-	for (int i = 0; i < items.length; i++) {
-		String string = items [i];
-		gtk_combo_box_insert(string, i);
-		if ((style & SWT.RIGHT_TO_LEFT) != 0 && popupHandle != 0) {
-			GTK3.gtk_container_forall (popupHandle, display.setDirectionProc, GTK.GTK_TEXT_DIR_RTL);
-		}
-	}
-}
-
-private void gtk_combo_box_text_remove_all() {
-	gtk_combo_box_toggle_wrap(false);
-	if (handle != 0) GTK.gtk_combo_box_text_remove_all(handle);
-	gtk_combo_box_toggle_wrap(true);
+	setModelItems (this.items, -1);
 }
 
 /**

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java

@@ -20,6 +20,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import java.util.concurrent.atomic.AtomicInteger;
@@ -648,6 +649,154 @@ public void test_removeII() {
 	}
 }
 
+@Test
+public void test_removeII_keepsSelectionOutsideRange() {
+	// Bug 506: removing a range outside the selection must keep the same item
+	// selected even though GTK rebuilds the model. Selection handling is platform
+	// specific, so assert it only on GTK.
+	String[] items = {"item0", "item1", "item2", "item3", "item4"};
+
+	// Selection after the removed range: index shifts down by the removed count.
+	combo.setItems(items);
+	combo.select(4);
+	combo.remove(0, 1);
+	assertEquals(3, combo.getItemCount());
+	if (SwtTestUtil.isGTK) {
+		assertEquals(2, combo.getSelectionIndex());
+		assertEquals("item4", combo.getItem(combo.getSelectionIndex()));
+	}
+
+	// Selection right after the removed range.
+	combo.setItems(items);
+	combo.select(2);
+	combo.remove(0, 1);
+	if (SwtTestUtil.isGTK) {
+		assertEquals(0, combo.getSelectionIndex());
+		assertEquals("item2", combo.getItem(combo.getSelectionIndex()));
+	}
+
+	// Selection before the removed range: index is unchanged.
+	combo.setItems(items);
+	combo.select(0);
+	combo.remove(2, 3);
+	assertEquals(3, combo.getItemCount());
+	if (SwtTestUtil.isGTK) {
+		assertEquals(0, combo.getSelectionIndex());
+		assertEquals("item0", combo.getItem(combo.getSelectionIndex()));
+	}
+
+	// Selection inside the removed range: selection is cleared.
+	combo.setItems(items);
+	combo.select(2);
+	combo.remove(1, 3);
+	assertEquals(2, combo.getItemCount());
+	if (SwtTestUtil.isGTK) {
+		assertEquals(-1, combo.getSelectionIndex());
+	}
+}
+
+@Test
+public void test_bulkUpdatesKeepShownItemsInSync() {
+	// Bug 506: setItems/remove/removeAll replace the GTK model, so selecting an item
+	// afterwards, and adding items to the new model, must still show the right text.
+	Combo editable = new Combo(shell, SWT.DROP_DOWN);
+	editable.setItems("item0", "item1", "item2", "item3");
+	editable.select(3);
+	assertEquals("item3", editable.getText());
+
+	editable.remove(0, 1);
+	editable.select(0);
+	assertEquals("item2", editable.getText());
+
+	editable.removeAll();
+	editable.add("y");
+	editable.add("x", 0);
+	editable.select(1);
+	assertEquals("y", editable.getText());
+	editable.select(0);
+	assertEquals("x", editable.getText());
+	editable.dispose();
+}
+
+@Test
+public void test_bulkUpdatesSendNoEventsWhenNothingIsSelected() {
+	// Bug 506: setItems/remove/removeAll rebuild the GTK model internally. That must
+	// stay invisible to applications, so an unselected combo must send no events.
+	// Editable combos are covered too, because they carry a second set of listeners
+	// on the entry widget.
+	String[] items = {"item0", "item1", "item2", "item3", "item4"};
+	for (int style : new int[] {SWT.READ_ONLY, SWT.DROP_DOWN}) {
+		String message = "style " + style;
+		Combo bulk = new Combo(shell, style);
+		AtomicInteger modifyCount = new AtomicInteger();
+		AtomicInteger selectionCount = new AtomicInteger();
+		bulk.addModifyListener(e -> modifyCount.incrementAndGet());
+		bulk.addSelectionListener(SelectionListener.widgetSelectedAdapter(e -> selectionCount.incrementAndGet()));
+
+		bulk.setItems(items);
+		bulk.remove(0, 1);
+		bulk.removeAll();
+		SwtTestUtil.processEvents();
+
+		assertEquals(0, selectionCount.get(), message);
+		if (SwtTestUtil.isGTK) {
+			assertEquals(0, modifyCount.get(), message);
+		}
+		bulk.dispose();
+	}
+}
+
+@Test
+public void test_removeII_keepsSelectedItemWithoutEvents() {
+	// Bug 506: a range removal that keeps the selected item must not change the shown
+	// text or its text selection, and must not report that as a modification or as a new selection.
+	assumeFalse(SwtTestUtil.isCocoa,
+			"Cocoa sends a Selection event for an editable Combo when items before the selection are removed");
+	String[] items = {"item0", "item1", "item2", "item3", "item4"};
+	for (int style : new int[] {SWT.READ_ONLY, SWT.DROP_DOWN}) {
+		String message = "style " + style;
+		Combo bulk = new Combo(shell, style);
+		bulk.setItems(items);
+		bulk.select(4);
+		bulk.setSelection(new Point(1, 3));
+		String textBefore = bulk.getText();
+		Point selectionBefore = bulk.getSelection();
+		AtomicInteger modifyCount = new AtomicInteger();
+		AtomicInteger selectionCount = new AtomicInteger();
+		bulk.addModifyListener(e -> modifyCount.incrementAndGet());
+		bulk.addSelectionListener(SelectionListener.widgetSelectedAdapter(e -> selectionCount.incrementAndGet()));
+
+		bulk.remove(0, 1);
+		SwtTestUtil.processEvents();
+
+		assertEquals(0, selectionCount.get(), message);
+		if (SwtTestUtil.isGTK) {
+			assertEquals(textBefore, bulk.getText(), message);
+			assertEquals(selectionBefore, bulk.getSelection(), message);
+			assertEquals(0, modifyCount.get(), message);
+		}
+		bulk.dispose();
+	}
+}
+
+@Test
+public void test_removeII_keepsVerifiedTextOfSelectedItem() {
+	// Bug 506: the shown text of a kept selection must survive a range removal, even
+	// when a Verify listener made it differ from the item. Whether select() runs Verify
+	// listeners is platform specific, so assert it only on GTK.
+	assumeTrue(SwtTestUtil.isGTK, "select() does not send Verify events on this platform");
+	Combo editable = new Combo(shell, SWT.DROP_DOWN);
+	editable.setItems("a", "b", "cat");
+	editable.addVerifyListener(e -> e.text = e.text.toUpperCase());
+	editable.select(2);
+	assertEquals("CAT", editable.getText());
+
+	editable.remove(0, 1);
+	assertEquals("CAT", editable.getText());
+	assertEquals(0, editable.getSelectionIndex());
+	editable.dispose();
+}
+
 @Test
 public void test_removeLjava_lang_String() {
 	int number = 5;