-
Notifications
You must be signed in to change notification settings - Fork 150
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
remove 500ms PostSelectionDelay #1695
Comments
I also suspect this delay is meant to not trigger action while user is still actively interacting. Users repeatedly pressing a key is a possible and frequent case I believe.
Is this true on all OSs? |
no. its from initial.
i tested on win only. |
CPUs were slower in the old days. IIRC I added a similar delay in the p2 installation dialog as it was constantly freezing if you typed. But if that delay is not helping, I think it is great if we can remove it. These days I use a lot of VS Code (for Flutter development) and the Eclipse IDE feels really slow compared to it (especially saving seems to delay frequently the UI while it should / could just run in the background). |
I thinks we should try 0ms on M1 and see if there is any drawback |
That seems worth trying, and doing it in M1 is certainly the best time to do so. |
This is currently also used for content assistant (auto activation chars?) and addition info hovers (javadoc?), so I think a value > 0 makes sense here or this can especially be disturbing if you move the mouse around maybe flicker a lot then or getting content assist while typing / inserting text. So I would rather at least use a few milliseconds that are still ages for a computer but nothing one should really notice as a human. |
https://developer.mozilla.org/en-US/docs/Web/Performance/How_long_is_too_long says "50ms feels immediate", and I remember we also adopted it in some other Eclipse plugins in the past without much regret. |
My original problem with type hierarchy was any value >0 cause concurrency issues.
If you really experience any issue please post a video about it. i have not see any defect yet. |
Maybe its then better to fix that issue instead of changing a global constant that is used at different places to a number that works best for one view.... |
If we find out why there is any benefit to have any delay that would be needed. Otherwise we can just make UI more responsive. |
We've already found out that the delay is here to prevent useless computation from happening too early in case of repeated fast interactions. |
did we? any proof? |
No proof, it's just the state of art of UI interactions and the reasoning of multiple developers led to the same conclusion. So it has solid support. In absence of a better answer, we'll have to take this one. |
It would be equal "responsive" if we use a smaller delay, 500ms might be noticeable, 50ms most probably not, but we can't claim we i"improve responsiveness" if in fact we want to solve some unkown "concurrency problem" in one component ... |
Multiple developers making the same assumption is no solid support. There have been so many useless waits in eclipse that have been successfully totally removed that you should at least give it a try. |
It is agreed that a smaller delay is suitable, but it seems you want to solve a completely different problem and that's simply wrong. |
Avoiding concurrency at all is a well known pattern to avoid concurrency issues. And if (to be tested) it even helps to improve other things is simply two slain with one |
Not sure about this. In our RCP app we have a Tree component that updates several other parts and components when a selection event occurs on the tree. When using the up and down keys very quickly the 500ms delay allows the tree to be traversed without a delay, but when set to zero there can be a lag as the other parts are updated. |
normaly such updates should be throttled by Throttler see |
All selections are handled in the SWT thread so I don't see where "concurrency" is happen here at all, but again you should not disable something to fix a bug in a component if you even can't explain why it will solve anything, if it is a concurrency problem then just because you don't see it with a timeout of zero does not mean it is gone, it maybe not happen that often that you don't notice it. Alot of effort was recently put into making "Viewers" only show a limited set because if you click an item it was slow, so claiming there are no problem is at least questionable if now we want to update it even more often on each and every cursor move... |
For clarification: I want to use 0 initial delay, but still a positive waiting period between events is ok. The Throttler already provides a distinction of the both concept. And it is working for years. Probably it only has to be used at more places. |
Using Throttler to handle reaction to delayed events so that intermediary events gets ignored is really welcome and fits really well the requirement here. FWIW, Throttler is a relatively recent addition to JFace, so many code that predates it could be improved to use it. |
When I read |
Funny to see the different perspectives here, @mickaelistria sees Throttler as a "recent addition to JFace" (similar to me), but @jukzi see its is "working for years". :-) I guess Mickael and I are now the old guys. I definitely like the approach to react immediately if possible and delay if multiple event happen. IIRC Throttler was implemented to speed up Jobs, as the Job progress reporting took a significant amount of time in the UI thread without the user having the change to see all updates (as our eye frequency is not high enough). |
…rm#1695 immediately calls firePostSelectionChanged by chance. eclipse-platform#1695
You mean the code element information? |
the right pop up with additional information, containing the javaDoc in this example. |
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
ok. Only on windows this is yellow ;-) |
Well then it is simple, just remove all references to |
It possibly is, as said completion is a general purpose component, that it works fast for your testing (with java) does not mean it is always fast (e.g. indexer not run) nor that others (CDT, PDP, WWD...) will always compute it fast enough. |
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
Yes that's true - not all completion proposals are as fast as Javas. In our product the "ABAP Development Tools" we even have to do calls over the network to get completion proposals and also to get the code element information for a selected completion proposal. |
Even when this does not break API literally it might have affect on API consumers when we reduce it to zero. |
Yes, element information is often expensive to compute and computing it should be avoided in most cases, and only performed when we know the information is useful. The delay is a way to know that the information is probably useful as user has been stuck on the given proposal for some time. |
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
Yes we don't do it in the UI thread. So the UI is not blocked. But there might be other consequences: Once the first request is sent via the network the computation is running on the server and cannot be stopped any more - we only can discard the response. I will try it out what happens in out product once I change the value to zero and will let you know. |
I just tested setting OpenStrategy#TIME so zero and triggering code completion in JDT. After that change the element information control does not come up any more. The reason is that in Line 164 in e630919
So maybe I misunderstood what you wanted to change. @jukzi Can you please explain? |
Either
That should be fixed anyway. Math.max(0,...), or using Throttler there, which guarantees at least one execution. May be even rethink why there is a substraction of 50. So in principle a first experiment to set TIME to 0 can correctly identify such Odds. thanks for testing. |
I played around a bit with JDTs code element info. This worked on my computer
When changing 10 so something shorter did not work reliably. 0 didn't work at all. And shorten then 10 did work most times. But some times I did not get a code element information control when scrolling through the proposals. So we seem to have time-ing issues somewhere. @jukzi Maybe you can investigate further on that and provide a draft PR. If can help with testing then. |
The current implementation of the AdditionalInfoController is like:
i.e. the "delay" acts like a "timeout". So all the claims that the "delay" is there to prevent computations are wrong for the AdditionalInfoController. Moving the allowShowing() from SECOND_WAIT to FIRST_WAIT totally disables that delay/timeout and gives instant proposals without changing any delay - just allow it to be shown as soon as computed (may also disable the timeout):
|
Thanks for finding out. I didn't "claim" something. I was just thinking about possible side effects.
One thing to improve:
does call triggerShowing twice. Once in allowShowing and once in setInfo.
does fix this. I tested this with the JDT completion and the element info. This works find on my machine. I also made some (poor man's System.out.println measurements. My use case was:
and I triggered the code completion at: Current implementation (with the delay):
With the changes mentioned above:
So we see that the time it takes after calculation of the element info until it's show reduced from approx 500 ms to approx 0,08 ms. Which is expected I would say. |
I thought more about it. When we remove the delay we don't do more calculations because already today we calculate the code element information and then delay the display of if. Displaying is now done more often. This is done with Display.asyncExec and displaying the control and rendering it on the UI takes some time in the UI thread and blocks this a bit more now. So if we now press and holt the cursor down key in the list of completions it might be that we can see that the selection now moves slower (not not so smooth any more) because of the UI thread constantly updating the control. Here are the two screencasts. withoutChange.movwithChange.movSo I think we should go on with the removal of the delay here. Any other opinions? Maybe somebody with slower hardware can do the same "speed test". |
+1 for the removal early in next M1 so that we have a full release to find issues. For this release it is a bit to late IMHO |
…tform#1695 Removes 500ms lag for javadoc information in the completion dialog. Sets related delays to 0 and fix code to handle such short delays. eclipse-platform#1695
…tform#1695 Removes 500ms lag for javadoc information in the completion dialog. Sets related delays to 0 and fix code to handle such short delays. eclipse-platform#1695
The discussion on #1726 gives the impression that the value of |
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
…tform#1695 Removes 500ms lag for javadoc information in the completion dialog. Sets related delays to 0 and fix code to handle such short delays. eclipse-platform#1695
…rm#1695 Avoids a 500ms delay after changing the selection, like higlighting the selected word. eclipse-platform#1695
Avoids a 500ms delay after changing the selection, like higlighting the selected word. #1695
…tform#1695 Removes 500ms lag for javadoc information in the completion dialog. Sets related delays to 0 and fix code to handle such short delays. eclipse-platform#1695
currently org.eclipse.jface.util.OpenStrategy.TIME is set to 500ms which has the following effect:
After selecting something (mouse or keeyboard) in the editor there will be a 500ms delay before the UI is update.. for example
example:
without delay all those features are almost instant:
I don't know of any benefit of the delay. Anybody does? I thought it might help to avoid too much events when let the cursor be pressed for scrolling. But those events are ignored anyway until key is released. Only if i use mouse to multiselect multiple lines there are many events, but i don't experience any lag due to it. It looks much more responsive when the outline view is updated instantaniously.
how i stumpled across that delay (i always thought the UI is just so slow that it need so much time to update): There currently is an error when you select a type and press ctrl+t within <500ms. The type hierarchy will use the statusline for progressreport but after 500ms the text is overridden by Java Editors Problem hint. (until now i never noticed the problem hint in the status bar, probably because it has that 500ms lag)
The text was updated successfully, but these errors were encountered: