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

[#808] Support overriden default-editor-association in EditorUtility #809

Merged

Conversation

ghentschke
Copy link
Contributor

@ghentschke
Copy link
Contributor Author

@jonahgraham can this be part of the next release or am I too late?

@ghentschke
Copy link
Contributor Author

Note: the cdt-lsp project overrides the default editor associations to ensure that the LSP editor settings on project level will be considered:
image
Context: As a C/C++-Developer I want to use for my C/C++ projects in my workspace for some projects the old and for some the new LSP-based editor.

@ghentschke ghentschke added this to the 11.6.0 milestone May 31, 2024
@jonahgraham
Copy link
Member

I am pretty comfortable with the last minute changes for the CDT-LSP components, but this is pretty core in CDT and we won't have time to respin if there are unintended consequences. Can we leave it for after this release? If you need it in this release, let me know and I will investigate more to see if we can squeeze it in.

@ghentschke
Copy link
Contributor Author

If you need it in this release, let me know and I will investigate more to see if we can squeeze it in.

I know that this is pretty late but I found this issue in the morning. IMO it's a bigger bug when users want to work with old an new editor in the same workspace (as we do in out product) and we want to update our IDE after this release. So it would be great if you have the time to check this PR.

@jonahgraham
Copy link
Member

OK - I will have a look in detail. If I don't see too much concern I will merge this before RC2 build.

IEditorRegistry registry = PlatformUI.getWorkbench().getEditorRegistry();
IEditorDescriptor desc = registry.getDefaultEditor(input.getName(), contentType);
IEditorDescriptor desc = null;
if (input instanceof ExternalEditorInput externalInput) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO the risk here is moderate, because this code will be reached only if the input is not an instance of IFileEditorInput and it must be an instanceof ExternalEditorInput which limits the call on non workspace files as well. However, when this code should throw an exception, the previous behavior is ensured (desc is still null).

Copy link
Member

Choose a reason for hiding this comment

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

Can you have a look at

} else if (input instanceof IURIEditorInput) {
IURIEditorInput uriEditorInput = (IURIEditorInput) input;
URI uri = uriEditorInput.getURI();
try {
IFileStore fileStore = EFS.getStore(uri);
id = IDE.getEditorDescriptorForFileStore(fileStore, false).getId();
} catch (CoreException e) {
// fallback to default case
}
and let me know if that pattern (which is also used in LSP4E) is more general and applicable here?

Besides that question - I agree this change is low risk and worth getting in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it works as well. Good point!

@jonahgraham jonahgraham merged commit 9f85daf into eclipse-cdt:main May 31, 2024
5 checks passed
@jonahgraham
Copy link
Member

Thanks @ghentschke - I have merged now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants