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

Bind Node::force_parent_owned() as make_owned_by_parent() and bind Node::is_owned_by_parent() as is_managed_by_parent(). #89388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch 2 times, most recently from 2b375dc to 7c4aac6 Compare March 12, 2024 06:55
@Daylily-Zeleen Daylily-Zeleen changed the title Bind Node::force_parent_owned() for plugin development. Add Node::set_owned_by_parent() and bind new property owned_by_parent. Mar 12, 2024
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch 2 times, most recently from 7fbe8f8 to c8bd229 Compare March 12, 2024 07:02
@Daylily-Zeleen
Copy link
Contributor Author

I decide to bind a new property owned_by_parent to decide a node is an internal node of parent or not and can be changed later by users, instead of exposing force_parent_owned() which can set it to true only.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch 2 times, most recently from fc527ac to d81b81f Compare March 12, 2024 10:33
@AThousandShips
Copy link
Member

I see no valid reason to be able to clear this value, and it creates a huge risk for unintended errors IMO

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch from d81b81f to eef3bfa Compare March 12, 2024 11:12
@Daylily-Zeleen
Copy link
Contributor Author

I see no valid reason to be able to clear this value, and it creates a huge risk for unintended errors IMO

Like my comment in #9282.
This change dose not change any existing logic, user who user this new feature should take responsibility for themselves (like EditorScript may lead to crash). So I don't think this change will bring any risk.

If I neglect something, please tell me, I will add a new field (may be named "owned_by_parent_override") to handle this feature.

@AThousandShips
Copy link
Member

This change dose not change any existing logic

It does, you can't clear this, only set it

Sure you can break things in ways, but I don't think we should add features that make it easier

What's the use case for clearing this value? The setting the value is reasonable, but there's no reason I can see to clear it

@Daylily-Zeleen
Copy link
Contributor Author

What's the use case for clearing this value? The setting the value is reasonable, but there's no reason I can see to clear it

A little confused, is the "clearing this value" you mentioned equal to data.parent_owned = false?

@AThousandShips
Copy link
Member

Yes, that's what I mean :) there's no way to set it to false currently, because it's not needed, or safe

@Daylily-Zeleen
Copy link
Contributor Author

Maybe a internal node which need processed, and take it out as a regular node at some situation (maybe extract a internal node and make it to be a regular node by a tool button).
This example is not very convincing, purely as a possibility, but it ultimately depends on the plugin developer.

Like ScrollContainer, if user clear ScrollBar's data.parent_owned, although this is strange, we cannot assume that it is wrong. In other words, we cannot assume the behavior of developers.

To be honest, I didn't think much about clearing this value. I just want it to become an property for convenience (both get and set).

@AThousandShips
Copy link
Member

Let's keep it as a method and not a property, there's no reason to clear it, and it is wrong in my opinion, there's no reason to ever clear it, it's not a matter of assuming, it's a matter of what this property is for and what it makes sense for

If you want to clear it all you need to do is reparent it

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch from eef3bfa to 185585b Compare March 12, 2024 15:18
@Daylily-Zeleen Daylily-Zeleen changed the title Add Node::set_owned_by_parent() and bind new property owned_by_parent. Bind Node::force_parent_owned() as make_owned_by_parent() and bind Node::is_owned_by_parent(). Mar 12, 2024
doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/Node.xml Show resolved Hide resolved
doc/classes/Node.xml Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch from 185585b to 613f067 Compare March 12, 2024 15:58
KoBeWi
KoBeWi previously approved these changes Mar 12, 2024
<return type="void" />
<description>
Marks the node as being managed by its parent. Parent managed nodes are not duplicated or copied when calling [method duplicate] or [method replace_by].
[b]Note:[/b] You can reparent a managed node to make it become a regular node.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is necessary to mention

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[b]Note:[/b] You can reparent a managed node to make it become a regular node.
[b]Note:[/b] Reparenting this node resets the managed status.

If we need to keep it

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 12, 2024
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch from 613f067 to 552cf9c Compare March 12, 2024 16:12
@Daylily-Zeleen Daylily-Zeleen changed the title Bind Node::force_parent_owned() as make_owned_by_parent() and bind Node::is_owned_by_parent(). Bind Node::force_parent_owned() as make_owned_by_parent() and bind Node::is_owned_by_parent() as is_managed_by_parent(). Mar 12, 2024
`Node::is_owned__by_parent()` as `is_managed_by_parent()`
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/bind_force_parent_owned branch from 552cf9c to 240d1a3 Compare March 12, 2024 16:15
@KoBeWi KoBeWi dismissed their stale review March 13, 2024 00:19

On a second though there might be a better solution. See my comment in the proposal.

@akien-mga akien-mga modified the milestones: 4.3, 4.x May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose Node::force_parent_owned() to allow avoid duplicate nodes for plugin development
4 participants