godot icon indicating copy to clipboard operation
godot copied to clipboard

Autofill comment when inserting a new line in documentation

Open Cronos87 opened this issue 1 year ago β€’ 39 comments

This PR implements the following proposal https://github.com/godotengine/godot-proposals/issues/8244.

Here it is in action:

https://github.com/user-attachments/assets/89cccc4b-482f-4f79-852f-c2857dfa088e

Please keep in mind that this is my first contribution and my first step into the C++ world, but I really want to contribute to Godot :)

Any feedback, help, or advice is very welcome!

  • Bugsquad edit, closes: https://github.com/godotengine/godot-proposals/issues/8244

Cronos87 avatar Aug 22 '24 16:08 Cronos87

CodeEdit should be language independent, so we can't use ## in it. The delimiter system is how we check for regular comments, so it looks like it needs to know about doc comments and the String to insert on a new line.

Then it can probably be set here:

https://github.com/godotengine/godot/blob/568589c9d8c763bfb3a4348174d53b42d7c59f21/editor/plugins/script_text_editor.cpp#L252-L262

kitbdev avatar Aug 22 '24 23:08 kitbdev

CodeEdit should be language independent, so we can't use ## in it. The delimiter system is how we check for regular comments, so it looks like it needs to know about doc comments and the String to insert on a new line.

It totally makes sense. I'll give it a try as soon as I can in the file you just pointed out.

Thank you for your feedback!

Edit: I took a look at the file you told me, @kitbdev. I can get the delimiters in the code_edit.cpp by doing get_comment_delimiters(). However, in my case in GDScript, the array will be ['##', '#']. It doesn't inform me which one is the doc comment, and I didn't find a way to identify it. Maybe you have a clue regarding this?

I continue to search for the answer in any case :)

Cronos87 avatar Aug 23 '24 06:08 Cronos87

Perhaps this makes sense not only for doc comments, but for regular ones too. Kate has interesting behavior:

  1. Enter copies the indentation of the previous line. If it ends with {, :, etc. (depending on the language), the indentation level is increased.
  2. Shift+Enter copies the indentation of the previous line with leading symbols/spaces up to the first letter/number.
  3. Ctrl+Enter inserts only a line break, like in simple text editors like Notepad.

Shift+Enter is also useful for Markdown and YAML lists.

dalexeev avatar Aug 23 '24 07:08 dalexeev

I can get the delimiter in the code_edit.cpp by doing get_comment_delimiters().

Why not use get_doc_comment_delimiters() which returns only ## for GDScript? That's also what's used in the code that kitbdev linked to.

RedMser avatar Aug 23 '24 19:08 RedMser

Why not use get_doc_comment_delimiters() which returns only ## for GDScript? That's also what's used in the code that kitbdev linked to.

I agree, but this method is only available in script_text_editor.cpp as far as I know. I suppose I can add a method to code_edit.cpp to retrieve the data, but I don't know if this is a good practice.

I'm digging into the codebase; I need time to get used to it, I suppose 😊

Cronos87 avatar Aug 23 '24 19:08 Cronos87

I did my best to enhance it and to be more flexible. Feedback is very welcome πŸ™

Cronos87 avatar Aug 24 '24 22:08 Cronos87

Perhaps this makes sense not only for doc comments, but for regular ones too. Kate has interesting behavior:

  1. Enter copies the indentation of the previous line. If it ends with {, :, etc. (depending on the language), the indentation level is increased.
  2. Shift+Enter copies the indentation of the previous line with leading symbols/spaces up to the first letter/number.
  3. Ctrl+Enter inserts only a line break, like in simple text editors like Notepad.

Shift+Enter is also useful for Markdown and YAML lists.

Sorry for the delayed answer, @dalexeev! Yes, indeed, that would be nice, but the proposal is limited to the documentation comment. I don't know what people think about it and if we should extend the feature to comments too.

I am open to discussion.

Cronos87 avatar Aug 27 '24 14:08 Cronos87

Yes, indeed, that would be nice, but the proposal is limited to the documentation comment. I don't know what people think about it and if we should extend the feature to comments too.

Regarding the Kate-like behavior, I agree that it's too much and requires a separate proposal. But I think it would be reasonable to discuss extending the feature to regular comments as part of this proposal/PR.

In fact, it would even simplify the implementation. When I added doc comment highlighting, I didn't add a new API to CodeEdit, since it was not necessary. The "doc comment" term was only introduced in the editor, not the scene.

Also, this can break compatibility in some ways. Previously, doc comments were considered comments, so is_in_comment() returned true for them. Now doc comments are separated into a new category, although it would be more logical to consider them a subset of comments.

dalexeev avatar Aug 27 '24 16:08 dalexeev

Regarding the Kate-like behavior, I agree that it's too much and requires a separate proposal. But I think it would be reasonable to discuss extending the feature to regular comments as part of this proposal/PR.

I totally agreed with you. I can start working on this.

In fact, it would even simplify the implementation.

Yes, it would be easier to implement it for both.

Also, this can break compatibility in some ways. Previously, doc comments were considered comments, so is_in_comment() returned true for them. Now doc comments are separated into a new category, although it would be more logical to consider them a subset of comments.

Sadly, my knowledge regarding the Godot codebase is limited; that's why I tried to implement this proposal to gain experience in both the Godot codebase and C++.

I would be glad to have some clues about how to implement itβ€”just a clue, not the full implementationβ€”and I will give it a try to respect the best practices in this codebase.

For example, is Code Edit the right place to implement it? And can you develop your idea to consider them as a subset, please?

By the way, what do you think, is it necessary to add an option to enable / disable this feature?

Many thanks for the help!

Cronos87 avatar Aug 27 '24 19:08 Cronos87

I think this behavior should only be on doc comments as initially proposed, if it is on regular comments it may be annoying to add new lines for regular code around comments. Though the implementation can be generalized for all types of delimiters if it is easier.

Also this shouldn't cause compatibility issues, CodeEdits behavior should stay the same, since the user hasn't specified to add doc comments instead of regular comments, so all the regular comment delimiter functions should work the same.

By the way, what do you think, is it necessary to add an option to enable / disable this feature?

It shouldn't be needed to have a dedicated enable option, a user can just not use doc comments, or not have a newline delimiter key, depending on how it is implemented.

kitbdev avatar Aug 29 '24 18:08 kitbdev

It seems like opinions are divided regarding expanding this behavior to regular comments. Originally, my idea was to reproduce the VSCode behavior that I used for years, and I always missed the autofill for the documentation block.

I'm new to Godot Core development, so how is this kind of situation resolved? How do we decide which way to take?

I'm waiting for new opinions before start working on this PR again :)

Cronos87 avatar Aug 30 '24 12:08 Cronos87

I'm not exactly qualified in this area of the code, but I'll try helping based on what I understand and know about the topic. Maybe others can chime in as well to give advice on next steps to make this PR work out!

CodeEdit is what Godot users can use in their projects, so any changes in there are likely to break existing projects that use this UI element, and would need new boolean options and keep compatibility in mind. So it would make sense to limit your changes to just editor-only classes (anything in the editor folder). From the ScriptTextEditor, you can get access to its CodeEdit via code_editor->get_text_editor(), but there does not seem to be any way to detect if _new_line was called.

So now the decision is either to implement everything inside CodeEdit and try to avoid changing the API too much (so that old projects still work same as before, and can opt-out of this feature if undesired). Or you add new API, e.g. a signal to detect when a new line was added, which can then be used to implement this from entirely within ScriptTextEditor.

I'm new to Godot Core development, so how is this kind of situation resolved? How do we decide which way to take?

Regarding implementation: Ideally the original proposal should include discussion about the actual implementation, rather than just the end result. So before working on a PR, you already write down your ideas on which APIs need to be edited, list of new editor settings, etc. This way, concerns about the implementation can be found before you spend too much time writing the actual code ^^

As for opinions, people voted for the proposal in the state of only handling doc comments, so I think it would be reasonable to limit its scope to that. Follow-up PRs can always be made later on, if new proposals also gather some interest.

RedMser avatar Aug 30 '24 19:08 RedMser

Many thanks for the clarification @RedMser, that's really helped me understand the workflow better! πŸ™

The clues you gave me are really useful for enhancing my understanding. Signal seems to be a good idea, but I need to dig into the code first and understand it. I'll give it a try this weekend and come back with a better PR when I'm satisfied with it!

I maintain a strong motivation to see this behavior merged one day!

Cronos87 avatar Aug 30 '24 19:08 Cronos87

I'm quite happy with the new implementation; it seems much better and more flexible. However, I still need to implement the case with a delimiter like /** */, where the newline delimiter is different. It is on my to-do list.

I still have a few questions:

  • Globally, is this implementation the right way to do it?
  • In script_editor_text.h, is there a workflow to place the function definition?
  • In the new signal, is is_inserted_above the correct way to name variables? Because I saw in the ScriptTextEditor new line function, it is called p_above; so I don't know which convention to follow.

Many thanks for the answers πŸ™

Cronos87 avatar Sep 02 '24 11:09 Cronos87

To me it looks like a pretty clean approach to implementing this, though I've not looked into the actual logic, just the files modified ^^

It looks like you edited the documentation XML by hand, since the new signal is not ordered alphabetically. For the future, you should use doctool to generate the documentation structure, which you then fill by hand. I think if you run it now, it will keep your docs changes, but move them to the correct place in the file :+1:

RedMser avatar Sep 02 '24 11:09 RedMser

It looks like you edited the documentation XML by hand

Yeah, haha, you got me! I didn't know about it! It will help me because every time I break the CI πŸ˜…

Thank you! I pushed the fix.

I think if you run it now, it will keep your docs changes, but move them to the correct place in the file πŸ‘

For general knowledge, it moved the signal to the right place but deleted the description. So it must be done before 😊

EDIT: I migrated to a new computer, and I didn't reinstall pre-commit; that is why I can successfully commit the doc. It is fixed now ^^

Cronos87 avatar Sep 02 '24 12:09 Cronos87

~TODO: Using complex operations without adding comments generates an error. Need to be sure a doc comment will be added.~

EDIT: Fixed in the last commit.

tx->begin_complex_operation();
tx->begin_multicaret_edit();

Cronos87 avatar Sep 03 '24 16:09 Cronos87

Every features I wanted to implement are now pushed, and it works with C# delimiters.

I still need clarification about the file core/object/script_language_extension.h. I don't know if I did it well.

Any feedbacks and suggestions are very welcome πŸ™πŸ»

Cronos87 avatar Sep 16 '24 19:09 Cronos87

I give it a try using callbacks instead of a signal. Please be aware that this is my first step in the C++ world; it might not be perfect, and I would be glad to have feedback to help me improve myself 😊

Cronos87 avatar Sep 25 '24 12:09 Cronos87

Using the standard library isn't generally allowed, and lambdas are discouraged so I'm not sure using std::function is going to be accepted here if it's not necessary, I'd say you should use Callable instead, for that case you'd use callable_mp(this, &ScriptTextEditor::_auto_fill_doc_comments)

AThousandShips avatar Sep 25 '24 13:09 AThousandShips

Yeah, I discover new things every day on this project; that's great :) Thank you for pointing this out to me!

I will change it. That aside, is the implementation in general OK, or are there other things I need to change?

Thank you for the review πŸ™

Cronos87 avatar Sep 25 '24 13:09 Cronos87

I'll do a pass over this today and give you my review!

AThousandShips avatar Sep 25 '24 13:09 AThousandShips

I changed to Callable as recommended.

I'll do a pass over this today and give you my review!

Many thanks for your time πŸ™πŸ»

Cronos87 avatar Sep 25 '24 16:09 Cronos87

Many thanks for the review and suggestions! I would be glad to work on some improvements that could be discussed in the proposal.

Cronos87 avatar Oct 03 '24 14:10 Cronos87

I think this should be in CodeEdit like before, not in the ScriptTextEditor.

  • It is general behavior, not specific to the Editor. It can be available to users.
  • It should be available in the shader editor. With the current implementation, it will have to be copy and pasted to text_shader_editor.h. See https://docs.godotengine.org/en/stable/tutorials/shaders/shader_reference/shading_language.html#comments.
  • In CodeEdit, it can have unit tests to ensure functionality.
  • There is no concern over compatibility, since users have to opt in to the new behavior by setting the block delimiter.

For a simple implementation, we can just add a block_key to Delimiter in CodeEdit with a getter and setter, and use its presence to change the new line behavior. We don't even need to add a separate 'doc comment' type. This way, existing is_in_comment calls in the editor will work as before.

kitbdev avatar Oct 03 '24 15:10 kitbdev

There is already quite a lot of effort put into this PR, so I want to be careful. But are we really sure this is a good approach? In my opinion, it looks a bit overcomplicated: regular comments vs doc comments, ScriptLanguage vs shaders, editor-only vs CodeEdit vs TextEdit. The three-part doc comment structure (delimiter_begin, delimiter_block, delimiter_end) also looks quite complicated.

Could we instead introduce a simpler "Smart Newline" feature, like in Kate: when you press Shift+Enter, the editor inserts a new line and repeats the prefix of the previous line, which includes whitespace and non-alphanumeric characters. This feature would be useful for regular comments, doc comments, Markdown lists, etc.

Examples (| denotes the cursor position and ^ symbols underline what was inserted after you press Shift+Enter):

# This is comment.
# |
^^

    # This is comment with indentation.
    # |
^^^^^^

## This is doc comment.
## |
^^^

## [codeblock]
## func f():
##     # This is comment in doc comment's codeblock.
##     # |
^^^^^^^^^

- This is Markdown list item.
- |
^^

dalexeev avatar Oct 03 '24 16:10 dalexeev

I appreciated the thinking @dalexeev, thank you! I admit this feature is important for me as I like to write documentation. But, I want it to fit everyone's expectations; which is hard I know πŸ˜…

Seems like we are in a dead end. I will give a try at Kate to understand better the behavior you are explaining. In the meantime, how to solve this situation? 😁 Who can take the decision in which way to go?

Once again, thank you everyone for the reviews, that's great to see everyone way of thinking to enhance this PR πŸ™πŸ»

Cronos87 avatar Oct 03 '24 17:10 Cronos87

"Smart Newline" feature

Since it would be on a different hotkey, it would be harder to discover, but also simpler to implement. It could just be a function in TextEdit. The hotkey Shift+Enter is also unused. I don't like the name Insert Smart Newline that Kate uses, its not very clear about what it does. I also couldn't find much about it online and its not in any other IDEs as far as I can tell. We don't support markdown, and markdown editors I've used just auto inserts - on regular enter (including GitHub). Smart Newline would also copy unwanted symbols when used for this purpose, for example: - [link](url) would copy the [. (related https://github.com/godotengine/godot/pull/78312) I'm not sure about the utility for other symbols, it only seems useful for comments and doc comments. There's not much use in copying the @ of an annotation. It would also not auto indent when used on a line with :.

It doesn't detect if the user is on a documentation line, so it is up to @Cronos87 if it fulfills the proposal.

Also I don't think the current approach is over complicated, it just adds doc_comment_block_delimiters to ScriptLanguage, passes them where needed, and then inserts them on a new line. I think the current approach is better since users don't need to remember a different shortcut when writing doc comments.

kitbdev avatar Oct 03 '24 18:10 kitbdev

We don't support markdown

.md files are editable in the built-in editor. See also:

  • #78312

I think the current approach is better since users don't need to remember a different shortcut when writing doc comments.

I think it would be useful not only for doc comments, but also for regular comments. I really dislike that the feature is limited to doc comments, since I often write multiline regular comments.

Also see the case with comments inside doc comment's codeblocks. This is why I suggested the alternative. Can you suggest other option that would be so simple and functional as Kate's Smart Newline? Another example:

var s := "First" \
        + "Second" \
        + "|
^^^^^^^^^^^

Smart Newline would also copy unwanted symbols when used for this purpose

Fair, but personally I used to the behavior in Kate, its pros outweigh the cons for me. We could also use a setting for auto-inserting symbols, to reduce the false positives. And use #/*- by default (comments and Markdown lists).

dalexeev avatar Oct 03 '24 18:10 dalexeev

My goal in this PR/proposal is to replicate the behavior of VSCode, which I think is great and a lot of people use it daily. I would like this feature to make them feel at home using the Godot Editor. Obviously, not everyone uses it, but most of the editors have this behavior.

As @kitbdev said, I fear that fewer people will discover the feature, and I'm not sure everyone reads change logs like they were the last bestseller, even if I have no doubt they are 😊

Is Kate able to split a sentence and still add the doc comment? (See the video around 15 seconds)

As I said, I'm open to discussion. We just need to find a consensus regarding the behavior.

Cronos87 avatar Oct 04 '24 08:10 Cronos87