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

Add Code Block Guidelines to Script Editor & CodeEdit #65757

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 13, 2022

Resurrects #20725, and #38833 somewhat.

In this PR, code_block_guidelines_style is added in CodeEdit. When set, lines appear between the start and end of a foldable code block. The line is appropriately indented.

image

Multiples guidelines may stack on top of each other, within reason, and prefer to connect to a bottom parenthesis.
The colour of the guideline becomes slightly more noticeable when the caret is inside the code block:

image

It can be switched between "None", "Line" and "Line (Close)". This feature is set to "None" by default for the Editor. Its colors can be customised in the Editor Settings. (good luck doing that without making master crash).

Draw Tabs Codeblock Guidelines
image image

Feedback is absolutely welcome.

Optional Todos(?)/Notes:

  • Improve the way the caret is detected inside a code block. May require fetching all code blocks first, then drawing all line guides later. Currently, it is way too flashy, at least to my tastes. It may be worth removing this part outright.
  • There's a desynchronisation issue when adding or removing lines at the end of the Script. This may likely not be addressed here. Rather, it seems like the v_scroll offset value is not updated properly.
  • When this setting is enabled, possibly have "stray" leading tabs and spaces drawn regardless of their settings (uneven or not belonging in any code block?).
  • Any issue with the folding detection being somewhat weird is not my doing, it's just the same function as it was before this PR. It can be improved upon another time.

@Mickeon Mickeon force-pushed the salvage-identation-guides branch 3 times, most recently from 0a96b46 to c52be7d Compare September 13, 2022 19:16
@Mickeon Mickeon marked this pull request as ready for review September 13, 2022 19:16
@Mickeon Mickeon requested review from a team as code owners September 13, 2022 19:16
@Calinou
Copy link
Member

Calinou commented Sep 13, 2022

This is turned off by default. Its colors can be customised in the Editor Settings (good luck doing that without making master crash).

I'd prefer this to be enabled by default, as it matches how other text editors look by default more closely.

@Calinou Calinou added this to the 4.0 milestone Sep 13, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 13, 2022

I'd prefer this to be enabled by default, as it matches how other text editors look by default more closely.

I wanted to avoid alienating existing Godot users but if people love this a lot I have no problem turning this on by default!

@Mickeon Mickeon force-pushed the salvage-identation-guides branch 4 times, most recently from fd032b6 to 564bea3 Compare September 13, 2022 22:50
@MewPurPur
Copy link
Contributor

How do the branches in the tree-like structure look when there's lots of nesting or when the editor is zoomed out?

@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 14, 2022

Anyway, we have an editor setting to display spaces as little points, off by default. A setting to display tabs, right next to that setting, would be super fitting. I don't think it should be a toggle between displaying tabs or displaying this tree-like thingy, some might want both, after all tabs are valid whitespaces too.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 14, 2022

How do the branches in the tree-like structure look when there's lots of nesting or when the editor is zoomed out?

image

Anyway, we have an editor setting to display spaces as little points, off by default. A setting to display tabs, right next to that setting, would be super fitting. I don't think it should be a toggle between displaying tabs or displaying this tree-like thingy, some might want both, after all tabs are valid whitespaces too.

The setting is available right here the Editor Settings, independent from tabs and spaces.
image
Both can be enabled just f- Oh my God, the tab icon doesn't scale.
image

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 14, 2022

Major sidenote: While this is heavily inspired by #20725 and #38833, it's not the same, although in practice it accomplishes the same thing, just with more "finesse".

Those PRs were aiming to do what Visual Studio Code does, for example. Those implementations suit more when "open and close" brackets are involved, because the indent line would directly connect between the top and bottom brackets.
9a91b51abbf37659fed569245b9863bd

@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 14, 2022

I mean like the uneven dictionary example, if there are 6+ of these little branches and the text is zoomed out, wouldn't they slip out on top?

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 14, 2022

There's an hard cap, at which point they just start stacking on top of one another (this should be proportional to the font size but it currently isn't yet, so technically yes, they do slip up right now at a small size)

@EricEzaM
Copy link
Contributor

EricEzaM commented Sep 14, 2022

Thanks for picking this feature again, it's a great one to have.
Personal preference notes:

  • I don't like how the vertical line starts in the middle of the first character. It should align to the start/left of the first character as that is where the block starts, not in the middle of it.
    • this also causes the line to overlap the tab/space character when the visibility of them is switched on. If they were aligned to the left this would not happen (and would imo look cleaner)
  • I personally am not a fan of the horizontal lines that indicate the closing of a block (looks like that's what they do?). Anyway, to me it clutters the visuals and is not necessary.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 14, 2022

With these preferences, it sounds like you'd much love if that is how indents were symbolised (like #65757 (comment)), instead. I like the way the PR currently looks, minus minor tweaks, and one could argue it's a matter of getting used to, but I will keep this in mind. Perhaps theme overrides could be used for the minor adjustments.

@Zireael07
Copy link
Contributor

@Mickeon symbolized indents could be a good idea BUT for a follow-up PR

@Mickeon Mickeon force-pushed the salvage-identation-guides branch from 564bea3 to 33b1ef2 Compare September 15, 2022 13:58
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 15, 2022

@MewPurPur

I mean like the uneven dictionary example, if there are 6+ of these little branches and the text is zoomed out, wouldn't they slip out on top?

This is the worst case scenario now:
image

Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Not a fan of the horizontal lines. Could turn it into an enum if there is a want for both styles.

This will not work if folding is disabled, whereas it should be independent of that setting, we can pull out the code into private methods if needs be.

editor/editor_settings.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.h Outdated Show resolved Hide resolved
scene/gui/code_edit.h Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
const double v_scroll = get_v_scroll();
const double h_scroll = get_h_scroll();
Vector<int> block_ends;
for (int i = 0; i < get_line_count(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

We should only draw what is on screen, rather then the entire text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this would require caching block start and endings whenever the text is changed and starting from there. The change may take a while to implement, but it has potential to make the additional "extra highlight" of the code block the caret is in more reliable.

Copy link
Member

Choose a reason for hiding this comment

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

We can certainly calculate from line 0 to end of the visible viewport, so long as we don't make any unnecessary calls to the RenderingServer. TextEdit already does quite a lot around this. Will probably require some form of benchmarking to see if it's worth caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what you're suggesting for now is to at least make sure that the RenderingServer doesn't draw guidelines that cannot be seen?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah whilst stopping calculation at the end of the visible viewport rather then all the way to the end of the script. I think caching will be faster, but better to benchmark first to make sure the extra complexity is worth it.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 19, 2022

Not a fan of the horizontal lines. Could turn it into an enum if there is a want for both styles.

I find it pretty nice. Godot doesn't have your classic closing brackets at the end of a block, which makes it a bit difficult to discern, were it not for the change of indentation. It could also be a matter of getting used to, but unfortunately you're not the only one expressing this opinion. A shame. Without it, and all of the detection required to figure out a code block, this PR would be little to no different than making all indents look like semi-transparent columns (like the "ancestor PRs", and Visual Studio Code do).

I could make it an enum. Perhaps, in the future, more styles could be added if requested (e.g. Visual Studio uses dashed lines).

This will not work if folding is disabled, whereas it should be independent of that setting, we can pull out the code into private methods if needs be.

This is true. This would require having CodeEdit detect code blocks beforehand and caching them, which could go hand-to-hand with the change required here #65757 (comment).

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 20, 2022

Done the drawing optimisation. All that is missing is #65757 (review) which, for the gutters thing, may possibly prove tricky and admittedly janky.

EDIT: Requesting review for the method splitting, while working on an enum.

@Mickeon Mickeon force-pushed the salvage-identation-guides branch from d38de7b to 34ec895 Compare September 20, 2022 18:27
@Mickeon Mickeon requested a review from Paulb23 September 20, 2022 18:27
@Mickeon Mickeon force-pushed the salvage-identation-guides branch 4 times, most recently from f2ddcb5 to 93cf9af Compare September 20, 2022 22:59
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 20, 2022

The property is now an enum, and it has a new style that removes the closing horizontal line as @Paulb23 and @EricEzaM would prefer.
image

@Mickeon Mickeon force-pushed the salvage-identation-guides branch 3 times, most recently from 47831b0 to 759f8ea Compare September 22, 2022 19:59
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Tested out the draw performance in CodeEdit alone, currently way to slow. 300ms slower on a ~5.5k line script. Even at 2k lines it takes 50ms.

Might be worth seeing if there are other places to optimise, otherwise looks like it will require some form of caching.

scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.h Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the salvage-identation-guides branch from 759f8ea to dcf10ee Compare September 27, 2022 09:28
Add `code_block_guidelines_style` in CodeEdit. When set, lines appear between the start and end of a foldable code block. This line is appropriately indented.

Multiples guides may stack on top of each other, within reason, and prefer to connect to a bottom parenthesis.

The colour of the guide line becomes _slightly_ more noticeable when the caret is inside the code block:

It can be switched between "None", "Line" and "Line (Close)". This feature is set to "None" by default for the Editor. Its colors can be customised in the Editor Settings.
@Mickeon Mickeon force-pushed the salvage-identation-guides branch from dcf10ee to 2fa1b34 Compare September 27, 2022 09:34
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 27, 2022

Might be worth seeing if there are other places to optimise, otherwise looks like it will require some form of caching.

I don't think there is much else to optimise. The issue right now is that the code blocks themselves are re-calculated every draw call. So maybe this feature may have to wait until CodeEdit can actually detect and store code blocks on its own (and not just code folding)?

@Calinou
Copy link
Member

Calinou commented Sep 27, 2022

Can we just modify the indent guidelines to use vertical lines instead of chevrons and not have the horizontal lines drawn? This should be much faster while suiting most use cases.

The horizontal lines can be left for a future PR as they're not essential.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 27, 2022

@Calinou If I were to implement that in another PR, for the time being, should it be based on the indent amount (so that both spaces and tabulations will display them) or should it be outright replacing the tabulation symbol?

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 5, 2023
@Calinou
Copy link
Member

Calinou commented Jun 2, 2023

@Calinou If I were to implement that in another PR, for the time being, should it be based on the indent amount (so that both spaces and tabulations will display them) or should it be outright replacing the tabulation symbol?

I think it should be based on the indent amount, so that trailing whitespace does not show indent guides. This will also make indentation look the same when using spaces for indentation, which matches the behavior in most other editors. (We could display invisible characters when selected to make troubleshooting easier, like in VS Code.)

Point2 point_start = Point2(indent_guide_x, row_height * visible_indent_start);
Point2 point_end = Point2(indent_guide_x, row_height * visible_indent_end - offset_y);

RenderingServer::get_singleton()->canvas_item_add_line(ci, point_start, point_end, color, 1.0);
Copy link
Contributor

@MewPurPur MewPurPur Oct 3, 2023

Choose a reason for hiding this comment

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

I might be a bit naive, but won't we get a performance boost by doing this at once with canvas_item_add_multiline()?

@ad-si
Copy link

ad-si commented Jun 4, 2024

Why was this never merged?

@AThousandShips
Copy link
Member

It hasn't been approved and still has feedback to address

@Mickeon
Copy link
Contributor Author

Mickeon commented Jun 6, 2024

The PR is pretty old so it would be better be done from scratch. It's also not really performant. These are both because there was no existing methods to accomplish this PR, and little to no caching in between. Both of these may no longer be a problem now (?)

@ndbn
Copy link

ndbn commented Sep 9, 2024

The PR is pretty old so it would be better be done from scratch. It's also not really performant. These are both because there was no existing methods to accomplish this PR, and little to no caching in between. Both of these may no longer be a problem now (?)

I made EditorPlugin (godot 4.3) very based on your PR and it (your code) works very well (as well as with canvas_item_add_multiline).
May be this can helps this PR come to life
image

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 10, 2024

It would be lovely if you shared your EditorPlugin as a proof-of-concept, perhaps in the Asset Library as well.

@ndbn
Copy link

ndbn commented Sep 10, 2024

It would be lovely if you shared your EditorPlugin as a proof-of-concept, perhaps in the Asset Library as well.

Still on pending at Asset Library, but repo is here
edit: accepted

It's a bit buggy, in GDScript not available CodeEdit::_is_line_hidden, for example.
And I'm not sure about how I get analog of theme cache variables, but for general purpose it works

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.

Add indent guides as an alternative to visible tab characters to the script editor
10 participants