Skip to content

Commit

Permalink
Call getLineCount in UI Thread
Browse files Browse the repository at this point in the history 
Fixes eclipse-platform#172
  • Loading branch information
@mickaelistria
mickaelistria committed Mar 29, 2023
1 parent b9355a2 commit 9515786
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions org.eclipse.jface.text/src/org/eclipse/jface/text/source/AnnotationRulerColumn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,12 @@ public int compare(Tuple o1, Tuple o2) {

@Override
public void textChanged(TextEvent event) {
if (event.getViewerRedrawState() && fCachedTextWidget.getLineCount() != previousLineCount) {
previousLineCount= fCachedTextWidget.getLineCount();
postRedraw();
}
fCachedTextWidget.getDisplay().execute(() -> {

This comment has been minimized.

Sign in to view

Copy link
@szarnekow

szarnekow Mar 30, 2023

I can find plenty ITextListener implementations in platform that assume the listener is notified on the UI thread (e.g. performs redraws of the widget). Why do you think this listener here should be notified not on the UI thread in the first place?

Point is: The change here looks wrong and the entire call stack in the ticket is too short to tell which code path let to the listener being invoked on a non-ui thread.

This comment has been minimized.

Sign in to view

Copy link
@mickaelistria

mickaelistria Mar 30, 2023

Author Owner

The stack at eclipse-platform#172 is all that is available but I think it tells it all: document.set() can be called in any thread, and from here the event can propagate to TextEvent in this same thread (this is usually OK), but this particular implementation of the listener requires the UI Thread as it calls low-level widget operations StyledText.getLineCount(), that why this particular listener needs to ensure it runs in UI Thread.
The could indeed be better way of implementing that, using some other API that do not require UI Thread to compute the same thing. If you have some suggestion, it would be welcome.

The change here looks wrong

Is it causing a regression somewhere?

This comment has been minimized.

Sign in to view

Copy link
@iloveeclipse

iloveeclipse Mar 30, 2023

@szarnekow : what exactly do you mean by "looks wrong"?

This comment has been minimized.

Sign in to view

Copy link
@szarnekow

szarnekow Mar 30, 2023

The change here patches a single ITextListener implementation to dispatch the logic to the display thread, if it currently does not run on the display thread.
ITextListener states

 * <p>
 * If a text listener receives a text event, it is guaranteed that both the
 * document and the viewer's visual representation are synchronized.</p>
 * <p>

I might read to much into it, but the fact that document and document are synchronized means to me that we are on the display thread. Otherwise the visual representation would not be accessible in the notification.

Which brings me to the question, why we didn't see similar failures for other existing implementations of ITextListener that happily require the display thread. All these would / should have failed in the past already and bugzilla would be full of such exceptions.
See

org.eclipse.ui.texteditor.TextViewerDeleteLineTarget.DeleteLineClipboard.textChanged(TextEvent)
org.eclipse.jface.text.TextViewer.FindReplaceRange.textChanged(TextEvent)
org.eclipse.ui.texteditor.IncrementalFindTarget.textChanged(TextEvent)
org.eclipse.jface.text.source.AbstractRulerColumn.InternalListener.textChanged(TextEvent)
org.eclipse.jface.text.source.LineNumberRulerColumn.InternalListener.textChanged(TextEvent)

and others.

Why can the LineNumberRuleColumn remain on the display thread but this one here would require a modification?

My gut tells me that the code path that lead to the stacktrace is supposed to be reviewed instead of point-fixing one instance of thread violation.

That was what I inaccurately summarized as "looks wrong".

This comment has been minimized.

Sign in to view

Copy link
@iloveeclipse

iloveeclipse Mar 30, 2023

Thanks Sebastian, I've missed the point by looking on the changed code, not on the context.
if (event.getViewerRedrawState() && fCachedTextWidget.getLineCount() != previousLineCount) {
previousLineCount= fCachedTextWidget.getLineCount();
postRedraw();
}
});
}
};

Expand Down

0 comments on commit 9515786

Please sign in to comment.