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

Do not bother with line colors if line_number_gutter is not yet calculated #84907

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

miv391
Copy link
Contributor

@miv391 miv391 commented Nov 14, 2023

Fixes #81135

When code files are changed with an external editor, all files in script list are reloaded. Files are validated and then _update_errors() is called for each file and type safe lines are colored by _update_errors() calling TextEdit's set_line_gutter_item_color() with line_number_gutter as a parameter. line_number_gutter is however calculated only when a file is opened to the edit view. When a project is opened, only one file is opened to edit view and only that file's line_number_gutter is calculated. If any file is now saved in external editor, all other files have line_number_gutter == -1 and the error message "Index p_gutter = -1 is out of bounds" is spawned for every line of those files. If the user opens all files in script list to the edit view before saving in the external editor, every file will have it's line_number_gutter calculated and there will be no errors.

This is now fixed simply by skipping the setting of line colors altogether if the line_number_gutter is not yet calculated. The script is not shown, so it doesn't matter. When the user opens the script file to the edit view, line_number_gutter is calculated and also _update_errors() is called again.

This is now fixed by not calling _validate_script() from reload_text() if editor_enabled == false.

@miv391 miv391 requested a review from a team as a code owner November 14, 2023 19:23
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 14, 2023
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 14, 2023
@akien-mga akien-mga changed the title Do not bother with line colors if line_number_gutter is not yet calculated Do not bother with line colors if line_number_gutter is not yet calculated Dec 12, 2023
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.

Thanks for the detailed description!

Looks like the root cause is the editor lazy loading, as you pointed out. Once the tab is loaded _validate_script is connected / called. So we should probably not call it in the first place if the editor is not enabled, as there might be other things in an unexpected state. Would guess it's called from reload_text.

@akien-mga
Copy link
Member

@miv391 Would you be able to look into @Paulb23's suggestion for fixing the issue earlier on?

I tried to reproduce #81135 myself to see if I could do it but didn't manage to trigger the problem in 4.3.dev so far.

@miv391
Copy link
Contributor Author

miv391 commented Feb 14, 2024

I cannot reproduce the bug with v4.3.dev2.official either.

Edit: Yes, I can reproduce the bug after all.

@@ -662,6 +662,10 @@ void ScriptTextEditor::_update_errors() {
errors_panel->pop(); // Indent.
}

if (line_number_gutter == -1) {
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth adding a comment to describe why this early return is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix changed completely. The current fix is a bit more self explanatory.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I can reproduce the original issue with the MRP from #81135 in 4.3.dev 7d2ca2d, and this PR fixes it (Linux + VS Code).

@miv391
Copy link
Contributor Author

miv391 commented Mar 1, 2024

I tried the solution suggested by Paulb23 and it seems to work, so I changed what this fix does. Now _validate_script is not called at all if editor_enabled == false. So even less is done if the file is not yet opened in the edit view.

@akien-mga akien-mga requested a review from Paulb23 March 4, 2024 16:52
@akien-mga akien-mga dismissed Paulb23’s stale review March 4, 2024 19:38

Requested changes done, presumably.

@akien-mga akien-mga merged commit 4320d53 into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@miv391 miv391 deleted the fix-gutter-error branch March 4, 2024 20:43
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

Index p_gutter = -1 is out of bounds error when saving file in external editor
5 participants