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

Code Editor: Fix regression with using doc comments for code regions #83216

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Oct 12, 2023

#region New Code Region
<code>
#endregion
##region New Code Region
<code>
##endregion

This is bad because ## is used for doc comments.

The reason of the regression is that the code editor selects the first comment delimiter, and the delimiter are sorted internally in descending order of delimiter length. I'm guessing the sorting was implemented incorrectly and was originally intended to be in ascending length order.

Addition order: ", ', """, ''', #, ##

Storage order (before): ''', """, ##, #, ', "

Sorting
"
', "
""", ', "
''', """, ', "
''', """, #, ', "
''', """, ##, #, ', "

Storage order (after): ", ', #, ##, """, '''

Sorting
"
", '
", ', """
", ', """, '''
", ', #, """, '''
", ', #, ##, """, '''

After changing the sorting, the regression is fixed, the shortest comment delimiter is selected for the region, I see a lot of sense in this. I didn't notice any other regressions due to this change.

I also fixed the sorting in a similar highlighter code (originally copied from the code editor).

@dalexeev
Copy link
Member Author

I thought about it and now I'm not sure if the order was originally intended to be in ascending length order. If one delimeter is a prefix of another, then it makes sense to check the longer delimeter first to support both. In this case, a better fix would be to invert the loop direction here:

godot/scene/gui/code_edit.cpp

Lines 2856 to 2875 in b137180

/* Code Region */
void CodeEdit::_update_code_region_tags() {
code_region_start_string = "";
code_region_end_string = "";
if (code_region_start_tag.is_empty() || code_region_end_tag.is_empty()) {
return;
}
for (int i = 0; i < delimiters.size(); i++) {
if (delimiters[i].type != DelimiterType::TYPE_COMMENT) {
continue;
}
if (delimiters[i].end_key.is_empty() && delimiters[i].line_only == true) {
code_region_start_string = delimiters[i].start_key + code_region_start_tag;
code_region_end_string = delimiters[i].start_key + code_region_end_tag;
return;
}
}
}

@dalexeev dalexeev force-pushed the code-editor-fix-region-doc-comment branch from 730092a to 580bca0 Compare October 12, 2023 21:40
@Paulb23
Copy link
Member

Paulb23 commented Oct 12, 2023

Yeah the longer delimiter should take priority in the highlighter. As #74843 is searching for the first comment it probably finds ## first after #72751, so reversing the region comment delimiter search makes sense to me.

@dalexeev dalexeev force-pushed the code-editor-fix-region-doc-comment branch from 580bca0 to 881fe67 Compare October 13, 2023 06:38
@akien-mga akien-mga merged commit cd9fd6d into godotengine:master Oct 13, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the code-editor-fix-region-doc-comment branch October 13, 2023 09:57
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.

3 participants