Community
Participate
Working Groups
The methods to retrive selection in TextViewer should not require UI Thread, as it's easy to plug listener computing the value for those methods, and returning the pre-computed result on demand. The effort of computing the values seems small enough to happen on selection change. Having them non-UI Thread would make some content-assist processor able to work without UI Thread.
I doubt this can be done, but let's see.
(In reply to Dani Megert from comment #1) > I doubt this can be done, but let's see. Challenge accepted ;)
New Gerrit change created: https://git.eclipse.org/r/142866
(In reply to Eclipse Genie from comment #3) > New Gerrit change created: https://git.eclipse.org/r/142866 This is work-in-progress, one issue at the moment is that some events that trigger a selection change don't send a notification so widget is not updated.
I think there is no good way to implement it without risk on backward compatibility: there is no guarantee that the listeners used by this patch to store selection happen before other listeners that might invoke the getSelectedRange() or getSelection() methods, which would return different result before the internal listener is called. Also, those methods are API and the contract doesn't say it doesn't require UI Thread. So changing them in one implementation and assume those methods are in the general case good for usage in a non-UI Thread would be faulty. As mentioned by Andrey in a previous comment IIRC, introducing a new method `getLastSelection()` on the interface, which we specify in the javadoc as not requiring UI Thread, seems to be a better way forward. We'd add a dummy default implementation, and a better working implementation in TextViewer. JDT could rely on this method to remove its adherence on UI Thread in completion computers. What do you think?
(In reply to Mickael Istria from comment #5) > I think there is no good way to implement it without risk on backward > compatibility: there is no guarantee that the listeners used by this patch > to store selection happen before other listeners that might invoke the > getSelectedRange() or getSelection() methods, which would return different > result before the internal listener is called. Also, those methods are API > and the contract doesn't say it doesn't require UI Thread. So changing them > in one implementation and assume those methods are in the general case good > for usage in a non-UI Thread would be faulty. > As mentioned by Andrey in a previous comment IIRC, introducing a new method > `getLastSelection()` on the interface, which we specify in the javadoc as > not requiring UI Thread, seems to be a better way forward. We'd add a dummy > default implementation, and a better working implementation in TextViewer. > JDT could rely on this method to remove its adherence on UI Thread in > completion computers. > > What do you think? I don't think this could solve the "listener problem".
Current version of the patch is ready for review (not an emergency).
Making it an API candidate for M1. This API addition would be profitable for 1. Users for ContentAssistant in asynchronous mode (relying on generic editor or setting the flag to true), which want to provide content-assist proposals that depend not only on the offset, but also on the current selection (for example a completion with a whole word selected might be driving to computation of possible replacement instead of possible inserts), but that are not allowed to call methods requiring UI Thread. 2. in JDT (bug 538858) to replace usage of methods requiring API by this method so JDT proposal computation could happen in a non-UI Thread, without blocking UI.
(In reply to Mickael Istria from comment #8) > Making it an API candidate for M1. > This API addition would be profitable for > 1. Users for ContentAssistant in asynchronous mode (relying on generic > editor or setting the flag to true), which want to provide content-assist > proposals that depend not only on the offset, but also on the current > selection (for example a completion with a whole word selected might be > driving to computation of possible replacement instead of possible inserts), > but that are not allowed to call methods requiring UI Thread. > 2. in JDT (bug 538858) to replace usage of methods requiring API by this > method so JDT proposal computation could happen in a non-UI Thread, without > blocking UI. The reason why this might require review is that it creates a API breakage: in https://wiki.eclipse.org/Evolving_Java-based_APIs_2 """ (8) Adding a default method will break an existing client type if it already implements another interface that declares a default method with a matching signature, and the client type already refers to the default method from the other interface (except when using the Interface.super.method() notation). The added default method will cause an IncompatibleClassChangeError at run time, see JLS8 13.5.6. Furthermore, re-compiling the type will result in a compile error. In cases where the risk of multiple inheritance of the same method from multiple interfaces is deemed low and the added value is deemed sufficiently high, selected default methods can be added to existing interfaces. However, such a change should only be applied in an early milestone, and it should be reverted if clients report any difficulties with the change. """ With the current name (getLastKnownSelection), I couldn't find any hit on GitHub and searching 2 search engines (Qwant and Google) didn't find any hit neither. So it seems to me that we're in the case where "the risk of multiple inheritance of the same method from multiple interfaces is deemed low", and the goal of this API method is IMO "deemed sufficiently high".
This will most likely miss M1 as I'd like to consolidate this proposal with examples of how it can enable JDT to become non-blocking.
In reply to Mickael Istria from comment #8) > Making it an API candidate for M1. (continued) On the patch https://git.eclipse.org/r/142866 , I've added a test example showing the kind of things that this new API would enable in the Generic Editor (where completion runs in non-UI Thread): this methods allows to make some completion processor easily use the TextSelection from the viewer to generate their proposal. It's something that is not properly possible without the suggested change or a similar one: content assist processor cannot get the text selection. In https://git.eclipse.org/r/142866 / Bug 538656, I've shown how this can be used in JDT to make it's current contentassistprocessors not requiring UI Thread. If you couple it with change https://git.eclipse.org/r/129022 / Bug 531061 then this enables JDT content assist to not block UI when computing proposals (similarly to generic editor, it would show the completion popup with a "computing" proposal while computing and then will show the list of proposals). This ability to make JDT completion non-blocking also depends on this ability to get the selection from out of the UI Thread.
-1 for adding the default method, especially since the default implementation is wrong in most cases. Clients using the new method can never be sure whether they get a correct result. The correct way is to use the pattern we always use when adding new methods to an interface: add an extension interface ==> ITextViewerExtension9 (and let TextViewer implement it). That way, clients can check whether the viewer implements that interface and a correct implementation of the method is available.
(In reply to Dani Megert from comment #12) > -1 for adding the default method, especially since the default > implementation is wrong in most cases. Clients using the new method can > never be sure whether they get a correct result. > > The correct way is to use the pattern we always use when adding new methods > to an interface: add an extension interface ==> ITextViewerExtension9 (and > let TextViewer implement it). > > That way, clients can check whether the viewer implements that interface and > a correct implementation of the method is available. Thanks Dani, that's indeed better. I'll do that as soon as I have the opportunity (ETA end of this week)
(In reply to Mickael Istria - away until July 15th from comment #13) > Thanks Dani, that's indeed better. I'll do that as soon as I have the > opportunity (ETA end of this week) latest patch set in https://git.eclipse.org/r/c/142866/ implements it with a ITextViewerExtension9. Would you like to give it a review?
(In reply to Mickael Istria - away until July 15th from comment #14) > You're still away ;-) > Would you like to give it a review? Yes, but it might take a while as I am in India with my team for the next 8 days.
(In reply to Dani Megert from comment #15) > > Would you like to give it a review? > Yes, but it might take a while as I am in India with my team for the next 8 > days. Ok, that's fine.
@Dani: do you still want to have a look at the proposal? It'd be great if we could merge it for M3.
Since this is to late to roll out this change in Platform Text and then in JDT without risk, let's postpone it for 4.14.M1. People are still encouraged the review the patch from now on; unless there are negative reviews, I'll merge it on the day 4.14 opens so we can adopt it in JDT ASAP with enough time for feedback.
(In reply to Mickael Istria from comment #17) > @Dani: do you still want to have a look at the proposal? It'd be great if we > could merge it for M3. Yes, it's in my big inbox. Sorry for the delay. 4.14 is a good plan.
(In reply to Dani Megert from comment #19) > Yes, it's in my big inbox. Sorry for the delay. 4.14 is a good plan. Is it OK for a merge today or tomorrow?
New Gerrit change created: https://git.eclipse.org/r/149497
Gerrit change https://git.eclipse.org/r/142866 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=b706c681d2e1dedf9cff05eb1a366db762abc8be
(In reply to Eclipse Genie from comment #22) > Gerrit change https://git.eclipse.org/r/142866 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/ > ?id=b706c681d2e1dedf9cff05eb1a366db762abc8be This breaks debugger, see bug 551180. Debug Shell is unusable for me, and so I can't use SDK for development anymore. We urgently need to fix this or revert the change (note: bundle versions were updated, so if reverting, we must revert only code).
(In reply to Andrey Loskutov from comment #23) > This breaks debugger, see bug 551180. Thanks for catching that, I'll have a look but cannot promise immediate solution so... > Debug Shell is unusable for me, and so I can't use SDK for development > anymore. We urgently need to fix this or revert the change (note: bundle > versions were updated, so if reverting, we must revert only code). ... I think reverting is worth it here. Feel free to revert and I'll try to cover the case of bug 551180 in some new version of the patch.
(In reply to Mickael Istria from comment #24) > (In reply to Andrey Loskutov from comment #23) > > This breaks debugger, see bug 551180. > > Thanks for catching that, I'll have a look but cannot promise immediate > solution so... > > > Debug Shell is unusable for me, and so I can't use SDK for development > > anymore. We urgently need to fix this or revert the change (note: bundle > > versions were updated, so if reverting, we must revert only code). > > ... I think reverting is worth it here. Feel free to revert and I'll try to > cover the case of bug 551180 in some new version of the patch. Mickael, I've merged now this patch: https://git.eclipse.org/r/#/c/149729/ to fix Debug Shell bug 551180. Please validate if and how this affects this implementation.
(In reply to Andrey Loskutov from comment #25) > Mickael, I've merged now this patch: > https://git.eclipse.org/r/#/c/149729/ > to fix Debug Shell bug 551180. > Please validate if and how this affects this implementation. Thanks. I'll check that when getting back to bug 528656.
Mickael, what is the status here? Can it be resolved?
Yes, let's mark is a resolved. API is here and working (in the tests I've made with downstream JDT bugs). If some more issue happen with it, we'll open new tickets.
