From ffbe971929b7ecbe21be6dcd632f58a9ca56cc7e Mon Sep 17 00:00:00 2001 From: Fuzss Date: Mon, 22 Jun 2026 19:06:06 +0200 Subject: [PATCH] Fix `ButtonList` `IndexOutOfBoundsException` for screens with renderable only widgets (#5445) * fix ButtonList implementation * add tests * remove legacy button list used for testing only * apply spotless * fix checkstyle --- .../fabric/impl/client/screen/ButtonList.java | 166 ++++++++------ .../fabric/mixin/screen/ScreenMixin.java | 2 +- .../screen/unittests/ButtonListTests.java | 215 ++++++++++++++++++ 3 files changed, 310 insertions(+), 73 deletions(-) create mode 100644 fabric-screen-api-v1/src/test/java/net/fabricmc/fabric/test/screen/unittests/ButtonListTests.java diff --git a/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/impl/client/screen/ButtonList.java b/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/impl/client/screen/ButtonList.java index 186be34fb9..76e65b94e7 100644 --- a/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/impl/client/screen/ButtonList.java +++ b/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/impl/client/screen/ButtonList.java @@ -38,88 +38,110 @@ public final class ButtonList extends AbstractList { @Override public AbstractWidget get(int index) { - final int renderableIndex = translateIndex(renderables, index, false); - return (AbstractWidget) renderables.get(renderableIndex); - } - - @Override - public AbstractWidget set(int index, AbstractWidget element) { - final int renderableIndex = translateIndex(renderables, index, false); - renderables.set(renderableIndex, element); - - final int narratableIndex = translateIndex(narratables, index, false); - narratables.set(narratableIndex, element); - - final int childIndex = translateIndex(children, index, false); - return (AbstractWidget) children.set(childIndex, element); - } - - @Override - public void add(int index, AbstractWidget element) { - // ensure no duplicates - final int duplicateIndex = renderables.indexOf(element); - - if (duplicateIndex >= 0) { - renderables.remove(element); - narratables.remove(element); - children.remove(element); - - if (duplicateIndex <= translateIndex(renderables, index, true)) { - index--; - } - } - - final int renderableIndx = translateIndex(renderables, index, true); - renderables.add(renderableIndx, element); - - final int narratableIndex = translateIndex(narratables, index, true); - narratables.add(narratableIndex, element); - - final int childIndex = translateIndex(children, index, true); - children.add(childIndex, element); - } - - @Override - public AbstractWidget remove(int index) { - index = translateIndex(renderables, index, false); - - final AbstractWidget removedButton = (AbstractWidget) renderables.remove(index); - this.narratables.remove(removedButton); - this.children.remove(removedButton); - - return removedButton; - } - - @Override - public int size() { - int ret = 0; - - for (Renderable renderable : renderables) { - if (renderable instanceof AbstractWidget) { - ret++; - } - } - - return ret; - } - - private int translateIndex(List list, int index, boolean allowAfter) { int remaining = index; - for (int i = 0, max = list.size(); i < max; i++) { - if (list.get(i) instanceof AbstractWidget) { + for (Renderable renderable : renderables) { + if (renderable instanceof AbstractWidget widget) { if (remaining == 0) { - return i; + return widget; } remaining--; } } - if (allowAfter && remaining == 0) { - return list.size(); + throw new IndexOutOfBoundsException(String.format("Index: %d, Size: %d", index, size())); + } + + @Override + public AbstractWidget set(int index, AbstractWidget element) { + AbstractWidget existing = get(index); + + int i = renderables.indexOf(existing); + if (i >= 0) renderables.set(i, element); + + i = narratables.indexOf(existing); + if (i >= 0) narratables.set(i, element); + + i = children.indexOf(existing); + if (i >= 0) children.set(i, element); + + return existing; + } + + @Override + public void add(int index, AbstractWidget element) { + // Remove any existing occurrence and adjust the target index accordingly. + int duplicateIndex = listIndexOf(element); + + if (duplicateIndex >= 0) { + renderables.remove(element); + narratables.remove(element); + children.remove(element); + + if (duplicateIndex < index) { + index--; + } } - throw new IndexOutOfBoundsException(String.format("Index: %d, Size: %d", index, index - remaining)); + if (index > size()) { + throw new IndexOutOfBoundsException(String.format("Index: %d, Size: %d", index, size())); + } else if (index == size()) { + renderables.add(element); + narratables.add(element); + children.add(element); + } else { + // Use an anchor widget and insert before it. + AbstractWidget anchor = get(index); + + int i = renderables.indexOf(anchor); + renderables.add(i >= 0 ? i : renderables.size(), element); + + i = narratables.indexOf(anchor); + narratables.add(i >= 0 ? i : narratables.size(), element); + + i = children.indexOf(anchor); + children.add(i >= 0 ? i : children.size(), element); + } + } + + private int listIndexOf(AbstractWidget element) { + int index = 0; + + for (Renderable renderable : renderables) { + if (renderable instanceof AbstractWidget widget) { + if (widget == element) { + return index; + } + + index++; + } + } + + return -1; + } + + @Override + public AbstractWidget remove(int index) { + AbstractWidget removedButton = get(index); + + renderables.remove(removedButton); + narratables.remove(removedButton); + children.remove(removedButton); + + return removedButton; + } + + @Override + public int size() { + int size = 0; + + for (Renderable renderable : renderables) { + if (renderable instanceof AbstractWidget) { + size++; + } + } + + return size; } } diff --git a/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/mixin/screen/ScreenMixin.java b/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/mixin/screen/ScreenMixin.java index dec0e45e6b..31e504bd1b 100644 --- a/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/mixin/screen/ScreenMixin.java +++ b/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/mixin/screen/ScreenMixin.java @@ -55,7 +55,7 @@ abstract class ScreenMixin implements ScreenExtensions { private List renderables; @Unique - private ButtonList fabricButtons; + private List fabricButtons; @Unique private Event removeEvent; @Unique diff --git a/fabric-screen-api-v1/src/test/java/net/fabricmc/fabric/test/screen/unittests/ButtonListTests.java b/fabric-screen-api-v1/src/test/java/net/fabricmc/fabric/test/screen/unittests/ButtonListTests.java new file mode 100644 index 0000000000..42f5479445 --- /dev/null +++ b/fabric-screen-api-v1/src/test/java/net/fabricmc/fabric/test/screen/unittests/ButtonListTests.java @@ -0,0 +1,215 @@ +/* + * Copyright (c) 2016, 2017, 2018, 2019 FabricMC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package net.fabricmc.fabric.test.screen.unittests; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import net.minecraft.client.gui.components.AbstractWidget; +import net.minecraft.client.gui.components.Button; +import net.minecraft.client.gui.screens.Screen; +import net.minecraft.network.chat.CommonComponents; + +import net.fabricmc.fabric.api.client.screen.v1.Screens; + +public class ButtonListTests { + @Test + public void testSize() { + List widgets = Screens.getWidgets(screen()); + assertEquals(7, widgets.size()); + } + + @Test + public void testAdd() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(button); + assertEquals(size, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + } + + @Test + public void testAddBeforeRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(0, button); + assertEquals(0, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + } + + @Test + public void testAddAtRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(1, button); + assertEquals(1, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + } + + @Test + public void testAddAfterRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(2, button); + assertEquals(2, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + } + + @Test + public void testRemove() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(button); + assertEquals(size, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(button); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveBeforeRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(0, button); + assertEquals(0, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(button); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveAtRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(1, button); + assertEquals(1, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(button); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveAfterRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(2, button); + assertEquals(2, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(button); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveIndex() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(size, button); + assertEquals(size, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(size); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveIndexBeforeRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(0, button); + assertEquals(0, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(0); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveIndexAtRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(1, button); + assertEquals(1, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(1); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + @Test + public void testRemoveIndexAfterRenderable() { + List widgets = Screens.getWidgets(screen()); + int size = widgets.size(); + Button button = button(); + widgets.add(2, button); + assertEquals(2, widgets.indexOf(button)); + assertEquals(size + 1, widgets.size()); + widgets.remove(2); + assertEquals(-1, widgets.indexOf(button)); + assertEquals(size, widgets.size()); + } + + private static Screen screen() { + // There must be more Button instances added via Screen::addRenderableOnly than via Screen::addWidget to properly test reliance on the backing Screen#renderables list. + return new Screen(null, null, CommonComponents.EMPTY) { + { + // Present in renderables: true, present in children: true, present in ButtonList: true + this.addRenderableWidget(button()); + // Present in renderables: true, present in children: false, present in ButtonList: true + this.addRenderableOnly(button()); + // Present in renderables: true, present in children: false, present in ButtonList: false (not an AbstractWidget) + this.addRenderableOnly((graphics, mouseX, mouseY, a) -> { + // NO-OP + }); + // Present in renderables: false, present in children: true, present in ButtonList: false + this.addWidget(button()); + // Present in renderables: false, present in children: true, present in ButtonList: false + this.addWidget(button()); + // Present in renderables: true, present in children: true, present in ButtonList: true + this.addRenderableWidget(button()); + // Present in renderables: true, present in children: false, present in ButtonList: true + this.addRenderableOnly(button()); + // Present in renderables: true, present in children: true, present in ButtonList: true + this.addRenderableWidget(button()); + // Present in renderables: true, present in children: true, present in ButtonList: true + this.addRenderableWidget(button()); + // Present in renderables: true, present in children: false, present in ButtonList: true + this.addRenderableOnly(button()); + } + }; + } + + private static Button button() { + return Button.builder(CommonComponents.EMPTY, _ -> { + }).build(); + } +}