Add support for quick doc hovers (support for opening files) #149
Conversation
| }); | ||
| } | ||
| } catch (MalformedURLException | URISyntaxException ex) { | ||
| LOGGER.error("Error opening doc link", ex); |
There was a problem hiding this comment.
Shall we log this as a warning and prompt a message to the user instead of logging as an error? (AFAIR error logs will also be shown as fatal errors, which might make this library looks unstable.. so currently I have to log the exceptions as warnings and I'm not sure whether we have alternatives) WDYT?
There was a problem hiding this comment.
Sure may be we can log this as a debug message and show a notification which will be shown in the event log. WDYT ?
There was a problem hiding this comment.
I'm afraid that the event log will be noisy if we continue prompting these kind of exceptions since it's shared by all the tools for prompting critical events :/ How about using the IntelliJ Messages API?
Messages.showErrorDialog(message, title);
There was a problem hiding this comment.
I meant to just show a info event in the event log. I don't see that we will get lot of such errors ?
Of cause we can use the Message API but then user will need to close the popup right ? Will that be better from UX point of view ?.
There was a problem hiding this comment.
@gayanper @NipunaRanasinghe my suggestion is also to move ahead with the Messages API. If we are going to add this into event log; we need to treat & add all error messages in other places in same level and I am afraid that will flood the events log. IMO since this action is user-initiated (clicking the hyperlink); showing error message in a failure won't harm the UX. WDYT?
|
Sorry for offtopic... Could you post some screenshots or gifs with $subject? |
Yeah that would be nice |
|
Do you want a gif on the quick doc with clickable links ? |
|
@gayanper Hopefully now you can proceed with this by resolving the conflicts as I've merged the other PRs. :) And as @nixel2007 requested, it would be great if you can update the PR description with an sample image/GIF so that the others can see the styling changes, if there are any. |
|
@nixel2007 @NipunaRanasinghe i have update the PR description with a gif. |
|
@gayanper sorry, i can't get the idea. what's the difference with regular hovers sent from LS? html support with |
|
This is a regular hover, but in this the hyperlinks can be actually clicked and navigate to Files. For example if you click the Type link it will open that class file in java editor. if you click the http link it will be opened in the browser etc. |
|
ah, and you open files via URI parsing and VFS. Understood, thanks. Cool! |
| if ((!LanguageDocumentation.INSTANCE.allForLanguage(language).isEmpty() && !language | ||
| .equals(PlainTextLanguage.INSTANCE)) || (!getIsCtrlDown() && !EditorSettingsExternalizable | ||
| .getInstance().isShowQuickDocOnMouseOverElement())) { | ||
| if ((!LanguageDocumentation.INSTANCE.allForLanguage(language).isEmpty() && !isPlainTextLanguage(language)) |
There was a problem hiding this comment.
Could you please explain the reason behind this cache based new check?
language.equals(PlainTextLanguage.INSTANCE) isn't working for you?
There was a problem hiding this comment.
Languages like java doesn't inherit the plain text language. Thats why this new check is added.
There was a problem hiding this comment.
Is it not working only for java? how about using a isFileSupported() check from our FileUtils instead, because you are registering a server definition for the java file extension?
There was a problem hiding this comment.
@NipunaRanasinghe i think this is a really good suggestion. I have changed the code now to use FileUtils check.
There was a problem hiding this comment.
@gayanper great :)
79bb96a to
a67af8a
Compare
| @@ -297,6 +296,11 @@ public void mouseMoved(EditorMouseEvent e) { | |||
| } | |||
| } | |||
|
|
|||
| private boolean isPlainTextLanguageFile(PsiFile file) { | |||
There was a problem hiding this comment.
| private boolean isPlainTextLanguageFile(PsiFile file) { | |
| private boolean isSupportedLanguageFile(PsiFile file) { |
There was a problem hiding this comment.
@gayanper WDYT since now we check for the registered server definitions as well?
There was a problem hiding this comment.
I think thats a good suggestion. I made the change and pushed it
a67af8a to
1a95390
Compare
- enabled HTML formatting for all content since LSP spec is now sending markup content - use a textpane with JB html kit to style the documentation - add support for lanuguage like java which doesn't inherit from plain text language. - add support for open source files from links in quick doc hover - add support for http urls
1a95390 to
644ab73
Compare
markup content
text language.