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

Fix script popup #286

Merged
merged 7 commits into from
Oct 23, 2024
Merged

Fix script popup #286

merged 7 commits into from
Oct 23, 2024

Conversation

manuq
Copy link
Contributor

@manuq manuq commented Oct 23, 2024

Here is how it looks now with dark and light themes:
Captura desde 2024-10-23 11-01-24
Captura desde 2024-10-23 11-00-34

This is better because it considers the editor single window mode.
The TextEdit control node had the background color hardcoded to black.
Remove black ColorRect node. Also remove the Scroll node because the
TextEdit containing the script can scroll automatically.

Add a PanelContainer and Panel node. And remove unneeded MarginContainer
nodes.

This makes the layout work as expected when resizing the window.
Previously the controls didn't shrink correctly (overflowing below the
window).
@manuq manuq requested a review from wjt October 23, 2024 14:09
Changing from TextEdit to CodeEdit shows a better background color,
making it more readable.
@manuq
Copy link
Contributor Author

manuq commented Oct 23, 2024

Update, changing from TextEdit to CodeEdit displays a better / more readable background:
Captura desde 2024-10-23 11-21-28
Captura desde 2024-10-23 11-22-07

@wjt
Copy link
Member

wjt commented Oct 23, 2024

This is a nice improvement though the code is still not always readable. For example, strings on a light theme are totally illegible:

Screenshot from 2024-10-23 16-30-36

I had the idea that it might be possible to reuse the syntax highlighting theme from the editor itself. It kind of is, with the caveat that it only works if you have at least one script open in the editor somewhere (even in a background tab). This is the result:

Screenshot from 2024-10-23 16-31-39

The colours are still not quite the same as in the real editor:

Screenshot from 2024-10-23 16-34-12

This is because we set the editable property to false. Spelunking through the TextEdit code, if I'm understanding correctly the problem is probably here:

https://github.com/wjt/godot/blob/f4942b735097c99b5e4882ec0b01b5a8df01a071/scene/gui/text_edit.cpp#L1307-L1308

At any given point, the text color's alpha channel is clamped to the font_readonly_color alpha, which happens to be 0.65.

I'll push another patch to fix that which gives this result which I'm quite satisfied with:

Screenshot from 2024-10-23 17-01-55
Screenshot from 2024-10-23 17-01-40

If there is at least one script open in the editor, we can steal its syntax
highlighter and apply it to the script window's CodeEdit control.

Otherwise, we continue to use a built-in SyntaxHighlighter.
Because the CodeEdit's readonly property is set, TextEdit forces the alpha
channel of all text rendered to be no higher than the alpha channel of the
default colour for text in disabled controls, which is 0.65 as I type this.

While we do not want the code in this popover to be editable, we do want
it to be legible.

Override the relevant font color on the control to set its alpha channel to 1,
effectively disabling this undesired clamping.
Copy link
Member

@wjt wjt left a comment

Choose a reason for hiding this comment

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

I also pushed a fixup for one of your commits.

WDYT?

@wjt
Copy link
Member

wjt commented Oct 23, 2024

BTW are you resizing the window when you take these screenshots?

If not… I wonder why the window is so narrow on my system. The window allegedly has a default size of 750×750 pixels which is not the case either on your system or on mine.

for x in script_editor.get_open_script_editors():
var control: Control = x.get_base_editor()
if control is TextEdit:
script_label.syntax_highlighter = control.syntax_highlighter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome!

Comment on lines +24 to +25
font_readonly_color.a = 1
script_label.add_theme_color_override("font_readonly_color", font_readonly_color)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good finding, I didn't know the read-only was doing that. Thanks for digging the sourcecode.

@manuq manuq merged commit add1611 into main Oct 23, 2024
3 checks passed
@manuq manuq deleted the fix-script-popup branch October 23, 2024 16:18
@manuq
Copy link
Contributor Author

manuq commented Oct 24, 2024

BTW are you resizing the window when you take these screenshots?

If not… I wonder why the window is so narrow on my system. The window allegedly has a default size of 750×750 pixels which is not the case either on your system or on mine.

No I didn't, instead I added a lot of blocks to make it scroll. Hmm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants