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

Apply set_read_only() to child classes of EditorProperty elements #51722

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Aug 16, 2021

Fixed #45769.

The read_only flag is in EditorProperty, so you can use it in the child classes of EditorProperty. According to the description, the read_only flag prevents operations in the inspector. However, if you don't set read_only for each GUI element implemented in the child class, they can manipulate the value in the inspector even if read_only == true.

Currently, it seems that the read_only flag is not commonly used, but for example, in #45699, it is necessary that read_only works correctly to prevent handling BonePose in RestMode. Also, the read_only flag can only be set in C++ currently, but it may be used in GDScript in the future with some way. So I fixed this.

2021-08-16.17.13.24.mov

@TokageItLab TokageItLab requested review from a team as code owners August 16, 2021 08:22
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch 3 times, most recently from 1c7fc10 to 425fea3 Compare August 16, 2021 08:37
@YeldhamDev YeldhamDev added this to the 4.0 milestone Aug 16, 2021
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch 5 times, most recently from 4f8abcb to 0f9ed37 Compare August 20, 2021 00:25
@KoBeWi
Copy link
Member

KoBeWi commented Aug 20, 2021

Disabled properties should have revert arrow unavailable probably.

EDIT:
Also their names should be greyed out. Right now for some properties it's difficult to tell whether they are editable or not (although this is also a styling problem).

EDIT2:
Seems like only EditorSpinSlider has style problem. Its font is always white. Fixing this would be enough, in case the greyed-out names turn out difficult to do. EDIT3: This is issue with font_readonly_color. It's like this, because audio bus editor needed it (#51114). I'd remove it in favor of font_disabled_color and add override to bus editor, but it can be tweaked in another PR.

Also I don't see any way to disable selected properties, only whole inspector can be disabled. This would be useful.

@TokageItLab TokageItLab marked this pull request as draft August 20, 2021 16:40
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 0f9ed37 to 3afcc2b Compare August 21, 2021 02:29
@TokageItLab TokageItLab marked this pull request as ready for review August 21, 2021 02:29
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 21, 2021

The revert button is now hidden when is_read_only()==true.

As for LayerGrid and Ease, the input processing is implemented in editor_properties.cpp, so I change the input processing is controlled by the value of is_read_only().

Also I don't see any way to disable selected properties, only whole inspector can be disabled. This would be useful.

I think it is necessary to separate methods such as is_all_read_only() and is_read_only(idx) (also set_read_only()) in order to control them separately. What do you think about it? @KoBeWi

The rest is a matter of style color. As for Easing, I used font_uneditable_color for the number value color when readonly, but the line color is not changed. Also, the base color of the LayerGrid may need to changed. Also label color. I think it is debatable.

Edited:
I changed label color, grid color, and Easing line color. If you feed Easing line color is too clear, it will be fixed by changing font_disabled_color, since the line color use font_disabled_color.

スクリーンショット 2021-08-21 14 58 04

@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch 3 times, most recently from 2a8c189 to 7052b50 Compare August 21, 2021 05:57
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 87d0b8e to a5e527c Compare August 22, 2021 14:11
editor/editor_inspector.cpp Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 23, 2021

@groud In other words, the only thing we need to implement here is a virtual function called set_editable(), for example, and in update_tree()

EditorProperty->set_editable(!(EditorProperty->is_readonly() || EditorInspector->is_readonly()));

should I do this, right?

@groud
Copy link
Member

groud commented Aug 23, 2021

@groud In other words, the only thing we need to implement here is a virtual function called set_uneditable(), for example, and in update_tree()
if (EditorProperty->is_readonly() || EditorInspector->is_readonly()) {
EditorProperty->set_uneditable();
}
should I do this, right?

Not really, no.

To make it clear, this PR has a lot of collisions with #30623. That PR adds a PROPERTY_USAGE_READ_ONLY flag that is used to determine if a property is read_only or not. You should add it in this PR too, and then, when building the tree, you should do something like:

bool property_read_only = (p.usage & PROPERTY_USAGE_CATEGORY) || read_only;
...
ep->set_read_only(property_read_only);

@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 23, 2021

@groud PROPERTY_USAGE_READ_ONLY flag in #30623 is implemented in the Control object, and the flag is implemented with hardcoding and private. With that implementation, I think it cannot to make it readonly individually by referring to a child class of EditorProperty from outside. As use case

in #45699, it is necessary that read_only works correctly to prevent handling BonePose in RestMode.

@groud
Copy link
Member

groud commented Aug 23, 2021

@groud PROPERTY_USAGE_READ_ONLY flag in #30623 is implemented in the Control object, and the flag is implemented with hardcoding and private.

I am not sure what you mean, the flag is added to object.h, so that any object can set its property to read_only if needed. BTW, the _validate_property(PropertyInfo &property)` function is here for that, it is called when listing the object properties so that you can dynamically update the flags of a property. (depending on a property's value, some fields might be disabled for example).

With that implementation, I think it cannot to make it readonly individually by referring to a child class of EditorProperty from outside.

I don't think why you would need that anyway. It's not a reliable way to identify EditorProperty nodes (at least in the inspector). If you need something more complex than using the default way the inspector does things (by creating EditorProperties out of the list of property), then you need to code the logic inside an inspector plugin. Otherwise, simply updating the nodes properties with the correct flags (in _validate_property(PropertyInfo &property)) should be fine.

@TokageItLab TokageItLab requested a review from a team as a code owner August 23, 2021 12:16
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 1096ca3 to 22135c7 Compare August 23, 2021 12:29
@TokageItLab
Copy link
Member Author

@groud Thank you for the explanation, now I understood it somehow. For example, the current SkeletonGizmo needs to be set to readonly by a specific edit mode instead of specific object state. In that case, it should implement _validate_property() in skeleton_3d.cpp to get the edit mode and set the flag, is it correct?

Fixed:

  • The _set_read_only() remains as a virtual function to control each handling in child classes, but it no longer to call the base method.
  • global_read_only has been omitted.
  • PROPERTY_USAGE_READ_ONLY flag is now implemented, and the inspector determines from the object property and the inspector whether it is readonly or not, and controls the handling with EditorProperty.

@groud
Copy link
Member

groud commented Aug 23, 2021

In that case, it should implement _validate_property() in skeleton_3d.cpp to get the edit mode and set the flag, is it correct?

It seems correct. I am not very familiar with how skeleton_3d.cpp work, but if you have a way to know in which mode you are editing (I guess it would set a property on Skeleton3D, maybe the "pose" one ?). Then it should be quite easy to implement.

Otherwise you may add a setter for a new property within Skeleton3D, that would only impact whether or not part of its properties are read-only.

@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 22135c7 to 15087fb Compare August 23, 2021 12:51
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch 3 times, most recently from 3062f95 to e07d91f Compare August 23, 2021 17:18
@TokageItLab TokageItLab requested review from groud and removed request for a team August 26, 2021 17:22
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 58917a9 to 1777035 Compare August 28, 2021 03:44
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from 1777035 to ec2c636 Compare September 5, 2021 00:03
@TokageItLab TokageItLab force-pushed the implement-set-read-only-in-extended-class branch from ec2c636 to facf8f1 Compare September 5, 2021 09:22
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Alright, LGTM

@groud groud merged commit c6f6c05 into godotengine:master Sep 5, 2021
@groud
Copy link
Member

groud commented Sep 5, 2021

Thanks!

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.

set_read_only() in EditorProperties doesn't work correctly
4 participants