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

Saving settings on other editors fails #32

Closed
LDAP opened this issue Dec 8, 2020 · 5 comments
Closed

Saving settings on other editors fails #32

LDAP opened this issue Dec 8, 2020 · 5 comments
Labels
1-bug 🐛 Issue type: Bug report (something isn't working as expected) 2-unconfirmed Issue status: Bug that needs to be reproduced (all new bugs have this label) 3-not-a-bug Issue resolution: LTeX behaves as expected, or bug cannot be reproduced

Comments

@LDAP
Copy link
Contributor

LDAP commented Dec 8, 2020

Describe the bug
Saving settings on other editors fails

Steps to reproduce
Use any code action that modifies user settings

Expected behavior
Actually not sure - is there any specification in the LSP?

Actual behavior
ltex-ls responds with false

Connected to LDAP/LSP-ltex-ls#3

@LDAP LDAP added 1-bug 🐛 Issue type: Bug report (something isn't working as expected) 2-unconfirmed Issue status: Bug that needs to be reproduced (all new bugs have this label) labels Dec 8, 2020
@LDAP
Copy link
Contributor Author

LDAP commented Dec 8, 2020

I looked into the commits, and it seams that the external settings-file management was moved out of ltex-ls.

A possible solution would be to move it back to ltex-ls and then expose
LTEX_GLOBAL_STORAGE_PATH, WORKSPACE, WORKSPACE_FOLDER to settings.
(I am reffering to the naming from here)

Part of the information can also be gathered from the initialize(1) command (using the workspaceFolders key).

@valentjn
Copy link
Owner

valentjn commented Dec 8, 2020

The LSP spec treats the client as responsible for managing the configuration (in this case: dictionary, disabled rules, etc.); the server retrieves it via workspace/configuration or workspace/didChangeConfiguration.

Consequently, ltex.addToDictionary and ltex.disableRules are handled by the client, which updates the configuration accordingly.

What was moved from ltex-ls to vscode-ltex was not really management of (external) settings, but reading in external dictionary files specified as special entries (colon followed by a path) in the dictionary the client sent to the server. Before the point in time when this was moved,

  • writing to external files (i.e., adding a word to the external dictionary file) was not possible,
  • implicit defaults for external setting files were not supported, and
  • external files were only possible for the dictionary setting, not for disabled rules or hidden false positives.

To implement these features (which were driven by popular demand), and as the client should be responsible for managing configuration, reading external files was moved to the client. Before that, ltex-ls only “replaced” the special entries in the client-supplied dictionary with the contents of the external files; there was no real management of settings on the server side.

@LDAP
Copy link
Contributor Author

LDAP commented Dec 8, 2020

Okey, this means its a client-side issue only.

I will look into this more deeply in the next weeks. Does the server expect a didChangeConfiguration or configuration after ltex.addToDictionary and ltex.disableRules?

I must admit I am not that familiar with the protocol itself. It was just enought to extend the existing framework on sublime to support ltex-ls.

@LDAP LDAP closed this as completed Dec 8, 2020
@valentjn
Copy link
Owner

valentjn commented Dec 9, 2020

workspace/configuration is a server-to-client request. workspace/didChangeConfiguration (a client-to-server notification) is "more correct," as it will trigger a recheck of all open documents.

"More correct" because LTEX LS reads the settings dictionary, disabledRules, enabledRules, and hiddenFalsePositives from the workspace-specific configuration. So what happens if you send a workspace/didChangeConfiguration notification to LTEX LS is that it sends an ltex/workspaceSpecificConfiguration request to the client, expecting the client to correctly merge the settings of the different scopes (not sure if Sublime has those), incorporate the external setting files, and send the resulting lists back to the server. The result will then be used for the actual checking.

@valentjn valentjn added the 3-not-a-bug Issue resolution: LTeX behaves as expected, or bug cannot be reproduced label Dec 9, 2020
@LDAP
Copy link
Contributor Author

LDAP commented Dec 9, 2020

I already implemented these custom commands and it seams that the LSP framework on sublime does the work for me in this case.

See LDAP/LSP-ltex-ls#4

That means LSP-ltex-ls is now fully functional. But without the per-project/workspace settings those are vscode-only (but may be implemented in the future). I do only add the exceptions to the user settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-bug 🐛 Issue type: Bug report (something isn't working as expected) 2-unconfirmed Issue status: Bug that needs to be reproduced (all new bugs have this label) 3-not-a-bug Issue resolution: LTeX behaves as expected, or bug cannot be reproduced
Projects
None yet
Development

No branches or pull requests

2 participants