Community
Participate
Working Groups
Most JDT content assist processors part of org.eclipse.jdt.ui actually don't require the UI Thread for their logic. However, they do all rely on the same super class that uses context.getViewer().getSelectedRange() which triggers a UI call (need to run in UI Thread). Some content-assist processors directly reference this chain. We should try to avoid requiring UI Thread for such requests, by storing the selectedRange -and its modifications- in the context and providing an API to get it directly from there instead of going through the viewer. Also, we should put a comment on context.getViewer() to remind extenders that by doing this, they have big risk of requiring UI Thread, leading to impossible parallel computation and guaranteed UI latencies. Then, once we fixed the JDT classes, we should declare them as non-UI Thread compatible (bug 538630)
One major issue I'm facing is that there seems to be no good way to just get the selection from a ITextViewer without requiring the UI Thread. Indeed, getViewer().getSelectedRange() call a checkWidget() and fails if not in UI Thread, getViewer().getSelectionProvider().getSelection() also invoke control methods calling checkWidget()... Does anyone know a trick to just get the selection without requiring UI Thread? It's really a must-have if we want to make the UI more reactive.
New Gerrit change created: https://git.eclipse.org/r/129010
One positive side-effect with this change is that by removing adherance to UI Thread, it may make the code easy to reuse or port to JDT-LS.
(In reply to Mickael Istria from comment #1) > One major issue I'm facing is that there seems to be no good way to just get > the selection from a ITextViewer without requiring the UI Thread. Indeed, > getViewer().getSelectedRange() call a checkWidget() and fails if not in UI > Thread, getViewer().getSelectionProvider().getSelection() also invoke > control methods calling checkWidget()... > Does anyone know a trick to just get the selection without requiring UI > Thread? It's really a must-have if we want to make the UI more reactive. I see this issue still exists with the patch and we don't have a solution to get the selection without wrapping/running the code in a UI thread. Will it be OK to do so internally even though the new extension attribute and preference option say that UI thread is not required? Also, the patch changes the signature of org.eclipse.jdt.internal.ui.text.template.contentassist.TemplateEngine.complete(...) method in terms of its parameters. Though it is an internal method, I can see that it is being used by JDT Debug at several locations. It might also be used by other clients like Code Recommenders. Dani, do you have any suggestions on how to proceed here?
(In reply to Noopur Gupta from comment #4) > (In reply to Mickael Istria from comment #1) > > One major issue I'm facing is that there seems to be no good way to just get > > the selection from a ITextViewer without requiring the UI Thread. Indeed, > > getViewer().getSelectedRange() call a checkWidget() and fails if not in UI > > Thread, getViewer().getSelectionProvider().getSelection() also invoke > > control methods calling checkWidget()... > > Does anyone know a trick to just get the selection without requiring UI > > Thread? Not really, except one adds some new API to retrieve *last* selection from widget, or something like comment 0. > It's really a must-have if we want to make the UI more reactive. > I see this issue still exists with the patch and we don't have a solution to > get the selection without wrapping/running the code in a UI thread. Will it > be OK to do so internally even though the new extension attribute and > preference option say that UI thread is not required? I personally would not do this, my fear is deadlocks etc. If the javadoc says no UI any client can take any number of locks assuming it will not hurt anyone, but syncExec() call from inside such code *is* an access to potentially locked resource (UI thread). You probably mean JavaContentAssistInvocationContext.getViewerSelection()? The code as is will not work anyway, getViewer().getTextWidget().getDisplay() will throw SWT error outside UI thread.
It out of the scope of this bug, but please note that I had the same problem when I try to manage templates with GenericEditor async completion. See my gerrit patch at https://git.eclipse.org/r/#/c/119801/1/org.eclipse.jface.text/src/org/eclipse/jface/text/templates/TemplateCompletionProcessor.java
(In reply to Andrey Loskutov from comment #5) Thanks, Andrey for taking a look. Yes, I was referring to the same (also, in the context of patches from bug 538630 and bug 531061). Mickael, please revisit the approach for this bug.
(In reply to Andrey Loskutov from comment #5) > I personally would not do this, my fear is deadlocks etc. If the javadoc > says no UI any client can take any number of locks assuming it will not hurt > anyone, but syncExec() call from inside such code *is* an access to > potentially locked resource (UI thread). There doesn't seem to be any other way with the current design, since the IContentAssistProcessor never gets a chance to do some preprocessing on the UI thread first. @Mickael: Shouldn't the jface API be separated in two stages? IAsyncContentAssistProcessor { // called from UI Context createContext(ITextViewer viewer, int offset) // may be called async ICompletionProposal[] computeCompletionProposals(Context context) } createContext() could then memorize the viewer selection. That also has the benefit that the selection would stay the same during the computation.
(In reply to Julian Honnen from comment #8) > @Mickael: Shouldn't the jface API be separated in two stages? This might work, but initial idea was to avoid creating new type and stick with IContentAssistProcessor to ease adoption. Introducing new classes involves adding extra maintenance cost, so unless it's really worth it, we should avoid it and try to deal with existing APIs. What you're suggesting is a new API + some heavy and error-prone refactoring, I'd like to make sure we consider other alternatives that remain internal to JDT.
(In reply to Mickael Istria from comment #9) > I'd like to make sure > we consider other alternatives that remain internal to JDT. I would have thought that other projects have similar problems, but I couldn't find any getSelection() (or similar) calls in PDE and CDT content assists. What I did find are more reasons for a new jface API: 1) progress monitor/cancellation: both JDT and CDT create a NullProgressMonitor in computeCompletionProposals() and then seem to report sensible progress on it. The ability to cancel previous computations seems like a nice benefit. 2) IContentAssistProcessors can't really be executed concurrently. JDT/CDT maintain a state (error message, # of results) which is cleared first in compute proposals. That statefulness is required by the current jface API (IContentAssistProcessor::getErrorMessage)
Some other projects (IIRC LSP4E, PDT, DLTK..) successfully use the current API wothout trouble. Why does JDT need UI Thread and getSelection()? Couldn't this be.replaced by other approaches like listeners or filters?
The TemplateEngine uses the selected text for selection variables (and skips templates containing selection variables when nothing is selected). Otherwise the selected range is used to override the replacement length in JavaCompletionProposal. Not sure exactly what that does. I tried hardcoding some different values without seeing an immediate difference. WDYT about some lower impact API change like a computeCompletionProposals(ITextViewer, ITextSelection) overload?
(In reply to Mickael Istria from comment #9) > (In reply to Julian Honnen from comment #8) > > @Mickael: Shouldn't the jface API be separated in two stages? > > This might work, but initial idea was to avoid creating new type and stick > with IContentAssistProcessor to ease adoption. Looking at this thread, I think the JDT use case justifies new API. (In reply to Julian Honnen from comment #8) > @Mickael: Shouldn't the jface API be separated in two stages? > > IAsyncContentAssistProcessor { > > // called from UI > Context createContext(ITextViewer viewer, int offset) > > // may be called async > ICompletionProposal[] computeCompletionProposals(Context context) > } Could we add these methods to IContentAssistProcessor as default methods? Adding new default methods to an interface should not break API.
(In reply to Lars Vogel from comment #13) > Could we add these methods to IContentAssistProcessor as default methods? > Adding new default methods to an interface should not break API. I'll submit a gerrit with an API proposal. Let's discuss what maintenance/compatibility issues it would cause there.
New Gerrit change created: https://git.eclipse.org/r/142669
Proposal here: https://git.eclipse.org/r/142669
I think fixing bug 547683 should resolve the main issues as it would allow to invoke context.getViewer().getSelection() from whatever thread.
Tentative for 4.13, patch is waiting for review (Jeff already +1'd it but would like Andrey and/or Noopur to make the final call).
(In reply to Mickael Istria from comment #18) > Tentative for 4.13, patch is waiting for review (Jeff already +1'd it but > would like Andrey and/or Noopur to make the final call). https://git.eclipse.org/r/#/c/129010/ I see ITextViewerExtension9#getLastKnownSelection being used now. So, I believe this will take care of Andrey's concerns. Please confirm.
(In reply to Noopur Gupta from comment #19) > I see ITextViewerExtension9#getLastKnownSelection being used now. So, I > believe this will take care of Andrey's concerns. Please confirm. Sorry, I thought you were asking Andrey. Yes, I confirm that usage of getLastKnownSelection is here to remove the risk of deadlocks, which was Andrey's main concern.
Gerrit change https://git.eclipse.org/r/129010 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d9cd2f3580c64bdf8dfa54f861eb251789ec5637
Released for 4.14M3
New Gerrit change created: https://git.eclipse.org/r/152623
New Gerrit change created: https://git.eclipse.org/r/152627
See Bug 553023
Gerrit change https://git.eclipse.org/r/152627 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=9f309774d613680abe496440534dece9e5e744be
Please ensure https://git.eclipse.org/r/129010 & https://git.eclipse.org/r/#/c/152623 go in together after review.
Reopened for review and re-committing the patches.
(In reply to Jeff Johnston from comment #22) > Released for 4.14M3 It will be good to check x-friends list while changing internal methods: org.eclipse.jdt.internal.ui.text.template.contentassist;x-friends:="org.eclipse.jdt.debug.ui",
New Gerrit change created: https://git.eclipse.org/r/152673
(In reply to Sarika Sinha from comment #29) > (In reply to Jeff Johnston from comment #22) > > Released for 4.14M3 > > It will be good to check x-friends list while changing internal methods: > org.eclipse.jdt.internal.ui.text.template.contentassist;x-friends:="org. > eclipse.jdt.debug.ui", Point taken. I have just posted a modified patch for review with the old TemplateEngine.complete() method that JDT debug uses marked as deprecated.
Gerrit change https://git.eclipse.org/r/152673 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=afe2ff9be7e361388cca3e43f94ce5c902149a21
Gerrit change https://git.eclipse.org/r/152623 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=89890a18182b48a3f12073c8e427a7c754d6beae
