Handle changing ItemLists from signals

[hpvb]

When an ItemList changes in response to a UI event on that ItemList we could now potentially have an out of date index into the ItemList cached.

After the signal gets emited we now re-check the index and the state of the list of items.

This fixes #100663

[hpvb]

Copied from the issue:

in void ItemList::gui_input(const Ref<InputEvent> &p_event)

we detect what item is clicked by

int closest = get_item_at_position(mb->get_position(), true);

But then on line emit_signal(SNAME("multi_selected"), i, true);

We emit a signal, and in the project with the error the signal handler for this clears items

So we then

if (items[i].selectable && select_mode == SELECT_TOGGLE) {

Which is where the crash happens, at this point the closest value is still set to 1, or 2, but items has been cleared.

[akien-mga]

What PR caused the regression? It's easier to assess the fix in context of what made it necessary.

[hpvb]

What PR caused the regression? It's easier to assess the fix in context of what made it necessary.

I will find the pr, and link it again tomorrow, but basically what it did was add this extra if block after the signal. Prior to the pr there was no extra check, so we were not crashing, but sending a bogus index in the signal.

[hpvb]

The regression was caused by #99355 notice how there is a second conditional after the signal fired that was previously not there.

Also notice that without that secondary if block, the data in the second signal was garbage.

[akien-mga]

CC @havi05 @KoBeWi

[havi05]

I tried to reproduce the error, but wasn't able to do so. (Does this only appear on the mono version?) Clearing all Items when the multi_selected signal fired didn't cause any errors on my side. (using 4.4dev7)

I also don't understand how the first if-condition in line 749 can modifiy the list of Items, because it checks if select_mode != SELECT_TOGGLE is true and the second (new) one (line 770) checks the opposite.

Edit: I understand now where the issue is appearing. (The first check in line 770 is problematic. ) I think I would prefer to solve it this way:

if (select_mode == SELECT_SINGLE) {
	if (items[i].selectable && (!items[i].selected || allow_reselect)) {
		select(i, select_mode == SELECT_SINGLE || !mb->is_command_or_control_pressed());
		emit_signal(SceneStringName(item_selected), i);

} else if (select_mode == SELECT_TOGGLE) {
	if (items[i].selectable) {
		if (items[i].selected) {
			deselect(i);
			current = i;
			emit_signal(SNAME("multi_selected"), i, false);

		} else {
			select(i, false);
			current = i;
			emit_signal(SNAME("multi_selected"), i, true);
		}
	}
} else {
	emit_signal(SNAME("multi_selected"), i, true);
	}
}

Personally I think this would be better readable than a return mid function. What do you think?

Since I'm on vacation it is difficult for me to debug this, but I will try my best to help you.

[hpvb]

@havi05 Sure I can do that too, I think generally we prefer early exits in Godot, that's why it is like this. But I can make it an if/then branch also.

@KoBeWi @akien-mga any preferences?

Note also that it doesn't solve the pre-existing problem of emit_signal(SNAME("item_clicked"), i, get_local_mouse_position(), mb->get_button_index()); being garbage if the new signal changes the ItemList. So we still need the code I wrote before that.

[KoBeWi]

This check could be moved to get_item_at_position() and return -1.
Then you could return if i is -1.

[KoBeWi]

tbh the crash can be fixed by just inverting the condition:

if select_mode == SELECT_TOGGLE && items[i].selectable

The code currently checks for the opposite thing independently, i.e.
(items[i].selectable && (!items[i].selected || allow_reselect) && select_mode != SELECT_TOGGLE) items[i].selectable && select_mode == SELECT_TOGGLEthese 2 lines are mutually exclusive, because the mode is either SELECT_TOGGLE or not, so this begs to add anelse` somewhere.

Or just use the code from havi, it looks cleaner.

Also I'd avoid calling get_item_at_position() just in case the user messed the list; it's unnecessary calculation. If the index can be invalidated, just do ERR_FAIL_INDEX(i, items.size()) with a message that modifying list in signal callback is not supported and you should use a deferred call.

[hpvb]

If we don't want to support people changing the selection mode etc from inside the handler then this can be done differently, sure. I will make a revision.

[hpvb]

@KoBeWi pretty sure this is fixed properly now.

[havi05]

LGTM.

[akien-mga]

Thanks!