Additional Completion Information: remove delay & timeout #1695 #1726

jukzi · 2024-02-29T14:55:13Z

Removes 500ms lag for javadoc information in the completion dialog. Sets related delays to 0 and fix code to handle such short delays.

#1695

github-actions · 2024-02-29T15:56:58Z

Test Results

918 files + 1 918 suites +1 48m 48s ⏱️ + 9m 48s
7 452 tests ± 0 7 301 ✅ + 2 151 💤 ± 0 0 ❌ - 2
23 505 runs +1 573 23 000 ✅ +1 456 505 💤 +119 0 ❌ - 2

Results for commit 576f0f5. ± Comparison against base commit adc4b6b.

♻️ This comment has been updated with latest results.

BeckerWdf

I manually tested this change with JDT's element info.
Also with the element info in our ABAP Development Tools.
This works as expected. Element Info in code completion is not much more snappy.

What about the Implementation in AdditionalInfoController2? Will you adapt it as well?

BeckerWdf · 2024-03-01T09:05:41Z

Let's merge this early once dev for 4.32 opens.

BeckerWdf · 2024-03-01T09:06:14Z

Pls. also have a look at the failing tests.

jukzi · 2024-03-01T09:16:37Z

java.lang.AssertionError: No new shell found expected:<1> but was:<2>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.eclipse.ui.genericeditor.tests.CompletionTest.findNewShell(CompletionTest.java:220)
	at org.eclipse.ui.genericeditor.tests.CompletionTest.testMoveCaretBackUsesAllProcessors_bug522255(CompletionTest.java:251)

Wrong job order: arrays first differed at element [0]; expected:<1. User Job(3523)> but was:<4. Usual job 2(3526)>
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:78)
	at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:28)
	at org.junit.Assert.internalArrayEquals(Assert.java:534)
	at org.junit.Assert.assertArrayEquals(Assert.java:285)
	at org.eclipse.ui.tests.progress.ProgressViewTests.testItemOrder(ProgressViewTests.java:192)

mickaelistria

In most cases, computing extra information for a completion item is expensive (and it's actually expensive JDT and Javadoc rendering, and expensive for LSP-based editors), and it often creates no value as someone who would hit repeatedly the down key in the completion list would not need the intermediary extra documentations and only wants the one for the target. Extra information is useless in many cases, so I don't think constantly computing extra useless information is a feature here, I see it more as a way to overload CPU with uninteresting operations.
IMO, a delay is necessary for best usability and performance; removing it would be a regression. A better value for the delay could maybe be found though.

BeckerWdf · 2024-03-01T09:59:56Z

In most cases, computing extra information for a completion item is expensive (and it's actually expensive JDT and Javadoc rendering, and expensive for LSP-based editors), and it often creates no value as someone who would hit repeatedly the down key in the completion list would not need the intermediary extra documentations and only wants the one for the target. Extra information is useless in many cases, so I don't think constantly computing extra useless information is a feature here, I see it more as a way to overload CPU with uninteresting operations. IMO, a delay is necessary for best usability and performance; removing it would be a regression. A better value for the delay could maybe be found though.

There are two delays 50 ms befor starting the computation. and 400 ms after the computation before it's displayed.
At least the 400ms don't help with the issues you talk about (as the calculation is done anyways).

jukzi · 2024-03-01T10:02:18Z

In most cases, computing extra information for a completion item is expensive

Please see the analysis in linked issue: the "timeout" did not prevent computing that information anyway. Please also note that this implementation was tested with both javadoc and even ABAPs network stuff and did not show a problem. If you find any performance issues please report them with a way how to reproduce.

mickaelistria · 2024-03-01T10:03:07Z

Both computating the data (first delay) and rendering it (second delay) can be expensive. But I don't think it makes a lot of sense to separate those. A single delay to rule them all would be enough, and there is indeed no value in computing the data if it's not to render it ASAP.

mickaelistria · 2024-03-01T10:05:31Z

If you find any performance issues please report them with a way how to reproduce.

Can you please check the overall CPU load of browsing a list of proposals? ie if I'm hitting the down arrow key 5 times to get the 6th completion item, I believe this change will cause 5 times more computations than before, for no obvious value.
It may not be perceivable by users, but more CPU for not clear additional value is a performance regresion.

jukzi · 2024-03-01T10:06:52Z

ie if I'm hitting the down arrow key 5 times to get the 6th completion item

sorry but i am unable to press down key 5 times within 50ms

laeubi · 2024-03-01T10:26:29Z

ie if I'm hitting the down arrow key 5 times to get the 6th completion item

sorry but i am unable to press down key 5 times within 50ms

This will probably help you:

https://www.youtube.com/watch?v=GvxjnW7cx20

Anyways the second delay is as mentioned before to prevent flickering (e.g. when scrolling through items by holding the key down(!)), or by moving the mouse around and to adhere to the principle that only if I'm "long enough" over an item I get additional information, e.g. for Tooltips see this one:

https://uxpickle.com/delay-before-the-tooltip-shows-up/

jukzi · 2024-03-01T10:31:28Z

ProgressViewTests

random failing see #195

CompletionTest

fixed the test

jukzi · 2024-03-01T10:33:17Z

Anyways the second delay is as mentioned before to prevent flickering

There is no flickering, the additional shell stays open anyway. please stick to real issues that are reproducible.

laeubi · 2024-03-01T10:48:13Z

Anyways the second delay is as mentioned before to prevent flickering

There is no flickering, the additional shell stays open anyway. please stick to real issues that are reproducible.

But only if the "real" content is always the same for all elements what would make such information quite useless.

Just see this example where the size and contents "jumps around" as one moves the mouse (still with delay so I need to hold over this):

Peek.2024-03-01.11-44.mp4

now assume that on every move over the document the popup is show immediately that will flicker around all the way an will at least drive me crazy, and that's the purpose of these delays, so even if the information is there it is only show when the user actually stops for a while.

jukzi · 2024-03-01T11:01:35Z

now assume that on every move over the document the popup

That popup is not a completion proposal. It is totally unrelated to the completion popup target by this PR.

laeubi · 2024-03-01T11:05:13Z

now assume that on every move over the document the popup

That popup is not a completion proposal. It is totally unrelated to the completion popup target by this PR.

It shows the problem and the background for delays, just open a completion proposal with many proposals, do completion, wait for the popup to show up and then hold down the down key.

With delay: It shows the last item unless you release the key
Without delay: the text changes (depending on how fast it could be computed) causing "flickering" as it is constantly redrawn.

jukzi · 2024-03-01T11:11:17Z

Without delay: the text changes (depending on how fast it could be computed) causing "flickering" as it is constantly redrawn.

Mind to share a video of such flickering and how to reproduce? To me it looks just smooth.

laeubi · 2024-03-01T11:16:37Z

Without delay: the text changes (depending on how fast it could be computed) causing "flickering" as it is constantly redrawn.

Mind to share a video of such flickering and how to reproduce? To me it looks just smooth.

If you like watching text-movies it looks smooth, but any animation any change or alike will catch up the eye so one should just avoid that, it is already moving the highlight of the selection, any other thing is just distracting: https://uxdesign.cc/the-ultimate-guide-to-proper-use-of-animation-in-ux-10bd98614fa9

BeckerWdf · 2024-03-01T12:11:09Z

Fix CompletionTest to find the completionShell

Jörgs Change does not affect this. It's about the element information in the completion popup. Pls. really do test the correct stuff.

BeckerWdf · 2024-03-01T12:13:08Z

If you like watching text-movies it looks smooth, but any animation any change or alike will catch up the eye so one should just avoid that, it is already moving the highlight of the selection, any other thing is just distracting: https://uxdesign.cc/the-ultimate-guide-to-proper-use-of-animation-in-ux-10bd98614fa9

This article speaks about animations. I don't see how this is related to this PR.

BeckerWdf · 2024-03-01T12:16:23Z

and even ABAPs network stuff and did not show a problem.

to be honest there is not problem with out "network stuff" because we anyway have a throttling in pace that send's requests to the server ever 400 ms at max. If the last request was triggered not longer then 400 ms in the past we don't trigger a new one. We have this in place since ages. Sending a request to the server on every selection change immediately wouldn't be a good idea.

laeubi · 2024-03-01T12:17:19Z

Yes for sure everything is always unrelated so why asking ;-)

It was asked why there are two delays, I explained why in UX design one has such delays --> unrelated

It was asked why there is flickering if it is "smooth" (aka animation) , I gave a reference how animations are correctly used --> unrelated

Jörg itself fixed a test about completion that fails now he change things --> unrelated

BeckerWdf · 2024-03-01T12:22:30Z

and rendering it (second delay) can be expensive

Pls. provide proof for this. I did test this. Showing the anyway computed information immediately did not cause any issue on my computer. I did not see that the selection did move slower and less smooth when simply holing the down key down.

Any pls. guys: Holding down the key for seconds is not a real-word use case - it's synthetic test case. In real word users might press down the cursor keys maybe 5-6 times repeatedly fast after each other. If the needed proposal is not found they might simply continue typing in the editor to narrow down the list of completions.

Maybe we can agree on keeping the 50ms that throttles the computation - but I don't see any value (in todays reality with fast computers) in waiting half a second before rendering the information.

BeckerWdf · 2024-03-01T12:28:41Z

Yes for sure everything is always unrelated so why asking ;-)

It was asked why there are two delays, I explained why in UX design one has such delays --> unrelated

It was asked why there is flickering if it is "smooth" (aka animation) , I gave a reference how animations are correctly used --> unrelated

Jörg itself fixed a test about completion that fails now he change things --> unrelated

@laeubi:
Your communication behavior is not constructive and goal-oriented. It does not comply with many points of the Code of Conduct of the Eclipse Foundation (https://www.eclipse.org/org/documents/Community_Code_of_Conduct.php). For example :

Using welcoming and inclusive language
Actively encouraging all voices
Helping others bring their perspectives and listening actively.
Being respectful of differing viewpoints and experiences
Gracefully accepting constructive criticism

Please let's try to come back to a constructive discussion.

BeckerWdf · 2024-03-01T12:33:33Z

and fix code to handle such short delays.

This really concerns me a bit. Might there be code outside of platform / JDT / ... that might break due to this change? @jukzi what's your feeling on this?

laeubi · 2024-03-01T12:37:29Z

Please let's try to come back to constructive discusion.

You mean starting each sentence where you want to actually force someone by "please"?
Or to simply qualify every unwanted thing as "totally unrelated" or even "not a real-word" ... but yes for sure its all me being not so friendly and welcoming any change.

It was repeatably discussed and accepted that a lower delay is suitable, but every time is is insisted that it must be zero (because it is meant to actually fix a different problem) and every one that don't agree is just an non real world idiot. So these things tend to always running in circles.

Also please note that I didn't blocked this PR in any way I just gave some hints why "delay = zero" is probably a bad choice from UX POV.

So maybe it would help if one not only demands things from other but actively following the mentions Code of Conduct and to be a role model in this regards.

jukzi · 2024-03-01T12:54:15Z

This really concerns me a bit. Might there be code outside of platform / JDT / ... that might break due to this change? @jukzi what's your feeling on this?

The comment was only because the Additional Completion internally had a problem with substracting those values, making wrong assumptions. Also all Completiontests which assert a delay may of cause break, but just of a needless assumption.
If any problem remains its likely to be fixable. Fixing the code could even stay when reverting the values.

BeckerWdf · 2024-03-01T13:03:53Z

This really concerns me a bit. Might there be code outside of platform / JDT / ... that might break due to this change? @jukzi what's your feeling on this?

The comment was only because the Additional Completion internally had a problem with substracting those values, making wrong assumptions. Also all Completiontests which assert a delay may of cause break, but just of a needless assumption. If any problem remains its likely to be fixable. Fixing the code could even stay when reverting the values.

But could you also imagine implementations out in the wild that might break because platform now is faster?

BeckerWdf · 2024-03-01T13:06:33Z

but every time is is insisted that it must be zero and every one that don't agree is just an non real world idiot

This is not true. I did not insist that every delay must be zero. See:

Maybe we can agree on keeping the 50ms that throttles the computation - but I don't see any value (in todays reality with fast computers) in waiting half a second before rendering the information.

and I also didn't us the word idiot.

jukzi · 2024-03-01T13:06:38Z

because it is meant to actually fix a different problem

The Statusline problem was only the initial trigger to stumble across this delay. This change is totally unrelated to that - which will be fixed by #1706.

is just an non real world idiot.
...
following the mentions Code of Conduct

That's a strange combination of sentences.

jukzi · 2024-03-01T13:11:32Z

But could you also imagine implementations out in the wild that might break because platform now is faster?

As any change can break adopters (https://www.hyrumslaw.com/) -of cause. We could also provide system properties for the actual timings to allow a revert without touching the binary.

mickaelistria · 2024-03-01T14:37:23Z

In real word users might press down the cursor keys maybe 5-6 times repeatedly fast after each other.

Yes, this is indeed the case to focus on here. And I don't think this proposal here which is about computing and rendering the information for the 5 hits is a better solution for than reducing the delay to a typical duraction of repeated hits (which I imagine is around ~300ms but is probably documented in some UX recommendation somewhere), which is probably what the OpenStrategy.getPostSelectionDelay() is already trying to capture.
Note that I think removing the hardcoded 50ms delay here makes sense, but I think the OpenStrategy.getPostSelectionDelay() should still be obeyed (even if we change it to some other value later as part of #1695 ).

...rg.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/AdditionalInfoController.java

mickaelistria · 2024-03-01T14:39:54Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/ContentAssistant.java

-		if (fInformationControlCreator != null)
-			controller= new AdditionalInfoController(fInformationControlCreator, OpenStrategy.getPostSelectionDelay());
+		if (fInformationControlCreator != null) {
+			/** delay until each additional information is shown **/


I think it's better to keep te OpenStrategy.getPostSelection() delay here (and probably improve its value in another PR to match the case of repeated hits on arrow keys)

As @laeubi mentioned: in other usecases (javadoc hover) a delay in other usecases may make sense. Therefore i prefer to touch only usecase by usecase. In this usecase the delay seem to not show any positive value.

Therefore i prefer to touch only usecase by usecase.

The thing here is that you're not touching usecase by usecase, but you're touching all use-cases at once. Also, if a delay make sense for some use-cases, it's likely to make sense for some other.

enlighten me; what is any other usecase here then showing the additional completion window? This value was only used to show the wrong (outdated) information for a fixed 0.5 sec and if the computation took too long to never show it. My guess is you still have not tried out what this value does. That was more a bug then a feature.

what is any other usecase here then showing the additional completion window?

Unless we're talking about different thing, any IContentAssistProcessor can return an IContentAssistProposal that returns a getAdditionalProposalInfo (which may be long to compute) and that will show an additional completion window (which may be long to render) in order to show completion details. CDT, LSP4E-based projects such as Wild Web Developer or ShellWax or m2e... and many others are example.

This value was only used to show the wrong (outdated) information for a fixed 0.5 sec

I suspect the value here is used to prevent from extra computation and flickering with new information if users performs other actions in a short time. Retaining an outdated value/popup for some ms is more comfortable than "blinking" popup.
Again, the root cause of the whole discussion is that 0.5s is too much and doesn't fit the reality of keyboard navigation. Just moving this value to ~0.3s (in OpenStrategy) would make a big difference without challenging all the existing performance or UX optimizations.

Just that there is no flickering nor computational saving nor any blinking popup related to this value. The popup open if and only if you press the accelerator and closes when you escape or commit.
That we additional reduce OpenStrategy for other usecase is still an option. Even if rendering takes long it does so after the delay. If you press keys faster then the rendering (which i still doubt or would qualify you for the tetris world record) the rendering is skipped anyway by the existing double check that the proposal is still the current. This state machine already has a build in throttler. just hard to see because it is a bit overcomplicated

jukzi · 2024-03-01T15:32:44Z

What about the Implementation in AdditionalInfoController2? Will you adapt it as well?

I don't know when/how it is used and don't want to touch things blindly. Can you hint to a usecase?

…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

Ignoring the additional Information Shell that may also show up.

mickaelistria

I'm still not convinced that this approach is viable. If the issue is mostly to improve reactivity, I think the right place to achieve that widely is to set a better value to OpenStrategy.getPostSelectionDelay().

The ContentAssistant could be widely improved (eg to use a Throttler, to remove the "data" delay vs the popup delay ...) but it seems to be a wider scope than what's intended in this patch.

mickaelistria · 2024-03-05T16:45:39Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/contentassist/ContentAssistant.java

-		if (fInformationControlCreator != null)
-			controller= new AdditionalInfoController(fInformationControlCreator, OpenStrategy.getPostSelectionDelay());
+		if (fInformationControlCreator != null) {
+			/** delay until each additional information is shown **/


Therefore i prefer to touch only usecase by usecase.

The thing here is that you're not touching usecase by usecase, but you're touching all use-cases at once. Also, if a delay make sense for some use-cases, it's likely to make sense for some other.

jukzi requested a review from BeckerWdf February 29, 2024 14:55

jukzi force-pushed the Additional_Completion_Information branch from 4e1d12c to 385ba29 Compare February 29, 2024 15:19

BeckerWdf approved these changes Mar 1, 2024

View reviewed changes

mickaelistria requested changes Mar 1, 2024

View reviewed changes

mickaelistria mentioned this pull request Mar 1, 2024

remove 500ms PostSelectionDelay #1695

Open

jukzi force-pushed the Additional_Completion_Information branch from 3d63c62 to d754fdf Compare March 5, 2024 12:35

EcljpseB0T added 2 commits March 5, 2024 14:59

Additional Completion Information: remove delay & timeout eclipse-pla…

672a31b

…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

Fix CompletionTest to find the completionShell

39e1bc1

Ignoring the additional Information Shell that may also show up.

jukzi force-pushed the Additional_Completion_Information branch from d754fdf to 39e1bc1 Compare March 5, 2024 13:59

version bump org.eclipse.ui.genericeditor.tests

576f0f5

mickaelistria reviewed Mar 5, 2024

View reviewed changes

Additional Completion Information: remove delay & timeout #1695 #1726

Are you sure you want to change the base?

Additional Completion Information: remove delay & timeout #1695 #1726

Conversation

jukzi commented Feb 29, 2024

github-actions bot commented Feb 29, 2024 • edited

Test Results

BeckerWdf left a comment

Choose a reason for hiding this comment

BeckerWdf commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

jukzi commented Mar 1, 2024

mickaelistria left a comment

Choose a reason for hiding this comment

BeckerWdf commented Mar 1, 2024

jukzi commented Mar 1, 2024

mickaelistria commented Mar 1, 2024

mickaelistria commented Mar 1, 2024

jukzi commented Mar 1, 2024

laeubi commented Mar 1, 2024

jukzi commented Mar 1, 2024

jukzi commented Mar 1, 2024

laeubi commented Mar 1, 2024

jukzi commented Mar 1, 2024

laeubi commented Mar 1, 2024

jukzi commented Mar 1, 2024

laeubi commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

laeubi commented Mar 1, 2024 • edited

BeckerWdf commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024 • edited

BeckerWdf commented Mar 1, 2024

laeubi commented Mar 1, 2024

jukzi commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

BeckerWdf commented Mar 1, 2024

jukzi commented Mar 1, 2024

jukzi commented Mar 1, 2024

mickaelistria commented Mar 1, 2024

mickaelistria Mar 1, 2024

Choose a reason for hiding this comment

jukzi Mar 1, 2024

Choose a reason for hiding this comment

mickaelistria Mar 5, 2024

Choose a reason for hiding this comment

jukzi Mar 5, 2024

Choose a reason for hiding this comment

mickaelistria Mar 5, 2024

Choose a reason for hiding this comment

jukzi Mar 5, 2024

Choose a reason for hiding this comment

jukzi commented Mar 1, 2024

mickaelistria left a comment

Choose a reason for hiding this comment

mickaelistria Mar 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 29, 2024 •

edited

laeubi commented Mar 1, 2024 •

edited

BeckerWdf commented Mar 1, 2024 •

edited