Add icon_modulate set/get functionality to PopupMenu

[the-sink]

Copies the get_item_icon_modulate and set_item_icon_modulate functionality already present in the ItemList class over to PopupMenu. I need this to be able to give different colors to items that have the same icon in a dropdown, so I'm guessing others may find the functionality useful as well?

[bruvzg]

New methods should be exposed to the GDScript (add the following code to the PopupMenu::_bind_methods) and documented (see Contributing to the class reference).

ClassDB::bind_method(D_METHOD("set_item_icon_modulate", "index", "modulate"), &PopupMenu::set_item_icon_modulate);
ClassDB::bind_method(D_METHOD("get_item_icon_modulate", "index"), &PopupMenu::get_item_icon_modulate);

[Chaosus]

@the-sink You need to squash the commits (see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#modifying-a-pull-request)

[the-sink]

@Chaosus Rebased, and... reading through all the contributing docs is probably in order! Appreciate it.

[KoBeWi]

Suggested change

	// Multiply icon color by the custom modulate color
	// Multiply icon color by the custom modulate color.

Although I think this comment isn't necessary.

[the-sink]

Thanks, I agree the comment is unnecessary - will go ahead and remove it.

[KoBeWi]

The PR looks ok functionality-wise, but it's not linked to any proposal (although I get how parity with ItemList somewhat justifies it).

I think it's missing being exposed to the inspector; you can only set modulate from code. Check how other properties are exposed and you could do the same.

[YuriSizov]

The PR looks ok functionality-wise, but it's not linked to any proposal

It's for godotengine/godot-proposals#768, to at least unlock that ability so the proposal can be implemented as a plugin.

[the-sink]

I think it's missing being exposed to the inspector; you can only set modulate from code. Check how other properties are exposed and you could do the same.

Referring to the icon_modulate for individual PopupMenu items? I agree that would be good to expose to the inspector, although as far as I can tell ItemList doesn't expose that option either. Do you think it would be better to expose the parameter for both classes in a separate PR or just do it here?

[KoBeWi]

Do you think it would be better to expose the parameter for both classes in a separate PR or just do it here?

I remember ItemList has many properties that were not exposed to not bloat the list, so maybe it's better to not do it at all. I'd wait until there is some actual demand.

[KoBeWi]

Looks fine, aside from the comment I commented about.

[akien-mga]

Needs a rebase, then it should be ready to merge :)

[the-sink]

Should be good now!

[akien-mga]

You need to validate the index like in all other PopupMenu setters:

if (p_idx < 0) {
	p_idx += get_item_count();
}
ERR_FAIL_INDEX(p_idx, items.size());

The CI caught a crash here:

GDSCRIPT CODE:     PopupMenu.new().set_item_icon_modulate(100, Color(100, 100, 100, 1))
ERROR: FATAL: Index p_index = 100 is out of bounds (size() = 0).
   at: get (./core/templates/cowdata.h:155)

[the-sink]

Oops, didn't notice during the rebase, thanks!

[akien-mga]

Thanks!