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 String/Comment tracking, AutoComplete and hints to CodeEdit #45393

Merged
merged 4 commits into from
Jun 1, 2021

Conversation

Paulb23
Copy link
Member

@Paulb23 Paulb23 commented Jan 23, 2021

Continuation of #31739

Builds on top of #42775 and blocked by #43663.


The main aim of this PR is abstracting out the auto complete system into CodeEdit, in order to do so it needed a system to track Strings.

The implementation of String tracking will also track Comments. The system closely follows that of the syntax highlighter, using the idea of regions.

It exposes add_[string|comment]_delimiter methods to register the begin and end keys for each type. Once registered CodeEdit will keep track of the region. It's possible to check regions via is_in_[string|comment] methods, if no column is provided it will return true if the entire line is a String or Comment. It is also possible to get the start and end position of regions via get_delimiter_[start|end]_position.

Regions are updated on the lines_edited_from signal, I've tried to make this somewhat fast to hopefully have minimal impact on performance.

AutoComplete is now exposed to the GDScript, items are added via add_code_completion_option. Once all items are added a call to update_code_completion_options will update the list of possible completions in the drop down. In addition to this there are three overridable methods:

  • _request_code_completion -> Checks and emits request_code_completion signal.
  • _filter_code_completion_candidates -> Filters the list of options.
  • _confirm_code_completion -> Inserts the selected option

There are also various ways to query autocomplete getting the current selected, setting the selected item and getting the full list of options.

As part of the refactor I've attempted to remove the editor specific code and added a couple of new features. The first one allowing the user to force autocomplete. Secondly, adding a second shortcut to replace the existing text rather then inserting ontop. This is currently bound to Tab, the old behaviour is preserved by using Enter / KP Enter.

In addition to this the request_completion signal has been renamed to request_code_completion

To handle drawing, I've added two methods to TextEdit to get the caret draw position and to check if the caret is visible.

Finally CodeHint has also been moved and exposed, and callhint_offset has been removed.


TODO


closes #22196
closes #36833
closes #23756
closes #34168
closes #24077
closes #44000

Should solve some of #38847 and godotengine/godot-proposals/issues/1514

@KoBeWi
Copy link
Member

KoBeWi commented Feb 24, 2021

So I tested if the PR really solves the linked issues and
#23756 doesn't look fixed, unless I'm misunderstunding something
fc28QUeP1U
#24077 isn't fixed either. Or at least it's impossible to test, because the actions don't appear at all.
#42382 same as above. Can't test print("stuff", <autocomplete here>, because print( doesn't suggest anything.

The autocompletion seems broken on master for some things, which makes testing this difficult.

EDIT:
After switching back to clean build, turns out this PR breaks autocompletion for $ operator.

@Paulb23
Copy link
Member Author

Paulb23 commented Feb 25, 2021

@KoBeWi
Re, #23756: There are now two shortcuts for completing. Enter / Kp Enter which keeps the old behaviour and inserts it inline. Tab is the new behaviour which will replace / merge the text.

Re, #24077 #42382: Yeah not entirely sure how to test these fully on master at the moment. Though the string tracking "should" fix them.

Re, '$': '$' seems to be the same as master for me? Do you have some steps to reproduce your issue?

@KoBeWi
Copy link
Member

KoBeWi commented Feb 25, 2021

There are now two shortcuts for completing. Enter / Kp Enter which keeps the old behaviour and inserts it inline. Tab is the new behaviour which will replace / merge the text.

Neat! You should probably mention it in the OP.

'$' seems to be the same as master for me? Do you have some steps to reproduce your issue?

Master:
image
This PR:
image

EDIT:
Seems like pressing Ctrl + Space fixes this. Only initial popup has wrong suggestions.

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from 74632cd to 36bb651 Compare March 7, 2021 12:57
@Paulb23 Paulb23 marked this pull request as ready for review March 7, 2021 13:41
@Paulb23 Paulb23 requested review from a team as code owners March 7, 2021 13:41
is_cursor_line_visible = true;
cursor_pos.y = line_top_offset_y;

if (!clipped && cursor.line == line && line_wrap_index == caret_wrap_index) {
Copy link
Member Author

Choose a reason for hiding this comment

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

CC @bruvzg, I'm not entirely sure about this change. Was there any reason this was ran on all wrapped lines rather then where the caret is?

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from 36bb651 to 8fab005 Compare April 25, 2021 14:40
@qarmin
Copy link
Contributor

qarmin commented Apr 26, 2021

Ci crash

CodeEdit.confirm_code_completion
Parameters [False]
ERROR: FATAL: Index p_index = 0 is out of bounds (size() = 0).
   at: get (./core/templates/cowdata.h:157)
handle_crash: Program crashed with signal 4
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] bin/godot.linuxbsd.tools.64s() [0x1e5e7d8] (/home/runner/work/godot/godot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7f712225a210] (??:0)
[3] CowData<ScriptCodeCompletionOption>::get(int) const (/home/runner/work/godot/godot/./core/templates/cowdata.h:157 (discriminator 7))
[4] Vector<ScriptCodeCompletionOption>::operator[](int) const (/home/runner/work/godot/godot/./core/templates/vector.h:89)
[5] CodeEdit::confirm_code_completion(bool) (/home/runner/work/godot/godot/scene/gui/code_edit.cpp:954 (discriminator 1))

This code should crash with same as above message

CodeEdit.new().confirm_code_completion(false)
ClassDB.instance("CodeEdit").confirm_code_completion(false)

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from 8fab005 to a6b13f7 Compare April 26, 2021 18:56
@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from a6b13f7 to e980f4c Compare May 9, 2021 14:10
@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from e980f4c to 874f3c8 Compare May 22, 2021 14:12
@Paulb23 Paulb23 requested a review from a team as a code owner May 22, 2021 14:12
Comment on lines +40 to +41
/* Keep enum in sync with: */
/* /core/object/script_language.h - ScriptCodeCompletionOption::Kind */
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just use the other enum, instead of introducing another one that needs to be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's possible, though it felt incorrect to bind a enum from /core to CodeEdit, not sure of any cases where enums aren't local to the class. Also can't bind it to ScriptCodeCompletionOption as it's a struct, because of that, if we did cross reference, the docs get confused tying to look for ScriptCodeCompletionOption.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's required for binding. Not sure then, either we move it to CodeEdit or maybe it's fine to have duplicates if they are commented properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that crossed my mind too, but as it's used in script_language, moving it would make /core dependent on /scene, which is a no go.

Not sure there is a clean way out of it as is, without duplicating. The alternative is to not use enums, or we rewrite ScriptCodeCompletionOption into a bindable class, which is a lot easier said then done.

@KoBeWi
Copy link
Member

KoBeWi commented May 30, 2021

I went over the code and, aside from that weird double enum I mentioned above, it looks mostly ok (although it's a lot of code and I didn't take a very detailed look). Would be nice to have this rebased, so it's possible to test.

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from 874f3c8 to 022f6e2 Compare May 31, 2021 13:16
@KoBeWi
Copy link
Member

KoBeWi commented May 31, 2021

I checked again all the linked issues and #42382 is not fixed.
opJA1ZJaEV
But IMO it's ok if you just remove it from the OP.

There are also many new methods that need to be documented. It would be best to have them filled in this PR, even if very briefly.

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from 022f6e2 to b05941c Compare June 1, 2021 14:38
@Paulb23
Copy link
Member Author

Paulb23 commented Jun 1, 2021

Updated docs and removed #42382 from the op.

@Paulb23 Paulb23 force-pushed the code_edit_autocomplete branch from b05941c to 1684276 Compare June 1, 2021 15:14
@akien-mga akien-mga merged commit c5f237e into godotengine:master Jun 1, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment