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

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

@@ -4186,6 +4186,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)
@@ -4237,16 +4247,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

@@ -328,11 +328,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

@@ -525,11 +525,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 *)
 	*/
@@ -540,6 +535,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,66 @@ 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 costs O(n) per row: the popup
+ *   keeps its own handlers on the model and rebuilds an item for every
+ *   row-inserted/row-changed, which makes filling or clearing a large combo
+ *   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. Filling
+ *   is then linear and the popup is rebuilt exactly once.</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);
+
+	// Swapping the model resets the active item and emits "changed". On a combo with
+	// an entry, restoring the selection also rewrites the entry text with the value it
+	// already had. Both would send spurious Modify/Verify events, so keep the combo
+	// and its entry quiet meanwhile; the visible text is unchanged either way.
+	OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+	if (entryHandle != 0) {
+		OS.g_signal_handlers_block_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+		OS.g_signal_handlers_block_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, DELETE_TEXT);
+		OS.g_signal_handlers_block_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, INSERT_TEXT);
+	}
+	gtk_combo_box_toggle_wrap (false);
+	GTK.gtk_combo_box_set_model (handle, model);
+	OS.g_object_unref (model);
+	if (activeIndex != -1) GTK.gtk_combo_box_set_active (handle, activeIndex);
+	gtk_combo_box_toggle_wrap (true);
+	if (entryHandle != 0) {
+		OS.g_signal_handlers_unblock_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, INSERT_TEXT);
+		OS.g_signal_handlers_unblock_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, DELETE_TEXT);
+		OS.g_signal_handlers_unblock_matched (entryHandle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+	}
+	OS.g_signal_handlers_unblock_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+
+	// 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
@@ -2066,13 +2126,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);
 }
 
 /**
@@ -2112,7 +2173,7 @@ public void removeAll () {
 
 	items = new String[0];
 	clearText();
-	gtk_combo_box_text_remove_all();
+	setModelItems (items, -1);
 }
 
 /**
@@ -2401,20 +2462,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

@@ -21,6 +21,7 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import java.util.concurrent.atomic.AtomicInteger;
@@ -649,6 +650,101 @@ 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 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_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, 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);
+		String textBefore = bulk.getText();
+		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(0, modifyCount.get(), message);
+		}
+		bulk.dispose();
+	}
+}
+
 @Test
 public void test_removeLjava_lang_String() {
 	int number = 5;