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;