fix: prevent stuck "Loading..." hover#1394
Conversation
sebthom
force-pushed
the
issue-1388
branch
2 times, most recently
from
a9bedaf to
dac2d52
Compare
| }); | ||
| }) | ||
| // Ensure the placeholder is not stuck if LS never replies | ||
| .completeOnTimeout(null, GET_HOVER_CONTENT_TIMEOUT_SECONDS, TimeUnit.SECONDS); |
There was a problem hiding this comment.
I am not convinced that closing the pop-up is a good thing to. Is it actually a UI problem that the UI shows loading if the server is stuck? I would rather say that is a problem for the server, as the user can always dimiss the dialog when he is tired of waiting by moving away the cursor, typing, closing the editor...
If the problem is that it appears that the UI is stuck, could we maybe have some dots appears one by one using a timer and the cycling to one dot after a bunch of those?
There was a problem hiding this comment.
Yeah, I cannot reproduce the issue that @FlorianKroiss described. So I just added this guard. Maybe we should cancel the initiateHoverRequest after some time and swap out the "Loading..." message by something else?
There was a problem hiding this comment.
The LS is NOT stuck in my case. I think the problem is that a null value that is returned from the future in AsyncHtmlHoverInput is handled incorrectly.
If I replace the dispose() for this case with super.setInput("Nothing to see"); it is properly updated:
The call to dispose() actually disposes the Shell, which is probably not intended?
There was a problem hiding this comment.
@FlorianKroiss I replaced the dispose calls with setText(""), can you please give this snapshot build a try: https://github.com/eclipse-lsp4e/lsp4e/actions/runs/19375743759/artifacts/4572560568
sebthom
changed the title
|
@sebthom , if we can get this one finished tomorrow, I would create another release. |
|
I think this is ready to be merged. |
| } else { | ||
| // No content from LS; hide placeholder | ||
| dispose(); | ||
| super.setInput(""); //$NON-NLS-1$ |
There was a problem hiding this comment.
I think I understand this one and I also think it is the best thing to do. My understanding is: the text "Loading..." will be replaced with the empty string, and the content will be available until the user dismisses it. Correct?
| if (ex != null) { | ||
| LanguageServerPlugin.logError(ex); | ||
| dispose(); | ||
| super.setInput(""); //$NON-NLS-1$ |
There was a problem hiding this comment.
I am not sure about this one. In case of an exception, should we not close the text hover? Or maybe use ex.getMessage() and set that to something like "An exception has happened: ..."
There was a problem hiding this comment.
I changed it to show the error message
Sorry, my mistake, I have not published my review before. I have done it now. I will read your answers tomorrow. Have a nice evening. |
3 participants
Addresses second issue raised in #1388