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

Rework code editor's multiline operations #72671

Merged

Conversation

aXu-AP
Copy link
Contributor

@aXu-AP aXu-AP commented Feb 3, 2023

Fixes #54362
Partially fixes #72410

This pr aims to rework multiple code editor's line operations for more consistent behavior. Main takeaways are:

  • Fix mixed behavior when there are multiple carets or selections on same line.
  • Unify behaviour when operation is toggleable, ie. affect all the selections same way instead of handling them separately. This is also the main cause of the first point.
  • Fix some operations affect line, on which the selection ends but no characters are selected. For this to make more sense, see also Selection visual at end of the line inconsistent #72265.

Delete lines

There were some issues with delete lines operation, fixing them separately would likely end up filling the function with more band-aid than anything, so rewrite seemed more appropriate.
Issues fixed:

Toggle comments

Fix a bug where multiple selections affect one line. Rearranges the flow of the function, but takes largely the same steps as original.
Changed behavior: handle all selections as one instead of separately. I'm not sure if there's a reason to toggle some selections on and some off, and this helped to simplify the logic, so I went with it.

Toggle breakpoints

Fix a bug if you toggle breakpoints when 2 carets are on the same line.
Changed behavior: The operation now removes all breakpoints in selection if there is any. This is useful after you have done debugging a certain part of the code (but don't want to clear breakpoints from the whole file). For consistency this works in inverse too, breakpoints are for every line selected if there's none currently in selection.

Toggle bookmarks

Similiar to toggle breakpoints.

Move lines up/down

Fix the bug when the selection ends at the start of new line, move lines up/down shouldn't affect that line.

editor/code_editor.cpp Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

Awesome work! I'll take a closer look later to make sure the reworks are robust, but for now here are a few things to fix:

  • Toggle Folding (Alt+F) is affected by the same issue. I thought I'd mention it here because I forgot to in my original issue.
  • Delete Line (Ctrl+Shift+K) has inconsistent behavior on the 1st line, only deleting its contents. It also leaves an error if the beginning of the selection is an empty line and the main caret is at that empty line
  • Toggle Comment (Ctrl+K) doesn't move the caret forward if it's at the beginning of the line
  • Move Down doesn't stop itself if you select the last line in the script

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Feb 5, 2023

Thanks for testing, I'll fix them (might as well check all the other operations if there is any more). EDIT: to be clear, I'll fix the other operations later. This pr already touches so many operations...

About toggle comment, It seems that this is the intended behavior. The original code (and my code) has an extra check to make this happen. I don't have any preference on this, both ways it makes sense, but I found out when this was implemented: #22738 (review)
I do kind of like the look of it: if you selected the whole line, visually the selection doesn't change. @Paulb23 was that the reasoning, or is there more into it?

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.

There quite a lot here, gave a initial look over.

I do want to move these methods into CodeEdit, so if you feel up to it feel free to move them there and then we can take advantage of some unit tests.

editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/plugins/script_text_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
@Paulb23
Copy link
Member

Paulb23 commented Feb 5, 2023

I do kind of like the look of it: if you selected the whole line, visually the selection doesn't change. @Paulb23 was that the reasoning, or is there more into it?

Yeah we should try and get the intention of the user, if the whole line is selected we should keep that way.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Feb 5, 2023

I do want to move these methods into CodeEdit, so if you feel up to it feel free to move them there and then we can take advantage of some unit tests.

Yes, this is what I've been thinking as well. I can try to do it. I have no experience in writing unit tests, but I'll write on contributor chat if I need help with those.

To be clear, do you think I should modify this pr to move them, or should we finish this first and then do the move? Do we have time to make it in 4.0 (since it would be an addition to the api) or wait for 4.1? Since rc phase is "just days away", I'd rather have more robust editor for 4.0 and nice additions to CodeEdit on next release.
Also, here's a list of operations I think should be moved into CodeEdit, do you agree?

  • Delete lines
  • Toggle comments
  • Toggle bookmarks
  • Toggle breakpoints (this needs a bit of extra logic from script editor to add breakpoints into debugger)
  • Move lines up/down
  • Duplicate selection (haven't touched to this in this pr, but also makes sense)

@Paulb23
Copy link
Member

Paulb23 commented Feb 5, 2023

Yes I was planning to rewrite and move these as part of 4.1 anyway, not sure how many editor assumptions we need to remove / general cleanup for API use at the same time. I'm fine if the move is handled in a later change. I wouldn't say this is a blocking issue 4.0 so if it gets in then great, if not 4.1 is fine. No need to "crunch" for it. 4.1 should be a much quicker cycle (at least compared to 4.0 ;) ).

Also, here's a list of operations I think should be moved into CodeEdit, do you agree?

Yep, all the "general" code editing methods should move, including the convert indent, trim whitespace etc

@aXu-AP aXu-AP force-pushed the code-editor-delete-lines-rewrite branch 2 times, most recently from ca3c50d to 436b6c1 Compare February 7, 2023 21:34
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Feb 7, 2023

I believe I have addressed all the problems now. Remind me if I forgot something, or something new has appeared. I tried to give these operations a good battering though, they do seem quite robust now 😄

@aXu-AP aXu-AP requested a review from Paulb23 February 7, 2023 21:58
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.

This is looking great! couple small comments, hopefully last pass.

editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/plugins/script_text_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
Fix bugs if 2 selections were on same line.
Fix bugs when selection ended at new line.
Make carets stay in place after operation and on undo.

Affects: delete lines, move lines, toggle comments, bookmarks and breakpoints.
@aXu-AP aXu-AP force-pushed the code-editor-delete-lines-rewrite branch from 436b6c1 to bdfb10f Compare February 12, 2023 18:54
@YuriSizov YuriSizov merged commit 3193efa into godotengine:master Feb 12, 2023
@YuriSizov
Copy link
Contributor

Amazing work, thanks!

@aXu-AP aXu-AP deleted the code-editor-delete-lines-rewrite branch February 15, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants