search

eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Show the popup for auto activated only if there are completions to shown #205

Closed rubenporras closed 1 year ago

rubenporras commented 1 year ago

The fix improves the popup so that if it is autoactivated it is only shown if there are completions to be shown. Until know it could happen that the popup would be show with the message "Still computing (0%)".

This happens because there is a race condition between the PopupVisibleTimer and the lambdas executed in computeAndPopulateProposals. The PopupVisibleTimer is started in the async mode (that is, the servers did not respond in less than 50ms) and has a timeout of 500ms. That means there is a range between the 50 and the 500 ms that the server can respond with an empty completion list (in which case the lambdas executed in computeAndPopulateProposals will not make the popup visible). Then after 500ms, the PopupVisibleTimer will make the popup visible, and since all completable futures have been processed, the popup will stay open until the user closes it.

rubenporras commented 1 year ago

@mickaelistria , sorry to ask, but this is new to me. What does it mean that "today is a freeze day"?

Is it possible still to fix bugs at this point in https://wiki.eclipse.org/Category:SimRel-2023-06?

As information if the bug is triggered, since the pop up steals the focus, the user is forced to press ESC, or arrow left/right (or something with the mouse). Arrow up/down does not work because the popup only has one entry. It was happening quite reliable for me while testing our implementation based on LSP4E and it was very annoying.

Regards

mickaelistria commented 1 year ago

Eclipse Platform has "freeze" days when building milestones or releases and during those freeze days, it's not permitted to merge code in. Moreover, Platform is currently in RC phase, so only regression fixes or trivial fixes are admitted. I don't think this patch qualifies as any of those. This will have to wait for the next version (4.29 / 2023-09).

rubenporras commented 1 year ago

Hi @mickaelistria ,

it is a regression introduced in https://github.com/eclipse-platform/eclipse.platform.text/commit/78fc9f6f6566323c691f1fee1eb14de2f27e7d34, where the timer was introduced. I can also simplify the PR to do less cleanup if you like so that it is only changing the run() method, than change would be very local.

Since the last released LSP4E forces an upgrade on the eclipse platform, we cannot avoid this regression.

Regards

mickaelistria commented 1 year ago

We can try to make an exception. Can you please facilitate that by minimizing the diff as much as possible so the change seems more trivial to most? Basically, some lines or conditions were reorder, unless it isn't necessary, please stick to the initial order and "just" add the ().

rubenporras commented 1 year ago

Thanks, I have done that. I am happy to make the change even less intrusive by using the new functions only in the run method, and keep the other method exactly as it is now. Then I would refactor the code to make it nice once the 2023-06 is out.

mickaelistria commented 1 year ago

Just for completeness, can you please describe here some steps to reproduce the issue (ideally with an editor from the Eclipse SDK (I believe editing a .target file with Generic editor is a good candidate), and validate this patch fixes it?

rubenporras commented 1 year ago

Just for completeness, can you please describe here some steps to reproduce the issue (ideally with an editor from the Eclipse SDK (I believe editing a .target file with Generic editor is a good candidate), and validate this patch fixes it?

Regarding this, it is a race condition between the PopupVisibleTimer and the lambdas executed in computeAndPopulateProposals. The PopupVisibleTimer is started in the async mode (that is, the servers did not respond in less than 50ms) and has a timeout of 500ms. That means there is a range between the 50 and the 500 ms that the server can respond with an empty completion list (in which case the lambdas executed in computeAndPopulateProposals will not make the popup visible). Then after 500ms, the PopupVisibleTimer will make the popup visible, and since all completable futures have been processed, the popup will stay open until the user closes it.

I will add this to the description of the PR.

mickaelistria commented 1 year ago

I think the fact that we're not even sure whether it's better to revert part of previous commit or whether it's better to tweak existing code is a sign that the issue is not trivial to resolve. Also, this is not fully a bugfix but a change in the functional behavior (we replace showing feedback of emptiness by no feedback), For those reasons, I think it's a bit risky to do in in RC and believe it's worth delaying it for 4.29.

iloveeclipse commented 1 year ago

I think it's a bit risky to do in in RC and believe it's worth delaying it for 4.29.

Yes. Please don't merge in 4.28.

rubenporras commented 1 year ago

Is the proposal to revert https://github.com/eclipse-platform/eclipse.platform.text/commit/be9ac163f257f88e0f5a64c4ed93bde68207bf34 and then https://github.com/eclipse-platform/eclipse.platform.text/commit/78fc9f6f6566323c691f1fee1eb14de2f27e7d34?

that would be fine for me. Reverting only my last commit, would be a bad idea, it would leave the editor unusable for anyone having a language server which cannot respond in 50 ms. It would be unusable because each time the user stops typing, after 50 ms he would see a popup that never closes and that steals the focus. If at the point where he stopped writing there are no possible completions he will just see a message "Computing proposals" forever, which is even worse. The user did not asked for the feedback, he was just typing.

That is why I think that we need to revert both or fix the problem.

mickaelistria commented 1 year ago

That is why I think that we need to revert both or fix the problem.

I think none is doable safely and properly in RC.

rubenporras commented 1 year ago

@mickaelistria, I think we can do a small fix that should improve the situation quite a lot (see https://github.com/eclipse-platform/eclipse.platform.text/pull/208 which I have created now). It is like a revert of my previous PR and a very small addition to fix the problem.

I understand that we did RC1 now and that changes should be very local (which I try to do), but on the other hand, I think we should fix clear bugs that we discover if they can be fixed locally. Otherwise we accept to deliver something in bad state and I would not understand what is the point of an RC where one cannot fix regressions.

mickaelistria commented 1 year ago

@rubenporras Can you please update this one into a proposal for 4.29 M1 since we merged #208 for 4.28 RC1 ?

rubenporras commented 1 year ago

Yes, I will do that.

rubenporras commented 1 year ago

I have created https://github.com/eclipse-platform/eclipse.platform.text/pull/210