-
Notifications
You must be signed in to change notification settings - Fork 65
fix: reduce LSPTextHover resize flicker #1449
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
base: main
Are you sure you want to change the base?
Conversation
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/hover/LSPTextHover.java
Outdated
Show resolved
Hide resolved
...eclipse.lsp4e/src/org/eclipse/lsp4e/operations/hover/FocusableBrowserInformationControl.java
Show resolved
Hide resolved
|
|
||
| if (!"complete".equals(safeEvaluate(browser, "return document.readyState"))) { //$NON-NLS-1$ //$NON-NLS-2$ | ||
| UI.getDisplay().timerExec(200, () -> updateBrowserSize(browser)); | ||
| UI.getDisplay().timerExec(50, () -> updateBrowserSize(browser)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that until we solve #1396, lowering this will increase the exceptions, so I would leave this for later. I think most likelz the other changes are enough. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case where I see the resize flicker it only gets considerably improved if I reduce the value to 50ms or lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to address the SWTException issue as part of this PR too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a second commit that should allow faster reschedules of updateBrowserSize without throwing the SWTExceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the exceptions. I think that even though the code will not throw the SWTException, if it happens, the dialog will not have the proper size, which not good from the user point of view. I will try to put aside some time till the end of the week to implement the solution I suggested #1396.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I cannot reproduce the exception. But if it occurs a new resize attempt will be done 20ms later so this should result in a correct size.
| updateBrowserSize(browser, 1); | ||
| } | ||
|
|
||
| private void updateBrowserSize(final Browser browser, final int attempt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebthom , can you rebase your PR? Now that I think I have solved the problem with safeEvaluate returning exceptions, I think you can go back to your original attempt which was much simpler.
rubenporras
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See last comment.
Fixes #1445