remove 500ms PostSelectionDelay

[jukzi]

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

• the link in the Outline view,
• Status Bar entry for warnings,
• highlighting the selected word,
• ...? 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)

[mickaelistria]

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.

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. Does the "blame" tell more about when/why this delay was introduce?

But those events are ignored anyway until key is released

Is this true on all OSs?

[jukzi]

Does the "blame" tell more about when/why this delay was introduce?

no. its from initial.

Is this true on all OSs?

i tested on win only.

[vogella]

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).

[jukzi]

I thinks we should try 0ms on M1 and see if there is any drawback

[mickaelistria]

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.

[laeubi]

I thinks we should try 0ms on M1 and see if there is any drawback

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.

[mickaelistria]

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.

[jukzi]

My original problem with type hierarchy was any value >0 cause concurrency issues.

This is currently also used for content assistant

If you really experience any issue please post a video about it. i have not see any defect yet.

[laeubi]

My original problem with type hierarchy was any value >0 cause concurrency issues.

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....

[jukzi]

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.

[mickaelistria]

If we find out why there is any benefit to have any delay that would be needed

We've already found out that the delay is here to prevent useless computation from happening too early in case of repeated fast interactions.

[jukzi]

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?

[mickaelistria]

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.

[laeubi]

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.

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 ...

[jukzi]

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.

[laeubi]

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.

[jukzi]

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

[Phillipus]

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.

[jukzi]

normaly such updates should be throttled by Throttler see https://github.com/eclipse-platform/eclipse.platform.ui/commit/56fd3f80f14125f357779a3e96c2c8eaa9facf93#diff-e2ef7af122667f19ba26e15fc1cda3712bdaf6d8efbd72123b944772b067d7d7R144 please check if just keeping the throttletime to 500ms solves your issue.

[laeubi]

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

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...

[jukzi]

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.

[mickaelistria]

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.

[merks]

When I read Probably it only has to be used at more places. I really wonder if that might actually very likely or definitely and I wonder whether there will be a proactive effort to locate those likely places and we simply try it and see if anything goes horrible wrong? (I agree that using that approach is very like much better than some arbitrary and rather large delay.)

[vogella]

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).

[jukzi]

the delay as used in org.eclipse.jface.text.contentassist.AdditionalInfoController controlles that the completion proposal YELLOW page pops up 500ms later ( i always thought its so expensive to calculate). Is there any benefit from a delay there? I suggest to not even throttle it (unlikely someone uses completion several times a second) but only drastically reduce the delay there. Example delay: WDYT?

[BeckerWdf]

YELLOW page pops

You mean the code element information?

[jukzi]

You mean the code element information?

the right pop up with additional information, containing the javaDoc in this example.

[BeckerWdf]

You mean the code element information?

the right pop up with additional information, containing the javaDoc in this example.

ok. Only on windows this is yellow ;-)

[laeubi]

normaly such updates should be throttled by Throttler see

Using Throttler to handle reaction to delayed events so that intermediary events gets ignored is really welcome and fits really well the requirement here

I wonder whether there will be a proactive effort to locate those likely places and we simply try it and see if anything goes horrible wrong?

I definitely like the approach to react immediately if possible and delay if multiple event happen. IIRC Throttler was implemented to speed up Jobs

I suggest to not even throttle it (unlikely someone uses completion several times a second) but only drastically reduce the delay there.

Well then it is simple, just remove all references to org.eclipse.jface.util.OpenStrategy.getPostSelectionDelay() except in the OpenStrategy deprecate its usage and then it is easy to fix / adjust / remove / the value as it will only affect the internal implementation, but as long as it is public API we can't just play around with its value and reducing it to zero without having evaluated effect on all consumers just to fix one component.

[laeubi]

i always thought its so expensive to calculate

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.

[BeckerWdf]

i always thought its so expensive to calculate

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.

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.

[BeckerWdf]

but as long as it is public API we can't just play around with its value and reducing it to zero without having evaluated effect on all consumers just to fix one component.

Even when this does not break API literally it might have affect on API consumers when we reduce it to zero.

[mickaelistria]

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.

[jukzi]

do calls over the network to get completion proposals

I bet you are not doing them in the UI Thread, are you? such proposals should be asynchronous anyway. And it does not help the user that slow thinks start 500ms later, taking even longer to complete, while CPU and developer idle together.

With a delay and actually using the proposal selection (scrolling with keys or mouse) the advanced information is currently wrong for 500ms, because it keeps showing the last one:

Another usecase where the delay is very counterproductive: "Show Revision Information", hover over the annotations and drink a coffee for each 500ms. I always hover them for the purpose of the advanced info only.

[BeckerWdf]

I bet you are not doing them in the UI Thread, are you?

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. So by calculating proposals / code element info immediately a user scrolling down 5 times until the correct entry is selected might trigger 4 unnecessary computations on the server putting unnecessary load on the server.

I will try it out what happens in out product once I change the value to zero and will let you know.

[BeckerWdf]

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 https://github.com/eclipse-platform/eclipse.platform.ui/blob/e6309193420f45895654d404d9f2adf875ca9432/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java#L164 from the delay (which is now 0) we substract 50. And this negative number is then added to the current time (which gives us a time in the past) and this is then used as the time at which the control should be shown. As this is in the past the control does not come up.

So maybe I misunderstood what you wanted to change. @jukzi Can you please explain?

[jukzi]

Either

• setting TIME to 0 or
• not using it or
• using Throttler with a positive value.

And this negative number

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.

[BeckerWdf]

That should be fixed anyway. Math.max(0,...)...

I played around a bit with JDTs code element info. This worked on my computer

@Override
public long delay() {
    return Math.max(10, fDelay - DELAY_UNTIL_JOB_IS_SCHEDULED);
}

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.

[jukzi]

The current implementation of the AdditionalInfoController is like:

1. start computation of proposal
2. wait the delay
3. if the computation already completed => show it
4. otherwise never show the computated result.

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):

diff --git a/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java b/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java
index 3333639..ee3b502 100644
--- a/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java
+++ b/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java
@@ -121,6 +121,7 @@
                             */
                            return new Status(IStatus.WARNING, "org.eclipse.jface.text", IStatus.OK, "", x); //$NON-NLS-1$ //$NON-NLS-2$
                        }
+                       allowShowing();
                        setInfo((ICompletionProposal) proposal, info);
                        return Status.OK_STATUS;
                    }
@@ -151,7 +152,7 @@
            @Override
            public void run() {
                // show the info
-               allowShowing();
+               //allowShowing();
            }

            @Override

[BeckerWdf]

The current implementation of the AdditionalInfoController is like:

1. start computation of proposal
2. wait the delay
3. if the computation already completed => show it
4. otherwise never show the computated result.

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.

Thanks for finding out. I didn't "claim" something. I was just thinking about possible side effects.

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):

One thing to improve:

allowShowing();
setInfo((ICompletionProposal) proposal, info);

does call triggerShowing twice. Once in allowShowing and once in setInfo.

fAllowShowing= true;
setInfo((ICompletionProposal) proposal, info);

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:

package test;
public class test {
    public static void main(String[] args) {
        // TODO Auto-generated method stub
    }
}

and I triggered the code completion at: main(String<triggerHere>[] args) and moved the selection down and up with the curser

Current implementation (with the delay):

Calculation took  128 705 666 ns
Trigger showing   323 482 750 ns after calculation ended
--------------------------------------------------------
Calculation took   29 581 542 ns
Trigger showing   425 187 000 ns after calculation ended
--------------------------------------------------------
Calculation took   21 829 583 ns
Trigger showing   432 868 667 ns after calculation ended
--------------------------------------------------------
Calculation took   18 049 375 ns
Trigger showing   436 794 084 ns after calculation ended
--------------------------------------------------------
Calculation took      865 917 ns
Trigger showing   454 106 291 ns after calculation ended
--------------------------------------------------------
Calculation took    1 519 583 ns
Trigger showing   448 844 500 ns after calculation ended
--------------------------------------------------------
Calculation took      880 750 ns
Trigger showing   450 355 750 ns after calculation ended
--------------------------------------------------------

With the changes mentioned above:

Calculation took  1 333 542 ns
Trigger showing     173 291 ns after calculation ended
--------------------------------------------------------
Calculation took  7 792 541 ns
Trigger showing      77 084 ns after calculation ended
--------------------------------------------------------
Calculation took  3 836 083 ns
Trigger showing      76 542 ns after calculation ended
--------------------------------------------------------
Calculation took  5 148 750 ns
Trigger showing      80 625 ns after calculation ended
--------------------------------------------------------
Calculation took  5 145 500 ns
Trigger showing      84 708 ns after calculation ended
--------------------------------------------------------
Calculation took  5 048 292 ns
Trigger showing      71 541 ns after calculation ended
--------------------------------------------------------

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.

[BeckerWdf]

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. With the hardware from 20 year ago this might have been really an issue - with todays hardware I don't think it is any more. I made two screencast and if your run them next to each other I cannot see a difference. So at least on my pretty fast M1 MacBook this isn't an issue.

Here are the two screencasts.

https://github.com/eclipse-platform/eclipse.platform.ui/assets/28338612/5f1f5bdf-3698-4347-85cd-205fe169c768

https://github.com/eclipse-platform/eclipse.platform.ui/assets/28338612/9d1f6b3c-ef37-41e6-b72d-fe5e96168f80

So 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".

[vogella]

+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

[mickaelistria]

The discussion on #1726 gives the impression that the value of OpenStrategy.getPostSelectionDelay() is too long: it seems like this value is meant to cover the case of unnecessary reaction when user hits some arrow key multiple time (and that's good IMO). However, the 500ms value is noticeably long because it makes things feel like a lag, while the frequency of hitting the down keys to navigate a list is probably around 5Hz/200ms between hits (some reference litterature about it would be good to consolidate that assumption). So we should probably reduce the value to ~250ms to improve reactivity without causing too much extra CPU work when navigating a list with arrow keys quickly.