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 LSP not returning expected localization for API docs. #104062

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

the-loki
Copy link

corresponding language of the editor. This issue arises because the DocTools class does not translate the description when loading the documentation. To address this, I have modified the DocTools class to process the translation of description using _translate_doc_string during loading. Additionally, to prevent caching-related issues, I have added the localization language as a feature to the hash in the _compute_doc_version_hash function within the EditorHelp class.

before:
before

after:
after

@akien-mga akien-mga changed the title fix LSP dose not return correct language doc. Fix LSP returning expected localization for API docs. Mar 13, 2025
@akien-mga akien-mga changed the title Fix LSP returning expected localization for API docs. Fix LSP not returning expected localization for API docs. Mar 13, 2025
@akien-mga akien-mga added this to the 4.5 milestone Mar 13, 2025
@akien-mga
Copy link
Member

I'll need to spend more time to review this properly, but if we change the DocTools loading to perform the localization at this stage, we need to check:

  • Is there any code that expects the loaded class reference to be the original source and not localized?
  • EditorHelp code that fetches translations for the class reference should likely be changed as it's now redundant if the data is already translated.

@akien-mga akien-mga requested review from KoBeWi and dalexeev March 13, 2025 10:09
@the-loki
Copy link
Author

I'll need to spend more time to review this properly, but if we change the DocTools loading to perform the localization at this stage, we need to check:

  • Is there any code that expects the loaded class reference to be the original source and not localized?
  • EditorHelp code that fetches translations for the class reference should likely be changed as it's now redundant if the data is already translated.

Got it. If further modifications are needed, I’m more than happy to do that.

@dalexeev
Copy link
Member

Perhaps the translation should be done in LSP. I am not very familiar with the GDScript LSP implementation and not at all with the VSCode plugin implementation. We should test this hypothesis.

DocTools is responsible for extracting the actual engine API, existing documentation from XML files and merging them (in case some API was added/changed/removed). So it is unlikely that DocTools should deal with translated documentation. Perhaps only as an explicit option, for example for --dump-extension-api-with-docs.

EditorHelp expects DocTools to return untranslated documentation. There is an editor setting that allows disabling documentation translation even if the rest of the editor interface is localized. We can assume a similar VSCode plugin setting, which should not depend on Godot editor settings and should be passed through LSP.

@the-loki
Copy link
Author

Perhaps the translation should be done in LSP. I am not very familiar with the GDScript LSP implementation and not at all with the VSCode plugin implementation. We should test this hypothesis.

DocTools is responsible for extracting the actual engine API, existing documentation from XML files and merging them (in case some API was added/changed/removed). So it is unlikely that DocTools should deal with translated documentation. Perhaps only as an explicit option, for example for --dump-extension-api-with-docs.

EditorHelp expects DocTools to return untranslated documentation. There is an editor setting that allows disabling documentation translation even if the rest of the editor interface is localized. We can assume a similar VSCode plugin setting, which should not depend on Godot editor settings and should be passed through LSP.

I've considered implementing the feature in this way: adding an option to VSCode to set the document language and then modifying the LSP implementation to return the corresponding translation. However, this would require changes to two repositories, so I opted for the approach with fewer modifications. If this method is more suitable, I'm happy to implement the relevant content, but I still need some time to familiarize myself with the LSP implementation.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 14, 2025

Storing translated strings in docs cache means you need to re-create the cache every time language is changed.

Is there any code that expects the loaded class reference to be the original source and not localized?

Localized class reference would make it more difficult to switch languages.

If there is a way to localize the strings on LSP/VSCode side, I think that would be better approach.

@the-loki
Copy link
Author

Storing translated strings in docs cache means you need to re-create the cache every time language is changed.

Is there any code that expects the loaded class reference to be the original source and not localized?

Localized class reference would make it more difficult to switch languages.

If there is a way to localize the strings on LSP/VSCode side, I think that would be better approach.

I believe that implementing translation on either the LSP side or the VSCode side is feasible. However, translating on the LSP side and providing configurations for VSCode would require modifications to two repositories. On the other hand, translating on the VSCode side might necessitate including localized po files for documentation, which could increase the plugin package size to some extent. Additionally, we need to maintain the version compatibility between the VSCode extension and Godot. Therefore, we should carefully consider which approach is more suitable.

@the-loki the-loki marked this pull request as draft March 20, 2025 01:55
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.

4 participants