Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Control.LayoutMode enum needs to be exposed for the layout_mode property #87374

Closed
geekroscom opened this issue Jan 19, 2024 · 21 comments
Closed

Comments

@geekroscom
Copy link

Tested versions

v4.2.1.stable.official [b09f793]

System information

Window 11 v4.2.1.stable.official [b09f793]

Issue description

When using item_label.layout_mode = 1, a warning message appears:

W 0:00:01:0356   Integer used when an enum value is expected. If this is intended cast the integer to the enum type.

but I can't find the corresponding available enumeration, such as LAYOUT_MODE_POSITION.

Steps to reproduce

var item_label:Label = Label.new()
item_label.layout_mode = LAYOUT_MODE_POSITION

Minimal reproduction project (MRP)

var item_label:Label = Label.new()
item_label.layout_mode = LAYOUT_MODE_POSITION
@geekroscom
Copy link
Author

I don't want to see the warning messages, and I need to know where you have hidden its enumeration?

@akien-mga
Copy link
Member

It seems like we forgot to bind the LayoutMode enum from the Control class, so indeed LAYOUT_MODE_POSITION isn't exposed in the API / docs.

@akien-mga akien-mga changed the title BUG:Integer used when an enum value is expected. If this is intended cast the integer to the enum type. Control.LayoutMode enum needs to be exposed for the layout_mode property Jan 19, 2024
@akien-mga akien-mga added this to the 4.3 milestone Jan 19, 2024
@akien-mga akien-mga self-assigned this Jan 19, 2024
@YuriSizov
Copy link
Contributor

It seems like we forgot to bind the LayoutMode enum from the Control class

No, we have not. You are not supposed to use these properties and these enums from scripting. They are internal.

@akien-mga
Copy link
Member

akien-mga commented Jan 19, 2024

Users keep trying to use those internal properties though, so I think we need to ask ourselves "why". Just telling them they're not supposed to use them doesn't seem sufficient.

  • Why can internal properties be used at all? It defeats the purpose of hiding them if we need to still support users using them.
  • Why do users keep trying to use them? How do they find them, and what do they allow? Do they work?
  • Why do we want to prevent users from using them?

I thought this was an oversight because the also internal anchors_preset has its enum fully exposed. But it seems it's used for some other method parameters, so it's just by chance I guess.

@AThousandShips
Copy link
Member

@akien-mga akien-mga removed this from the 4.3 milestone Jan 19, 2024
@akien-mga akien-mga removed their assignment Jan 19, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Jan 19, 2024

The problem here is that the property is exposed with PROPERTY_USAGE_INTERNAL, which AFAIK prevents them from being exposed to extensions and C#, but due to as far as I know an oversight this wasn't applied for GDScript

It was exposed, but the intention was likely to prevent this exposure

@geekroscom
Copy link
Author

If this enumeration is not suitable for public disclosure, then there shouldn't be any warning messages. I always prefer my programs to be cleaner. I have spent an entire afternoon dealing with and searching for an answer to this warning message.

@AThousandShips
Copy link
Member

It has an enum type because it's still stored, the proper solution is to not expose it at all as it is internal and doesn't work as a property, it only works when loading a scene in the editor

@dalexeev
Copy link
Member

dalexeev commented Jan 19, 2024

Personally, I was in favor of removing the layout_mode since the Position mode is the same as the Top Left preset. Plus, it would remove 2 extra clicks. See #69860 (comment) for details. But is it too late now due to compatibility?

@geekroscom
Copy link
Author

Then how should I deal with the layout issue? This belongs to the category where if not set, the layout becomes disorganized.

var font_path = load("res://framework/statics/fonts/msyh.ttc")
	var texture_normal = load("res://framework/statics/scenes/launch/server/server_button_0.png")
	var texture_pressed = load("res://framework/statics/scenes/launch/server/server_button_1.png")
	var texture_hover = load("res://framework/statics/scenes/launch/server/server_button_2.png")
	for i in range(len(server_list)):
		var item:TextureButton = TextureButton.new()
		var item_label:Label = Label.new()
		item.texture_normal = texture_normal
		item.texture_pressed = texture_pressed
		item.texture_hover = texture_hover
		item_label.text = server_list[i]["area_name"]
		item_label.horizontal_alignment = HORIZONTAL_ALIGNMENT_CENTER
		item_label.vertical_alignment = VERTICAL_ALIGNMENT_CENTER
		item_label.layout_mode = 1
		item_label.anchors_preset = PRESET_CENTER
		item_label["theme_override_font_sizes/font_size"] = "12"
		item_label["theme_override_colors/font_color"] = Color("#cba368")
		item_label["theme_override_fonts/font"] = font_path
		item.add_child(item_label)
		item.connect("pressed", _on_item_pressed.bind(server_list[i]["token"]))
		server_list_box.add_child(item)

@AThousandShips
Copy link
Member

AThousandShips commented Jan 19, 2024

It's internal and undocumented so not covered by compatibility, it's not part of the public API, except by how it is in the editor interface, and that can be changed too if it's not too disruptive

@AThousandShips
Copy link
Member

@geekros see the linked issue above for the topic of how this works and doesn't work

This property doesn't work and you need to handle these things in other ways, mainly with anchors

@geekroscom
Copy link
Author

item_label.layout_mode = 1

This can be correctly laid out, but a warning message will appear in the debugger:

W 0:01:18:0661   Integer used when an enum value is expected. If this is intended cast the integer to the enum type.
W 0:01:18:0661   Cannot assign 1 as Enum "LayoutMode": no enum member has matching value.

I don't want to see warning messages!

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2024

If this enumeration is not suitable for public disclosure, then there shouldn't be any warning messages. I always prefer my programs to be cleaner. I have spent an entire afternoon dealing with and searching for an answer to this warning message.

@geekros It's not enumerations which aren't supposed to be public. It's the properties themselves. There is no use in setting them or using them in any way from scripting. They are only hints for the editor and the inspector dock. They were created as properties because there was no other way to influence the flow of the inspector, but they have no meaning for you as a user when writing scripts.

You can just set the anchors to anything you want.

PS. Also instead of item_label["theme_override_font_sizes/font_size"] = "12" you should use item_label.add_theme_font_size_override("font_size", 12), and the same for other overrides.

@AThousandShips
Copy link
Member

The property doesn't actually work except in the position mode, it's just for the editor:

godot/scene/gui/control.cpp

Lines 889 to 906 in 7827c8e

void Control::_set_layout_mode(LayoutMode p_mode) {
bool list_changed = false;
if (data.stored_layout_mode != p_mode) {
list_changed = true;
data.stored_layout_mode = p_mode;
}
if (data.stored_layout_mode == LayoutMode::LAYOUT_MODE_POSITION) {
data.stored_use_custom_anchors = false;
set_anchors_and_offsets_preset(LayoutPreset::PRESET_TOP_LEFT, LayoutPresetMode::PRESET_MODE_KEEP_SIZE);
set_grow_direction_preset(LayoutPreset::PRESET_TOP_LEFT);
}
if (list_changed) {
notify_property_list_changed();
}
}

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2024

Users keep trying to use those internal properties though

@akien-mga Yes, they do keep finding them and try to use them. I know, I reply to every issue related to that 🙁 This issue permanently occupies my mind at this point.

  • Why can internal properties be used at all? It defeats the purpose of hiding them if we need to still support users using them.

I cannot speak for every internal property. The reality is, the internal hint is barely implemented, and we didn't really use it consistently in the past. The only logic associated with the internal hint that was ever implemented is hiding them from the docs. That's it.

However, this was the only way for me to implement the inspector flow that we designed. There is just no other way to influence the inspector to add a widget at an arbitrary place without creating a property. At least no straightforward way. So this is how I've implemented it, hoping we can resolve all the missing parts of internal properties.

But in reality, these have no purpose existing as properties. They were never meant to be properties. They have no logic associated with them which would be useful to users as properties. They are only hints for the inspector and they don't limit the API usage in any way. They only limit the look of the inspector.

  • Why do users keep trying to use them? How do they find them, and what do they allow? Do they work?

That's a good question. They are not documented, but they do come up in auto-completion. They also have a tooltip just like any other property in the inspector, which suggests you can use them by their name. This last part can be easily fixed though.

  • Why do we want to prevent users from using them?

As mentioned above, we don't want to prevent people using them. We just don't really want to expose them as properties, that was not the intention of them. And they have no practical use. You can set and get all layout properties at any time regardless of layout_mode and anchors_preset. They don't restrict that, only the inspector.

I thought this was an oversight because the also internal anchors_preset has its enum fully exposed. But it seems it's used for some other method parameters, so it's just by chance I guess.

It's the other way around, anchors_preset relies on the existing enum, which was already exposed for those other methods, yes.


The main issue is we cannot hide them from scripting at this point. It's a convoluted task, we discussed it a lot with core team members. Basically, we still need those properties to be settable and gettable, just not from scripting. Which is hard to implement, because how would you differentiate the two cases when both of them go through the same set()/get() logic?

We could still improve the inspector tooltip. And we could still adjust the code completion tools to hide them. It would probably not help C# users, though. So the best solution at this point is for me to hack the inspector in a way that would remove the need to have properties. I don't know how yet.

@geekroscom
Copy link
Author

I switched methods and used rectangles and position positioning to solve it, the warning messages are gone, thank you for your help.

var font_path = load("res://framework/statics/fonts/msyh.ttc")
var texture_normal = load("res://framework/statics/scenes/launch/server/server_button_0.png")
var texture_pressed = load("res://framework/statics/scenes/launch/server/server_button_1.png")
var texture_hover = load("res://framework/statics/scenes/launch/server/server_button_2.png")
for i in range(len(server_list)):
    var item:TextureButton = TextureButton.new()
    var item_label:Label = Label.new()
    item.texture_normal = texture_normal
    item.texture_pressed = texture_pressed
    item.texture_hover = texture_hover
    item_label.horizontal_alignment = HORIZONTAL_ALIGNMENT_CENTER
    item_label.vertical_alignment = VERTICAL_ALIGNMENT_CENTER
    item_label.text = server_list[i]["area_name"]
    item_label.size = Vector2(157, 24)
    item_label.position = Vector2(10, 11)
    item_label.add_theme_font_size_override("font_size", 12)
    item_label.add_theme_color_override("font_color", Color("#cba368"))
    item_label.add_theme_font_override("font", font_path)
    item.add_child(item_label)
    item.connect("pressed", _on_item_pressed.bind(server_list[i]["token"]))
    server_list_box.add_child(item)

@geekroscom
Copy link
Author

If this enumeration is not suitable for public disclosure, then there shouldn't be any warning messages. I always prefer my programs to be cleaner. I have spent an entire afternoon dealing with and searching for an answer to this warning message.

@geekros It's not enumerations which aren't supposed to be public. It's the properties themselves. There is no use in setting them or using them in any way from scripting. They are only hints for the editor and the inspector dock. They were created as properties because there was no other way to influence the flow of the inspector, but they have no meaning for you as a user when writing scripts.

You can just set the anchors to anything you want.

PS. Also instead of item_label["theme_override_font_sizes/font_size"] = "12" you should use item_label.add_theme_font_size_override("font_size", 12), and the same for other overrides.

I'll tell you why, as a new user, I chose to use item_label.layout_mode for layout. During the early design of our game, we needed to layout the interface first and then connect to interface data for layout rendering. This process, due to our unfamiliarity with Godot, naturally led us to assume that the configurations and properties adjusted directly in the GDScript would suffice. Hence, we encountered the problem we're discussing now.

For beginners, it's challenging to distinguish which properties are meant for GDScript or the Inspector, as they are all presented together. Now, can I assume that when I hover the mouse over a property and the documentation tooltip shows "No description available," that property or configuration should not be used in GDScript?

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2024

Now, can I assume that when I hover the mouse over a property and the documentation tooltip shows "No description available," that property or configuration should not be used in GDScript?

No, you shouldn't assume that. We don't have 100% documentation completion, so a missing description can just be that, a missing description. But I think I can fix the tooltip and make sure that for internal properties we show a special message.

@geekroscom
Copy link
Author

Now, can I assume that when I hover the mouse over a property and the documentation tooltip shows "No description available," that property or configuration should not be used in GDScript?

No, you shouldn't assume that. We don't have 100% documentation completion, so a missing description can just be that, a missing description. But I think I can fix the tooltip and make sure that for internal properties we show a special message.

Mm-hmm, doing so would be more user-friendly. Thank you for your efforts; my problem has been solved. Next, when setting properties, I will first check whether the attribute is included in the documentation. If it's not, I won't consider using it.

@akien-mga
Copy link
Member

Resolved by #87381.

@akien-mga akien-mga added this to the 4.3 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants