Inline colors in TextEdit #14731

poke1024 · 2017-12-16T11:51:05Z

Prologue:

The current master has a cool function to open a color picker in the code editor using the context menu and Pick Color, but it's well hidden and I wasn't aware of it until I found it in the code:

This PR removes this great but hidden context menu Pick Color in favor of inline colors, i.e. colored circles in front of Color (you can click on them to open the color picker):

Full demo: https://www.dropbox.com/s/fdzqwnt6sm77w3u/demo-inline-colors.mp4?dl=0

I'm currently using unicode 0x25CF to draw the color circle, I'm not sure if that's safe on all versions of Freetype/Linux (plus sides: it adapts to all font sizes and is much faster than draw_circle).

27thLiz · 2017-12-16T13:47:13Z

Nice feature :)
However, I don't think it's wise to put this code in TextEdit.

groud · 2017-12-16T13:55:48Z

Nice! Great idea.

volzhs · 2017-12-16T13:58:04Z

I agree with @Hinsbart

brainsick · 2017-12-16T19:02:25Z

I'm not qualified to review the code, but a couple questions come to mind. Does this have any implication for code that cares about a line's length? I'm thinking of the recent patches for code indentation behavior that impacted the selection area.

This updates ClassDB entries, specifically moves _color_changed from ScriptTextEditor to TextEdit. Is this then considered part of the public API? Should that be documented? Not familiar with community guidelines on things that start with _; _color_changed doesn't appear to be documented now, so it's at least status quo. :)

poke1024 · 2017-12-16T19:16:48Z

It's indeed an API change so it's Godot 3.1.

The reason for the signal move is that TextEdit draws and handles the color circle (and knows when it's clicked). We could still move the color picker handling to ScriptTextEditor (probably a good idea?), but TextEdit would be the one to send the signal "show it now".

There should be no impact on selection code or the like as TextEdit collates the C character of Color with the color circle as if both were one glyph at rendering level. Any code above does not see the circle in positions or selection areas or the like - it's happening only inside the rendering and does not take up a position.

That's also the reason I believe this thing belongs in TextEdit and not somewhere else; that's the safest way to assure no other code is impacted. If we had it somewhere else, TextEdit would probably need to take non-pure-text input (i.e. streams of text blocks plus other stuff, much like RichTextLabel) and be able to report back events on non-text-input chunks; that would be a very big change as far as I see. Or we'd need to insert the circle as char, and then you have all the position problems coming.

blurymind · 2017-12-16T19:46:41Z

This is fantastic!! Really hope to see it in godot master 👍

brainsick · 2017-12-16T21:38:31Z

@poke1024 I definitely didn't mean to challenge this against the freeze. I don't see why this wouldn't be viable for 3.0 yet. Not that I have any say. Thanks! :)

poke1024 · 2017-12-17T11:13:05Z

@brainsick No problem, I'm actually not that familiar with Godot API change rules, but AFAIK you could grab that signal from gdscript so it's probably part of the API.

akien-mga · 2017-12-17T19:55:23Z

scene/gui/text_edit.cpp

@@ -30,6 +30,7 @@
 #include "text_edit.h"

 #include "message_queue.h"
+#include "modules/gdscript/gdscript_tokenizer.h"


GDScript is an optional module, we can't have a dependency on it in a core Control.

So this code should be moved somehow to the ScriptEditor, and guarded by #ifdef GDSCRIPT_ENABLED.

poke1024 · 2017-12-18T17:55:48Z

@akien-mga All gdscript parsing and the color picker are now moved (back) to the ScriptEditor, I did a number of bug fixes, cleanups, and generalizations along the way.

This should make it relatively easy to embed other inline content in the future if needed.

As a simple test, I've added a triangular inline error indicator that shows the column when an error comes up. Just an experiment along the way.

Paulb23 · 2017-12-28T15:33:51Z

Whats the general consensus with this kind of inheritance / OOP should it use a new node type / signals instead?

poke1024 · 2017-12-28T16:13:46Z

Currently InlineProcessor feels a bit like a hack. Note though that:

InlineProcessor::_parse_inline gets called for every line while drawing
the editor plugin system (e.g. CollisionPolygonEditorPlugin) use inheritance
probably should not/need not be accessible from outside the tools

akien-mga · 2018-01-04T14:02:59Z

Thanks a lot for your contribution!
We are now entering a strict release freeze for Godot 3.0 and will only consider major bug fixes. We won't merge new features and enhancements anymore until 3.0 is released.

Moving this PR to the 3.1 milestone, to be reviewed once the release freeze is lifted. It could eventually be cherry-picked for a future 3.0.1 maintenance release if it doesn't change the user-facing APIs and doesn't compromise the engine's stability.

vnen · 2018-02-15T00:26:07Z

This would close #4882.

akien-mga · 2018-07-25T10:05:36Z

This needs a rebase after the various TextEdit/CodeTextEditor abstractions done by @Paulb23 (#17923, #19225 and some others) which should also allow a cleaner implementation of this feature.

Maybe @Paulb23 could help review it?

poke1024 · 2018-08-04T16:32:28Z

Rebased (which I don't see myself doing again) and cleaned up.

The implementation is basically the same as before, the new abstractions for the syntax coloring are too high level to make use fo them for rendering the additional inline elements.

There are still problems when wrapping is enabled (basically not yet implemented as this was not there before).

Paulb23 · 2018-08-07T18:51:54Z

Tried this out locally, seems good for the most part.

It failed when the Colour is defined incorrectly:

var c = Color()

or when its defined over multiples lines e.g.

var c = Color(
    0,
    255
    0
)

In the first case, as its not valid code, will be erroring out at the tokenizer.
As for the second one, its probably the same reason, due it only inspecting one line a time, so the code won't be valid (most likely the same for wrapping).

It might be worth hiding the inline in these cases, rather then bothering the user with something they cannot interact with / displayed incorrectly.

As far as code goes, so long as the multi-inheritance is okay, I would move the tokenizer part further down something like:
script->get_language()->parse_inlines()

As a side thought, it might be worth looking at caching these in a similar way to ColorRegions.

vnen · 2018-08-07T19:56:43Z

It failed when the Colour is defined incorrectly:
var c = Color()

This is valid code, I don't know why this would give any error.

Paulb23 · 2018-08-08T00:08:54Z

This is valid code, I don't know why this would give any error.

My bad, it doesn't error. As there are no arguments, it can't find the colour values so it doesn't display correctly / set it as editable.

reduz · 2018-08-23T18:07:51Z

This branch has conflicts and some concerns of places where syntax wont work. Any progress on this so far?

poke1024 · 2018-08-24T17:41:29Z

I'll take a look at this this weekend.

- Renamed "inline" to "annotation" - Don't display partially parsable Colors - Fix annotations with line wrapping - Moved tokenizing to GDScriptLanguage - Caching annotation vectors in TextEdit

poke1024 · 2018-08-26T18:19:44Z

Changes:

Don't display partially parsable Colors
Moved all tokenizing to GDScriptLanguage
Annotation vectors are now cached in TextEdit

Also:

Renamed "inline" to "annotation
Fix annotations with line wrapping

Apart from the color pick annotation, this currently also includes the red triangle error marker for the current error position; I personally find this useful, but it's not strictly part of this PR.

Multi-line color-definitions are ignored.

I didn't squash the commits yet, to make the changes more trackable.

Paulb23

Its Looking good, There's a issue with the error marker sometimes not appearing, or appearing in the incorrect place:

But apart from that, only very minor code issues.

Paulb23 · 2018-09-10T19:17:50Z

editor/plugins/script_text_editor.cpp

@@ -35,6 +35,18 @@
 #include "editor/script_editor_debugger.h"
 #include "os/keyboard.h"

+#ifdef GDSCRIPT_ENABLED
+#include "modules/gdscript/gdscript_tokenizer.h"


This import is no longer needed.

Paulb23 · 2018-09-10T19:18:43Z

modules/gdscript/gdscript_editor.cpp

+ code_editor->get_text_edit()->set_line(c.line, line.substr(0, c.begin) + new_args + line.substr(c.end + 1, line.length()));
+ code_editor->get_text_edit()->update();
+ edited_inline_color.end = c.begin + new_args.length();
+ }*/


No need to keep this.

Paulb23 · 2018-09-10T19:22:51Z

scene/gui/text_edit.h

@@ -382,20 +387,19 @@ class TextEdit : public Control {
 void _scroll_lines_up();
 void _scroll_lines_down();

+ void _color_changed(const Color &p_color);


This is not implemented anywhere?

Paulb23 · 2018-09-10T19:23:52Z

scene/gui/text_edit.h

@@ -31,6 +31,7 @@
 #ifndef TEXT_EDIT_H
 #define TEXT_EDIT_H

+#include "scene/gui/color_picker.h"


This should be imported in ScriptTextEditor

akien-mga · 2018-12-09T22:37:05Z

Moving to 3.2 milestone as we're reaching beta stage for 3.1, and there are still changes needed in this PR.

Anutrix · 2019-07-11T13:23:13Z

@poke1024 Rebase?

akien-mga · 2019-08-28T09:05:34Z

This needs to be rebased indeed to be mergeable, but we haven't reached a clear consensus about the implementation.

The feature looks awesome, but I have some concerns over adding annotations to ScriptLanguage for this, and the extensive changes to TextEdit for a feature which is mostly relevant for editor tools.

I mentioned in other issues and on IRC that I find TextEdit to grow too big and beyond the scope it should have as a generic text input field for games.
I think we should aim to refactor this for 4.0 and have TextEdit be a lightweight text input control, and add a CodeEdit or similar that extends TextEdit and implements all code/tool-specific features like inline colors, but also syntax highlighting, indentation, breakpoint gutter, etc.

If we decide to do this, I would then hold off on this PR until such refactoring has been done, and then the PR could be redone to implement the feature via CodeEdit (or whatever its name would be). I'll open an issue about it (Edit: it's #31739).

akien-mga · 2020-02-07T13:46:22Z

As mentioned in #14731 (comment), a correct implementation for this feature should likely wait for #31739 to be implemented. I will close this for now as it's not going to be merged as is.

This should likely be rediscussed in a proposal in the https://github.com/godotengine/godot-proposals/ repository, to see if/how we want to support this feature for the script editor (and shader editor?) while keeping the underlying controls manageable.

akien-mga added enhancement topic:editor usability labels Dec 16, 2017

akien-mga added this to the 3.0 milestone Dec 16, 2017

akien-mga reviewed Dec 17, 2017

View reviewed changes

poke1024 force-pushed the inline_colors branch 4 times, most recently from a2b8357 to 9655abc Compare December 18, 2017 16:28

poke1024 force-pushed the inline_colors branch from 9655abc to ee10412 Compare December 28, 2017 16:30

akien-mga modified the milestones: 3.0, 3.1 Jan 4, 2018

ghost mentioned this pull request Feb 6, 2018

GDNative Requests / push motivation towards it? #16457

Closed

poke1024 force-pushed the inline_colors branch 2 times, most recently from 84d49b9 to 49ead84 Compare August 4, 2018 16:30

poke1024 force-pushed the inline_colors branch from 49ead84 to 8e32016 Compare August 4, 2018 18:38

Inline colors in TextEdit

5e449d3

poke1024 force-pushed the inline_colors branch from 8e32016 to c3ce0c0 Compare August 26, 2018 15:54

poke1024 requested review from bojidar-bg, reduz and vnen as code owners August 26, 2018 15:54

TextEdit annotations: redesign

d41fe42

- Renamed "inline" to "annotation" - Don't display partially parsable Colors - Fix annotations with line wrapping - Moved tokenizing to GDScriptLanguage - Caching annotation vectors in TextEdit

poke1024 force-pushed the inline_colors branch from c3ce0c0 to d41fe42 Compare August 26, 2018 18:03

akien-mga mentioned this pull request Sep 10, 2018

Add color highlighting in Script editor #21926

Closed

Paulb23 reviewed Sep 10, 2018

View reviewed changes

akien-mga added the salvageable label Dec 9, 2018

akien-mga modified the milestones: 3.1, 3.2 Dec 9, 2018

akien-mga added topic:core needs work labels Aug 28, 2019

akien-mga modified the milestones: 3.2, 4.0 Aug 28, 2019

akien-mga closed this Feb 7, 2020

akien-mga added the archived label Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline colors in TextEdit #14731

Inline colors in TextEdit #14731

poke1024 commented Dec 16, 2017

27thLiz commented Dec 16, 2017

groud commented Dec 16, 2017

volzhs commented Dec 16, 2017

brainsick commented Dec 16, 2017

poke1024 commented Dec 16, 2017

blurymind commented Dec 16, 2017

brainsick commented Dec 16, 2017

poke1024 commented Dec 17, 2017

akien-mga Dec 17, 2017

poke1024 commented Dec 18, 2017

Paulb23 commented Dec 28, 2017

poke1024 commented Dec 28, 2017

akien-mga commented Jan 4, 2018

vnen commented Feb 15, 2018

akien-mga commented Jul 25, 2018

poke1024 commented Aug 4, 2018

Paulb23 commented Aug 7, 2018

vnen commented Aug 7, 2018

Paulb23 commented Aug 8, 2018

reduz commented Aug 23, 2018

poke1024 commented Aug 24, 2018

poke1024 commented Aug 26, 2018

Paulb23 left a comment

Paulb23 Sep 10, 2018

Paulb23 Sep 10, 2018

Paulb23 Sep 10, 2018

Paulb23 Sep 10, 2018

akien-mga commented Dec 9, 2018

Anutrix commented Jul 11, 2019

akien-mga commented Aug 28, 2019 •

edited

Loading

akien-mga commented Feb 7, 2020

Inline colors in TextEdit #14731

Inline colors in TextEdit #14731

Conversation

poke1024 commented Dec 16, 2017

27thLiz commented Dec 16, 2017

groud commented Dec 16, 2017

volzhs commented Dec 16, 2017

brainsick commented Dec 16, 2017

poke1024 commented Dec 16, 2017

blurymind commented Dec 16, 2017

brainsick commented Dec 16, 2017

poke1024 commented Dec 17, 2017

akien-mga Dec 17, 2017

Choose a reason for hiding this comment

poke1024 commented Dec 18, 2017

Paulb23 commented Dec 28, 2017

poke1024 commented Dec 28, 2017

akien-mga commented Jan 4, 2018

vnen commented Feb 15, 2018

akien-mga commented Jul 25, 2018

poke1024 commented Aug 4, 2018

Paulb23 commented Aug 7, 2018

vnen commented Aug 7, 2018

Paulb23 commented Aug 8, 2018

reduz commented Aug 23, 2018

poke1024 commented Aug 24, 2018

poke1024 commented Aug 26, 2018

Paulb23 left a comment

Choose a reason for hiding this comment

Paulb23 Sep 10, 2018

Choose a reason for hiding this comment

Paulb23 Sep 10, 2018

Choose a reason for hiding this comment

Paulb23 Sep 10, 2018

Choose a reason for hiding this comment

Paulb23 Sep 10, 2018

Choose a reason for hiding this comment

akien-mga commented Dec 9, 2018

Anutrix commented Jul 11, 2019

akien-mga commented Aug 28, 2019 • edited Loading

akien-mga commented Feb 7, 2020

akien-mga commented Aug 28, 2019 •

edited

Loading