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

Syntax highlighter refactor #38440

Merged
merged 4 commits into from
Jul 14, 2020

Conversation

Paulb23
Copy link
Member

@Paulb23 Paulb23 commented May 3, 2020

First part of #31739

Started as part of #17923 however as we could not break compatibility full separation was not possible. This PR completed that work leaving the base concept intact.

Firstly, the SyntaxHighlighterclass has been converted into a resource and exposed to GDScript. All syntax highlighting code is now managed by this class not TextEdit.

The return of _get_line_syntax_highlighting has been changed to Dictionary<int, Dictionary> instead of the struct HighlighterInfo to allow use in GDScript, currently the only supported value in this Dictionary is the key 'color'.

The TextEdit changes are as follows:

  • New property to set the syntax highlighter, the bound methods for which have been renamed from get/set_syntax_highlighting to get/set_syntax_highlighter.
  • The syntax_coloring property has been removed.
  • All syntax highlighting colours in themes, properties and methods have been removed.
  • A new signal line_edited_from with the param of the changed line number.
  • The current highlighting code now exists in the CodeHighlighter syntax highlighter resource.

On the editor side a new EditorSyntaxHighlighter resource extending SyntaxHighlighter has been created for use in the script editor. This class and the get/set methods have been exposed for tool scripts. There are now three editor highlighters:

  • EditorStandardSyntaxHighlighter
  • EditorPlainTextSyntaxHighlighter
  • GDScriptSyntaxHighlighter

Lastly, It is now also possible to edit the syntax highlighter in the inspector.

Closes godotengine/godot-proposals/issues/597
Closes #33094
Part of #37540

@YuriSizov
Copy link
Contributor

Thank you very much!

I know that this breaks compatibility in some parts, but given the nature of this change and the careful way it was implemented, how possible it is to be included in 3.x at some point before 4.x release? I'd argue, that general uses of TextEdit are intact and only those who developed some devtools/plugins are affected. And this crowd (myself included) can adjust their codebase.

@Zylann
Copy link
Contributor

Zylann commented May 5, 2020

The return of _get_line_syntax_highlighting has been changed to Dictionary<int, Dictionary> instead of the struct HighlighterInfo to allow use in GDScript, currently the only supported value in this Dictionary is the key 'color'.

I got a bit surprised by this. Any reason why it should be a nested dictionary? Right now for a highlighter, forcing to create plenty of small dictionaries with string keys inside just to represent colors is calling for a lot of unnecessary allocations. At least if the goal is to allow more data (but which data?) it could use Variant, or have a wrapper object to fill in that data (like SurfaceTool does).
In the case of C# it would also cause more GC.

@Paulb23
Copy link
Member Author

Paulb23 commented May 5, 2020

@pycbouh Unfortunately it's unlikely to be included in a 3.x release in part due to the compatibility breakage.

@Zylann Yeah I'm not the biggest fan of it either. The nested dictionary is Dictionary<String, Variant> as you say is to allow adding more data, currently I have in mind foreground / background colour, font weight and italics. I can't see a way to avoid the Dictionary allocation other then replacing it with a class. To avoid the string key there is the potential to use StringName instead.

@@ -355,6 +355,38 @@ ScriptCodeCompletionCache::ScriptCodeCompletionCache() {
singleton = this;
}

void ScriptLanguage::get_core_type_words(List<String> *p_core_type_words) const {
Copy link
Member

Choose a reason for hiding this comment

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

This could likely be used to remove the hardcoded core type names from modules/gdscript/gdscript_editor.cpp.

That's likely work for another PR though. There might be more synergy possible between the syntax highlighter code and the auto-completion code, at least as far as identifying core types, classes and keywords goes. (Or maybe not, it has to be assessed whether it makes sense/would be convenient or would just be too cumbersome.)

p_core_type_words->push_back("StringName");
p_core_type_words->push_back("NodePath");
p_core_type_words->push_back("RID");
p_core_type_words->push_back("Callable");
Copy link
Member

Choose a reason for hiding this comment

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

From variant.h you missed "Signal" after "Callable".

There's also Object, null, bool, int, float, but I guess those are left out on purpose.

@akien-mga akien-mga requested a review from neikeq May 6, 2020 07:09
@akien-mga
Copy link
Member

CC @neikeq as this might be relevant if we want to add C# syntax highlighting.

@Paulb23 Paulb23 force-pushed the syntax_highlighter_refactor branch from e930f04 to f910327 Compare May 7, 2020 19:52
@Paulb23
Copy link
Member Author

Paulb23 commented May 7, 2020

Added 'Signal' into the core type list.

@akien-mga
Copy link
Member

Needs a rebase after my recent style and init changes, otherwise should be good to merge.

@Paulb23 Paulb23 force-pushed the syntax_highlighter_refactor branch 2 times, most recently from 5649a0d to 45c5a02 Compare May 17, 2020 16:56
@ThakeeNathees
Copy link
Contributor

syntax

does it fix syntax highlighting for methods with the same name of builtin functions?

@Paulb23
Copy link
Member Author

Paulb23 commented Jun 14, 2020

@ThakeeNathees No, the syntax highlighting part is unchanged. (#28866)

@Paulb23 Paulb23 force-pushed the syntax_highlighter_refactor branch from 45c5a02 to d92d4b6 Compare July 10, 2020 12:20
@Paulb23 Paulb23 force-pushed the syntax_highlighter_refactor branch from d92d4b6 to 5fcafc3 Compare July 11, 2020 15:05
…hter

- Extacted all syntax highlighting code from text edit
- Removed enable syntax highlighting from text edit
- Added line_edited_from signal to text_edit
- Renamed get/set_syntax_highlighting to get/set_syntax_highlighter
- Added EditorSyntaxHighligher
@Paulb23 Paulb23 force-pushed the syntax_highlighter_refactor branch from 5fcafc3 to bc4cee4 Compare July 11, 2020 16:23
@akien-mga akien-mga merged commit ca5958d into godotengine:master Jul 14, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

@Paulb23 Raises those errors in a simple test project:

ERROR: color region with start key '/*' already exists.
   at: add_color_region (scene/resources/syntax_highlighter.cpp:484)
ERROR: color region with start key '//' already exists.
   at: add_color_region (scene/resources/syntax_highlighter.cpp:484)
ERROR: color region with start key '/*' already exists.
   at: add_color_region (scene/resources/syntax_highlighter.cpp:484)
ERROR: color region with start key '//' already exists.
   at: add_color_region (scene/resources/syntax_highlighter.cpp:484)

@Stuffe
Copy link

Stuffe commented Jan 15, 2022

Will this allow complete control over text coloring? For my game it would be extremely useful to be able to color by character index range manually.

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.

Allow all engine syntax highlighters in TextEdit
6 participants