mirror of
https://github.com/FabricMC/fabric
synced 2026-08-19 06:26:07 -04:00
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
This commit is contained in:
Fuzss
GitHub
parent
1f023a9e6c
commit
ffbe971929
3 changed files with 310 additions and 73 deletions
|
|
@ -38,88 +38,110 @@ public final class ButtonList extends AbstractList<AbstractWidget> {
|
|||
|
||||
@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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -55,7 +55,7 @@ abstract class ScreenMixin implements ScreenExtensions {
|
|||
private List<Renderable> renderables;
|
||||
|
||||
@Unique
|
||||
private ButtonList fabricButtons;
|
||||
private List<AbstractWidget> fabricButtons;
|
||||
@Unique
|
||||
private Event<ScreenEvents.Remove> removeEvent;
|
||||
@Unique
|
||||
|
|
|
|||
|
|
@ -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<AbstractWidget> widgets = Screens.getWidgets(screen());
|
||||
assertEquals(7, widgets.size());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testAdd() {
|
||||
List<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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<AbstractWidget> 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();
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue