Minimap can cause NPE #69

[GeraldMit]

Minimap can cause NPE #69 - fix with null checks

[mickaelistria]

On which aspect is using the StyledText widget instead of the ITextViewer better? If there is no reason, maybe we could just get rid of all the code that uses the widget and only consider the viewer. @angelozerr any remembrance?

[angelozerr]

On which aspect is using the StyledText widget instead of the ITextViewer better?

Sorry I don't understand your comment. We need to use StyledText for mini map, no?

[mickaelistria]

Sorry I don't understand your comment. We need to use StyledText for mini map, no?

@GeraldMit seems to have a case of minimap without StyledText.

[angelozerr]

Minimap StyledText backgound is filled with Editor Styledtext background https://github.com/eclipse-platform/eclipse.platform.text/pull/138/files#diff-837e48e63ea38444cbdccb28c934140420b4948f877a6613a3ffe047f843fac6R289

How to manage that with ITextViewer?

[iloveeclipse]

@GeraldMit : any steps how to reproduce #69?

[GeraldMit]

@GeraldMit : any steps how to reproduce #69?

The editor that displayed the issue in the MinimapWidget is proprietary and I can't share it, but it has existed for many years and predates Minimap.

I could possibly write a different ITextViewer implementation not using SourceViewer or TextViewer as an example in order to demonstrate the issue but that will take me quite a while, and as this is entire issues is a minor change just protecting against NPEs in the MinimapWidget, I am not sure the effort is worth the results?

Note: My case is that the rendering widget is not a StyledText because I am not using the standard SourceViewer or TextViewer; so my choices are to ignore API and hack something together, or return null.

StyledText editorTextWidget = fEditorViewer.getTextWidget();

fEditorViewer if not disposed is expected to return a StyledText as the rendering of textWidget, which is based on the ITextViewer API.

Seems reasonable in that context, but as I can't use a StyledText from my ITextViewer implementation in the UI as I need a different behavior and control.

This isn't new by any means... https://archive.eclipse.org/eclipse/downloads/documentation/2.0/html/plugins/org.eclipse.platform.doc.isv/reference/api/org/eclipse/jface/text/ITextViewer.html

The API isn't really specific, because it assumes the TextWidget that the extension for Viewer is using MUST be a StyledWidget and you MUST have one unless you are disposed.

getTextWidget public StyledText getTextWidget() Returns this viewer's SWT control, null if the control is disposed. Returns: the SWT control

The problem is that the Interface is the most base level, and explicitly declares a UI Class that can't be extended or otherwise reimplemented.

Anyways, just accepting a null can happen (which is the case if the textWidget was disposed but not the Viewer yet) is enough to not have constant NPEs if MiniMap is present, which is enough for me.

[mickaelistria]

@GeraldMit What is expected to happen in this case? The minimap shows nothing? This patch adds null-check in many places. Wouldn't it be possible to just get the execution flow of the minimap fully disabled when the viewer doesn't provide a StyledText, adding fewer null-checks earlier and keeping the "behavior" code unchanged?

[GeraldMit]

@mickaelistria Correct, it shows nothing. It could instead show a message, but with no editor available it also shows nothing so I thought it was fine; I could not think immediately of a message that conveyed useful succinct information for the user.

I was unsure about the other possible null case with the dispose as an example so it seemed safest to just always check and have the most operation possible.

updateMinimap could be reduced to practice a bit, not assigning to variables and reformatting to repeating a bit with an if statement.

if (editorTextWidget == null) {
     fMinimapTracker.updateMinimap(
          fEditorViewer.getTopIndex(), 
          fEditorViewer.getBottomIndex(), 
          0, textChanged);
     return;
}
int editorTopIndex = JFaceTextUtil.getPartialTopIndex(editorTextWidget);
int editorBottomIndex = JFaceTextUtil.getPartialBottomIndex(editorTextWidget);
int lineHeight = editorTextWidget.getLineHeight();
int maximalLines = (lineHeight == 0) ? 0 : editorTextWidget.getClientArea().height/ lineHeight;
fMinimapTracker.updateMinimap(editorTopIndex, editorBottomIndex, maximalLines, textChanged);

This would reduce the amount of checks.

[mickaelistria]

@GeraldMit Your latest proposal seems better. If you agree with it, can you please amend your PR to implement it as discussed here?