godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

TextEdit read-only panel display is inconsistent #10774

Open EmilyV99 opened 2 months ago

EmilyV99 commented 2 months ago

Describe the project you are working on

A UI tool

Describe the problem or limitation you are having in your project

368002981-749a9f49-0b7c-4d44-a205-9a525dc47532 TextEdit looks far different when "read-only" (editable = false) than most other control nodes, despite using identical theme settings to the LineEdit in this example. Notably, the TextEdit draws its' normal panel, and then draws the read-only panel as well; while other controls draw just the read-only panel. This looks wrong when the read-only panel has any transparency (which, notably, it does by default). I reported this as a bug here, because I assumed it would easily count as a bug to be fixed; but was directed here. The proposal is for TextEdit to be able to only draw the read-only panel, like basically all other controls do.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Without this, it's impossible to match the style of the TextEdit node with other controls while using transparency to indicate read-only controls.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Do what the other controls already do

If this enhancement will not be used often, can it be worked around with a few lines of script?

No idea how to work around this if you want to use transparency for the read-only panel.

Is there a reason why this should be core and not an add-on in the asset library?

It should work the same as the other core nodes.

kleonc commented 2 months ago

No idea how to work around this if you want to use transparency for the read-only panel.

As a workaround you could set readonly TextEdit's normal StyleBox override to a StyleBoxEmpty with the same content margins applied as the original normal StyleBox.

More or less (tweak to your needs):

var text_edit: TextEdit = ...
if not text_edit.editable:
    var normal_stylebox: StyleBox = text_edit.get_theme_stylebox("normal")
    var empty_stylebox := StyleBoxEmpty.new()
    empty_stylebox.content_margin_bottom = normal_stylebox.content_margin_bottom
    empty_stylebox.content_margin_left = normal_stylebox.content_margin_left
    empty_stylebox.content_margin_right = normal_stylebox.content_margin_right
    empty_stylebox.content_margin_top = normal_stylebox.content_margin_top
    text_edit.add_theme_stylebox_override("normal", empty_stylebox)

Note you might want to store the original normal_stylebox for restoring it on enabling editing etc.

EmilyV99 commented 2 months ago

More or less (tweak to your needs):

Thanks! <3 works perfectly. Still definitely feels like this shouldn't require a script to behave like all the other controls, regardless, but at least I can not worry about this now.

AThousandShips commented 2 months ago

You don't need a script, just change your theme

EmilyV99 commented 2 months ago

You don't need a script, just change your theme

How exactly does changing your theme help? I see no way that changing the theme can allow me to have a TextEdit that shows transparently when non-editable. If you're saying "just don't use transparency", that entirely misses the point, being that I want transparency.

AThousandShips commented 2 months ago

You said the code above worked perfectly, you can do the exact same from the editor by setting the style, I responded to that you said:

Still definitely feels like this shouldn't require a script to behave like all the other controls

EmilyV99 commented 2 months ago

You said the code above worked perfectly, you can do the exact same from the editor by setting the style, I responded to that you said:

Still definitely feels like this shouldn't require a script to behave like all the other controls

I couldn't simply change the theme though; I would have to change the theme override on each individual TextEdit every time I toggle the Editable checkbox to accomplish this.

Additionally, it should not be more complicated to toggle it via script than editable = false or editable = true; but it is. (Given, I made a subclass wrapping it, so it's as simple as disabled = true or disabled = false if using that wrapper; but you can't override the setter/getter for an internal node's properties directly, so setting editable directly will fail to update the style still...)

AThousandShips commented 2 months ago

That's true my bad, but you can prepare the theme externally as a custom theme item to make it easier to handle as a subclass, the simplest way would be to just create a custom theme class for your theme which uses this custom stylebox and change the theme_type_variation whenever you change the editable, this would be a simple compact piece of code and you could change this every time you change the editable status

As editable is never changed automatically it is only changed by code that means you always have some place where you set it to editable or clear it, in that place you could also do line_edit.theme_type_variation = &"LineEditEditable" if line_edit.editable else &"LineEditNonEditable" or just line_edit.theme_type_variation = &"" if line_edit.editable else &"LineEditNonEditable"

WhalesState commented 2 months ago

Let's just be honest, the TextEdit shouldn't draw the readonly over the normal style 😄 it's even mixing the calculation of both of them.

??? image