Skip to content

Conversation

@MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented Mar 22, 2023

Fixes #176, which was caused by SearchFieldEntry.children() using List.of() to form the list of children.

List.of() returns an unmodifiable list, which doesn't permit null members. This means that List.indexOf() will throw a NullPointerException when null is passes to it, reveals a bug in the vanilla method ContainerEventHandler#changeFocus(boolean).

ContainerEventHandler.changeFocus() with comments regarding the vanilla bug
// decompiled using loom & fernflower, manually cleaned & commented
boolean changeFocusFernFlower(boolean moveForward) {
    GuiEventListener currentlyFocused = this.getFocused();
    boolean focusedIsNotNull = currentlyFocused != null;
    
    // First try letting the currently focused gui handle changing focus
    if (focusedIsNotNull && currentlyFocused.changeFocus(moveForward)) {
        return true;
    }
    
    // Otherwise iterate over the children
    List<? extends GuiEventListener> list = this.children();
    
    // We want to start iterating from the next/previous gui to `currentlyFocused`
    // This is where the crash occurs - if `currentlyFocused` is null & `list` does not allow null entries:
    int index = list.indexOf(currentlyFocused); // NullPointerException
    int start;
    // This is a vanilla bug:
    // list.indexOf() should only be run if focusedIsNotNull
    if (focusedIsNotNull && index >= 0) {
        start = index + (moveForward ? 1 : 0);
    } else {
        start = moveForward ? 0 : list.size();
    }
    /* Vanilla fix:
    // Mojang could fix this by only calling indexOf() when `focusedIsNotNull`:
    int index, start;
    if (focusedIsNotNull && (index = list.indexOf(guiEventListener)) >= 0) {
        start = index + (moveForward ? 1 : 0);
    } else {
        start = moveForward ? 0 : list.size();
    }
     */

    // Get an iterator starting at the appropriate index
    ListIterator<? extends GuiEventListener> iterator = list.listIterator(start);

    // Get a reference to either iterator.hasNext() or iterator.hasPrevious()
    BooleanSupplier hasAnother;
    if (moveForward) {
        Objects.requireNonNull(iterator);
        hasAnother = iterator::hasNext;
    } else {
        Objects.requireNonNull(iterator);
        hasAnother = iterator::hasPrevious;
    }

    // Get a reference to either iterator.next() or iterator.previous()
    Supplier<? extends GuiEventListener> another;
    if (moveForward) {
        Objects.requireNonNull(iterator);
        another = iterator::next;
    } else {
        Objects.requireNonNull(iterator);
        another = iterator::previous;
    }

    // Iterate until a gui accepts focus, or we run out of guis
    GuiEventListener gui;
    do {
        if (!hasAnother.getAsBoolean()) {
            this.setFocused(null);
            return false;
        }
        
        gui = another.get();
    } while(!gui.changeFocus(moveForward));

    this.setFocused(gui);
    return true;
}

This PR fixes the issue by having SearchFieldEntry.children() use Collections.singletonList() instead of List.of(). Singleton lists permit null values, so indexOf() doesn't need to throw any exceptions when we ask for the index of null.

I've also included a couple commits fixing mapping names I stumbled across while debugging. I can open a separate PR for those if you like, but I figured so long as the commits don't get squashed together when merging one small PR should be ok. EDIT: moved to #202

`SearchFieldEntry.children()` was returning `List.of(editBox)`, which itself returns an "unmodifiable" list.

Unmodifiable lists do not permit null entries, and [`indexOf()` will throw a `NullPointerException`][1] if the specified element is `null` and the list does not permit `null` elements.
This reveals a bug in vanilla's `ContainerEventHandler#changeFocus(boolean)` method, where `indexOf()` is called without a null-check, causing the crash.

Instead, we can use `Collections.singletonList()` which returns a `SingletonList`. Still immutable, but no silly NPEs.

Fixes shedaniel#176

[1]: https://docs.oracle.com/javase/8/docs/api/java/util/List.html#indexOf-java.lang.Object-
Copy link
Owner

@shedaniel shedaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@5HT2
Copy link

5HT2 commented Mar 24, 2023

@shedaniel merge?

@MattSturgeon

This comment was marked as duplicate.

@MattSturgeon

This comment was marked as duplicate.

@shedaniel
Copy link
Owner

Thanks for reminding me! I will merge it this weekend!

@MattSturgeon

This comment was marked as duplicate.

@MattSturgeon
Copy link
Contributor Author

@shedaniel it'd be great to get this merged 😄

@shedaniel shedaniel merged commit 461ae8e into shedaniel:v8 Dec 21, 2023
shedaniel pushed a commit that referenced this pull request Dec 21, 2023
`SearchFieldEntry.children()` was returning `List.of(editBox)`, which itself returns an "unmodifiable" list.

Unmodifiable lists do not permit null entries, and [`indexOf()` will throw a `NullPointerException`][1] if the specified element is `null` and the list does not permit `null` elements.
This reveals a bug in vanilla's `ContainerEventHandler#changeFocus(boolean)` method, where `indexOf()` is called without a null-check, causing the crash.

Instead, we can use `Collections.singletonList()` which returns a `SingletonList`. Still immutable, but no silly NPEs.

Fixes #176

[1]: https://docs.oracle.com/javase/8/docs/api/java/util/List.html#indexOf-java.lang.Object-
@MattSturgeon MattSturgeon deleted the fix-176 branch December 21, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trying to navigate using TAB key in config menu crashes the game

3 participants