Prevent editing properties managed by parent container

[Awkor]

Addresses #28718 by making the properties that are managed by parent container not editable inside the inspector.

Currently the properties that get disabled are:

• Anchor
• Margin
• Position
• Scale
• Size

This is achieved by disabling mouse interactions and preventing focus on the aforementioned inspector properties.

This is how it looks when properties get disabled:

Bugsquad edit: Fixes #28718 and fixes godotengine/godot-proposals#2202.

[groud]

This function looks a little bit dirty. Most buttons (and hopefully spinboxes) have a disabled property, it might make sense to use that instead of using a mouse filter and changing focus mode.

[Awkor]

Currently the inspector uses EditorSpinSliders which don't have a way to be disabled (I tried setting them as read-only but could still edit them if I got focus through tabbing). I'll use the same approach used in SpinBox and add set_editable and is_editable to EditorSpinSlider (if it's considered a good approach).

Here's the code used when trying to use the read-only approach:

void EditorProperty::disable() {
	// Make it visually apparent that the property is disabled
	Color color = get_modulate();
	color.a = color.a * 0.5;
	set_modulate(color);
	// Make it non editable
	set_mouse_filter(MOUSE_FILTER_IGNORE);
	set_focus_mode(FOCUS_NONE);
	int child_count = get_child_count();
	for (int i = 0; i < child_count; i++) {
		Node *child = get_child(i);
		Container *container = Object::cast_to<Container>(child);
		if (container) {
			container->set_mouse_filter(MOUSE_FILTER_IGNORE);
			container->set_focus_mode(FOCUS_NONE);
			int container_child_count = container->get_child_count();
			for (int j = 0; j < container_child_count; j++) {
				Node *container_child = container->get_child(j);
				EditorSpinSlider *spin_slider = Object::cast_to<EditorSpinSlider>(container_child);
				if (spin_slider) {
					spin_slider->set_read_only(true);
				}
			}
		} else {
			EditorSpinSlider *spin_slider = Object::cast_to<EditorSpinSlider>(child);
			if (spin_slider) {
				spin_slider->set_read_only(true);
			}
		}
	}
}

And here's a video to show the behavior:

[groud]

add set_editable and is_editable to EditorSpinSlider

Yes, that's likely the best solution IMHO.

[groud]

I completely agree with the proposed feature, only the implementation needs a rework. :)

[PostPollux]

Thanks! I'm really looking forward to this feature!
This was so confusing and it drove me crazy once in the past. I wasn't that familiar with Godot at that time and I used a margin container. Whenever I changed some margin values and started the game, it didn't work. I took me a while to figure out to use the "custom constant" values. I think it's much less confusing for beginners if those values will be disabled!

[TheDuriel]

Perhaps we can expand this and flip it around so that the expand flags are disabled unless the node is in a container?

[PostPollux]

Any news on this one?

[Awkor]

Hey, sorry for not giving any updates. I'm available to apply any needed modifications to make this pull request mergeable.

[YuriSizov]

• Anchor
• Margin
• Position
• Size

Note that rect_scale should be there too: #38356

[groud]

Note that rect_scale should be there too: #38356

No sorry, I just realized that I might be wrong there. rect_scale is not overridden by containers, so I am not sure it should be disabled. However this property tends to mess with children of containers, but that's another problem (see #19068).

[YuriSizov]

@groud When I was testing the case in #38356, the scale was reset on visibility toggle. So, a child of HBox clearly was not controlling it. So, who does, if not the container?

Edit:
This is what Container::fit_child_in_rect does:

	p_child->set_position(r.position);
	p_child->set_size(r.size);
	p_child->set_rotation(0);
	p_child->set_scale(Vector2(1, 1));

So, yeah, it should be disabled (along with rotation, it seems).

[groud]

Interesting. I think this was not done before, maybe this was modified. I am not sure what should be done there, maybe the scale should be reset for all containers, but I am not sure it is the case.

[TheDuriel]

Scale was certainly reset (by the Container) in the past.

[groud]

Ah yes you are right, so indeed rect_scale should be added to the list of non-editable properties then.

[Awkor]

rect_scale added as a non-editable property.

[aaronfranke]

@Awkor This needs to be rebased on the latest master branch when you have the chance.

[Awkor]

@aaronfranke Done.

[YuriSizov]

You have formatting issues, namely empty lines at the start of some functions.

[DoctorWhoof]

Sorry to disrupt the productive conversation, but I just wanted to say that I was about to request this and I agree 100% that it’ll make beginner’s lives easier. It took months for Godot’s Controls to “click”with me, and I think the lack of this feature is partially responsible for the steep learning curve.

Thanks for making it happen!

[YuriSizov]

I think we can use this as a solution and address concerns voiced by @KoBeWi with a future PR. If a new method was to be added to the Object class it should be something like get_managed_property_list which would return property names and, I guess, the name of a class that controls that property if the current node is its child. But this can also be added to the good old get_property_list, and maybe even to the ADD_PROPERTY bindings, which would likely be more consistent.

This would complicate things, however, and the problem that this PR fixes is very much annoying to a lot of new users. We can clean up the backend later, I think, if we address the UX now.

---

Also, @groud, can you please check if your concerns were addressed and update your review?

[groud]

Also, @groud, can you please check if your concerns were addressed and update your review?

Well, there are a things do not seem correct to me with the current implementation. Few points:

• I would prefer this disabling mechanism to be generic. Control nodes' properties are not the only ones that would benefit from a disabling mechanism. As I mentioned in my review, I'd rather go with a (likely virtual) function to disable any EditorProperty. Thus we should add something like set_disabled(bool p_disabled) to the EditorProperty class, then implement it for the different classes extending EditorProperty.
• I quite dislike the fact we hardcode things into EditorInspector::update_tree, the editor inspector is supposed to be agnostic to what it handles. Consequently, we should probably use an inspector plugin instead, or add a way to set a property as disabled.
• EditorProperty exposes a set_read_only(bool p_read_only) function, maybe it could be replaced by the new set_disabled I proposed. That's a lilttle bit out of the scope of this PR, but it might be good to review where set_read_only is used and if it would be equivalent to use set_disabled.

In the current state, the PR does the job well but is quite hacky. We still have some time before 4.0, so I'd rather have this made in a cleaner way.

[AaronRecord]

See also godotengine/godot-proposals#2841

[KoBeWi]

There is #51722 which adds a proper set_read_only() to every EditorProperty. It would be useful for this PR.

[Awkor]

There is #51722 which adds a proper set_read_only() to every EditorProperty. It would be useful for this PR.

If it gets merged, I'll rebase and use that.

[TokageItLab]

@Awkor #51722 has been merged, so I think you can rebase with it and resume this PR. Regards,

[Awkor]

Done.

[TokageItLab]

It seems to be needing to include container.h in control.cpp.

[groud]

OK looks good to me. Let's merge this!
Thanks!

[p10tr3k]

Guys, I am not sure if this is still working correctly in 3.5.1.

Should Margin editor be disabled? Because it is not and the margin property is being reset.

[KoBeWi]

The issue is not fixed in 3.5, because the fix depends on #51722, which was merged only on master.