From ced379808a0266477db54a83731d12063223f301 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 Setting or removing a large number of combo items (>5000) was very slow on GTK: editing the GtkListStore a combo is showing costs O(n) per row, because the popup keeps its own handlers on the model and rebuilds an item for every row-inserted/row-changed. Filling or clearing a large combo was therefore 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. Filling is then linear and the popup is rebuilt exactly once. Measured on GTK3 with 16000 items: setItems 41.9s -> 0.4s, removing half the items 12.2s -> 0.5s, removeAll 2.1s -> 0.1s. The model swap resets the active item and emits "changed", and on a combo with an entry restoring the selection rewrites the entry text with the value it already had. The CHANGED closure on the combo and the CHANGED, INSERT_TEXT and DELETE_TEXT closures on the entry are therefore blocked while the model is swapped, 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. 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 --- .../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 | 90 +++++++++++++---- .../Test_org_eclipse_swt_widgets_Combo.java | 96 +++++++++++++++++++ 5 files changed, 181 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 24178bd35c9..1e60f0458a7 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 @@ -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) 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 f30ca89d0ad..f15e3d60699 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 @@ -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, 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 9a6c2cb9cf8..ca8fa6f5997 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 @@ -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); 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 d58fed8720e..3e3bb539310 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,66 @@ private void gtk_combo_box_toggle_wrap (boolean wrap) { } } +/** + *

Bug 506. Bulk updates of the item list.

+ * + *

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.

+ * + *

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.

+ * + * @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); } /** 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 7b977c2eddc..1119cd6a9c6 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 @@ -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;