From dae9026a7f5610619f178a843960525b6bf376a2 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Wed, 24 Jun 2026 11:35:01 +0200 Subject: [PATCH] [GTK] Speed up Combo setItems/removeAll/remove for large item counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting or removing a large number of combo items (>5000) was very slow on GTK, because 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 was quadratic. Detaching the model with gtk_combo_box_set_model(handle, 0) does not help, since the popup keeps the model even when the combo drops it. Instead setItems, removeAll and remove(start, end) now 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. The wrap width is not turned off around the swap, since a freshly filled model does not need the toggle that makes single row inserts cheap, but it is still enabled afterwards when the combo has items, since editable combos used to get it on insert. Building the popup is still superlinear inside GTK, but with 16000 items on GTK 3.24 setItems drops from about 46s to under 2s, and removing half the items or all of them improve similarly. On GTK4 the model swap emits "changed" for the dropped active item, so the CHANGED closure is blocked meanwhile, which also removes the spurious Modify events the per-row insert used to send. remove(start, end) restores the active selection, adjusted for the rows removed before it. On a combo with an entry, all "changed" handlers are blocked for that restore, so GTK does not rewrite the entry from the model row and the shown text (possibly altered by a Verify listener) and the caret stay as they were. Adds the gtk_combo_box_set_model native binding and drops the now-unused gtk_combo_box_text_remove_all binding. Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/506 Assisted-by: multiple AI agents and layers of automated tooling 🤖 --- .../Eclipse SWT PI/gtk/library/os.c | 20 +-- .../Eclipse SWT PI/gtk/library/os_stats.h | 2 +- .../gtk/org/eclipse/swt/internal/gtk/GTK.java | 10 +- .../gtk/org/eclipse/swt/widgets/Combo.java | 88 ++++++++--- .../Test_org_eclipse_swt_widgets_Combo.java | 149 ++++++++++++++++++ 5 files changed, 232 insertions(+), 37 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os.c b/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os.c index 900bb61de81..ed7820e1125 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os.c +++ b/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) diff --git a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os_stats.h b/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os_stats.h index 6b398d03cb1..ce100534d00 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os_stats.h +++ b/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, diff --git a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/GTK.java b/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/GTK.java index 5f240f511bc..491224e8889 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/GTK.java +++ b/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); diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java index a0d72ec58ee..1a44dcddff6 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java +++ b/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) { } } +/** + *

Bug 506. Bulk updates of the item list.

+ * + *

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.

+ * + *

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.

+ * + * @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); } /** diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java index 89851719cb5..f046f46b51c 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java +++ b/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;