-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Editor Help: Add syntax highlighting for code blocks #89263
Editor Help: Add syntax highlighting for code blocks #89263
Conversation
dalexeev
commented
Mar 7, 2024
•
edited
Loading
edited
- Closes Add highlighting to code blocks in the built-in documentation (and more?) godot-proposals#7617.
f77dc81
to
371617a
Compare
return singleton; | ||
} | ||
|
||
EditorHelpHighlighter::HighlightData EditorHelpHighlighter::_get_highlight_data(Language p_language, const String &p_source, bool p_use_cache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method probably needs a mutex, since a single Script
+ TextEdit
+ EditorSyntaxHighlighter
are used (within one language). And probably different EditorHelp
s work in different threads? But I didn't notice any problems during testing however.
Please test with C# build, I didn't do that. By the way, since Example (set |
CC @godotengine/dotnet I don't think we actually have a C# syntax highlighter currently, so the addition here is theoretical. But it might be a good incentive to add at least a minimal highlighter for docs' sake, or the handful of users who for some reason might be opening C# scripts in the builtin editor instead of a C# IDE :) |
BTW I forgot we even had this feature, and I assume most users don't know it. We should probably expose this as a button in the EditorHelp that lets you enable GDScript and/or C# snippets (the latter being disabled on non-mono builds). Like it could be one button per language just reusing the language's script icon. |
@@ -95,7 +95,7 @@ | |||
print(get_stack()) | |||
[/codeblock] | |||
Starting from [code]_ready()[/code], [code]bar()[/code] would print: | |||
[codeblock] | |||
[codeblock lang=text] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless it's derived from somewhere where it's standard, I'd rather be explicit and write the whole parameter name (languange=text
) instead.
Also there's other places where this would be needed but for this PR it's completely fine to only use it here as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless it's derived from somewhere where it's standard, I'd rather be explicit and write the whole parameter name (
languange=text
) instead.
Your typo makes a strong case against this suggestion ;)
lang
is clear enough and less prone to typos IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a follow-up task for documentation team if this PR gets merged. I corrected only one file to test the RST generator and for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW an alternative would be to always use [codeblocks][gdscript]...
for actual code examples, and thus also make sure we provide C# examples where relevant. And then default to assuming plain text for [codeblock]
.
Maybe with an optional [codeblock lang=gdscript]
/[codeblock lang=csharp]
for whenever it really doesn't make sense to provide both languages (e.g. in @GDScript
docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up the air now, but this addition has me question if the [codeblocks]
+ [gdscript]
/[csharp]
syntax should be replaced by [codeblocks]
+ [codeblock lang=gdscript/csharp]
. Not to break what isn't broke, but just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We literally wrote the same thing within the same minute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[codeblocks]
, [gdscript]
and [csharp]
are more like preprocessor directives that depend on the editor setting. While [codeblock lang=code]
does not depend on the setting. Also hypothetically you could use (this already works):
Prints:
[codeblocks]
[gdscript lang=text]
GDScript output.
[/gdscript]
[csharp lang=text]
C# output.
[/csharp]
[/codeblocks]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up the air now, but this addition has me question if the
[codeblocks]
+[gdscript]
/[csharp]
syntax should be replaced by[codeblocks]
+[codeblock lang=gdscript/csharp]
. Not to break what isn't broke, but just a thought.
Well IMO that would be quite verbose:
[codeblocks]
[codeblock lang=gdscript]
pass
[/codeblock]
[/codeblocks]
and hard to read, where s
is the only difference between different tags.
It's a pre-existing issue with us having both [codeblock]
and [codeblocks]
, but both are top-level tags, while [gdscript]
/[csharp]
are nested tags that further specify [codeblocks]
.
My proposal is instead to support both this:
[codeblocks]
[gdscript]
pass
[/gdscript]
[csharp]
System.Collections.MadeUp.Pass<T>;
[/csharp]
[text]
some text
[/text]
[/codeblocks]
and this:
[codeblock lang=gdscript]
pass
[/codeblock]
[codeblock lang=csharp]
System.Collections.MadeUp.Pass<T>;
[/codeblock]
[codeblock]
some text
[/codeblock]
Note that the two aren't equivalent - the first is a single code block with 3 tabs, the second is three code blocks each with different highlighting (with lang=text
being inferred if not specified).
@@ -3504,21 +3505,23 @@ static String _process_doc_line(const String &p_line, const String &p_text, cons | |||
from = rb_pos + 1; | |||
|
|||
String tag = line.substr(lb_pos + 1, rb_pos - lb_pos - 1); | |||
if (tag == "code") { | |||
if (tag == "code" || tag.begins_with("code ")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better here to make a distinction between the initial "tag" and additional parameters, using something like String.split()
. But then there's result += "[" + tag + "]";
below so I am not sure.
I had in my mind to eventually expose all Editor Help Settings as easily accessible preferences on the top-right, or something. There needs to be toggles to see deprecated/experimental class reference soon. |
I think for C# we use basic highlighting ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR was a post on a community forum it would get 500 upvotes and not a single comment, let alone one against it.
371617a
to
b926209
Compare
We do, but it is very rough. For instance, I couldn't tell you why one of the following is highlighted half-decently, and the other isn't. |
b926209
to
6956013
Compare
6956013
to
225babc
Compare
225babc
to
87718d2
Compare
Thanks! |