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

Fix GDScript docs not updating when modified externally #97710

Merged

Conversation

anvilfolk
Copy link
Contributor

Currently on master if you change a GDScript with an external text editor while Godot is open, it will update the script itself but not the documentation. This PR fixes this.

It's a minor regression from #97168, which avoids scripts being reloaded twice by removing one of the two loads. Unfortunately it was the one that remembered to add docs to EditorHelp. Just made sure the remaining one adds docs.

This does highlight a need for us to refactor documentation generation & adding to EditorHelp. Perhaps this responsibility should exist within the Script classes themselves, or at least ScriptServer. They are the ones that know when documentation has been generated and could add them to EditorHelp when TOOLS_ENABLED.

@anvilfolk anvilfolk requested a review from a team as a code owner October 1, 2024 19:48
@anvilfolk
Copy link
Contributor Author

@Hilderin

@anvilfolk anvilfolk force-pushed the think-of-the-dooooooooooooooocs branch from a87c393 to f481933 Compare October 1, 2024 19:55
@anvilfolk anvilfolk requested a review from a team as a code owner October 1, 2024 19:55
@Hilderin
Copy link
Contributor

Hilderin commented Oct 1, 2024

Am I right that this fixes the documentation update for this specific situation:

  • The script is open in the editor and it's updated in an external editor not connected to LSP server and you click "Discard local changes and reload" when you go back to Godot..

I was not able to reproduce the problem in any other situation.

I'm not sure it's really a regression because, even before #97168, an opened script was only updated via the ScriptEditor::_reload_scripts which never updated the documentation.

Anyway, it seems a good correction. Nice catch!!

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Oct 1, 2024

Am I right that this fixes the documentation update for this specific situation:

* The script is open in the editor and it's updated in an external editor not connected to LSP server and you click "Discard local changes and reload" when you go back to Godot..

Yes, exactly that and no other situation that I noticed!

I did a manual bisect by looking at parent commits but I don't think that was smart, since other possibly relevant PRs that were merged before your commit's parent might've affected it. Going to try to properly bisect it just in case :)

@anvilfolk
Copy link
Contributor Author

Ok, so that is the commit that introduced the regression. Specifically, it was removing this piece of code from _update_script_documentation():

Ref<Resource> res = ResourceCache::get_ref(path);
if (res.is_valid()) {
	res->reload_from_file();
}

It makes sense, since using Resource::reload_from_file() will call ResourceLoader::load() with CACHE_MODE_IGNORE, and force-reload the docs. Reintroducing that code makes the docs update but interestingly ⚠️ the code file itself will no longer update. Super weird ⚠️

@anvilfolk anvilfolk force-pushed the think-of-the-dooooooooooooooocs branch from f481933 to 0ad55e9 Compare October 2, 2024 00:06
@akien-mga akien-mga merged commit 15cee4b into godotengine:master Oct 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

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