Introduce SWT.Disposed event supplementing SWT.Dispose

[tmssngr]

Is your feature request related to a problem? Please describe. There is an event Dispose that is sent when a control is disposed. Example: If a resource is created, that is shared among some controls, and needs to be disposed after using, one might tend to create code like (see a full example snippet at the bottom)

final Font font = new Font(composite.getDisplay(), "Courier New", 11, 0);
composite.addListener(SWT.Dispose, event -> font.dispose());

final Label label = new Label(composite, SWT.NONE);
label.setFont(font);
...

But this is wrong, because when the shell is disposed (click [x]), the Dispose listener is sent at the beginning of the disposing process in Widget.release(boolean), before it proceeds to child controls, before any child control has been disposed. In the example, the Dispose listener to dispose the font is called before label gets disposed! Hence, the label will held a disposed font for a short time. Often this is not a problem, because nothing needs that font in the mean time, but sometimes this causes hard to reproduce crashes, because, e.g. disposing a Shell with a Tree control disposes a tooltip, resulting in a FocusIn event to a not yet disposed child, causing a color change on a StyledText. The StyledText control can't detect that, because during the disposing process, no parent's isDisposed method will return true.

Describe the solution you'd like Currently, there is no event sent after the control has been really disposed, including all children. Hence I suggest to introduce an Disposed event (mind the additional d at the end) and send it at the end of Widget.release(boolean).

Describe alternatives you've considered In the SWT.Dispose listener one can delay the font disposal with event.display.asyncExec(() -> font.dispose());, but this will run much later (maybe too late), if at all. We tend to avoid display.asyncExec() to get the events into the right order, because this easily becomes a nightmare.

Full example snippet of problematic code

import org.eclipse.swt.*;
import org.eclipse.swt.custom.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class ParentDisposeBeforeChildren {

    public static void main(String[] args) {
        final Display display = new Display();
        display.addListener(SWT.Skin, event -> {
            if (event.type == SWT.Skin) {
                if (event.widget instanceof StyledText st) {
                    updateSelectionBackground(st, st.isFocusControl());
                }
            }
        });

        final Shell shell = new Shell(display);
        shell.setLayout(new FillLayout());

        createComposite(shell);

        shell.setSize(400, 300);
        shell.open();

        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }

        display.dispose();
    }

    private static void createComposite(Composite parent) {
        final Composite composite = new Composite(parent, SWT.NORMAL);
        composite.setLayout(new GridLayout());

        final Text text = new Text(composite, SWT.BORDER);
        text.setLayoutData(new GridData(SWT.FILL, SWT.TOP, true, false));

        final Font font = new Font(composite.getDisplay(), "Courier New", 11, 0);
        composite.addListener(SWT.Dispose, event -> font.dispose());

        final StyledText st = createStyledText(composite, font);
        createStyledText(composite, font);

        text.addListener(SWT.Dispose, event -> st.setFocus());
    }

    private static StyledText createStyledText(Composite parent, Font font) {
        final StyledText st = new StyledText(parent, SWT.BORDER | SWT.READ_ONLY);
        st.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
        st.setFont(font);
        st.setText("Hello world!");
        st.selectAll();

        final Listener listener = event -> updateSelectionBackground(st, event.type == SWT.FocusIn);
        st.addListener(SWT.FocusIn, listener);
        st.addListener(SWT.FocusOut, listener);
        return st;
    }

    private static void updateSelectionBackground(StyledText st, boolean focused) {
        st.setSelectionBackground(st.getDisplay().getSystemColor(focused ? SWT.COLOR_DARK_GREEN : SWT.COLOR_GRAY));
    }
}

[laeubi]

I think your presented disposal procedure is wrong as the font does not belong to the composite but to the label, so why not add the listener to the label?

Beside that its hard to guess what you mean with "crash", will there be a core-dump?

How would one need to run the snippet to see the behavior? Are there any screenshots / stack-traces you can share?

[tmssngr]

I think your presented disposal procedure is wrong as the font does not belong to the composite but to the label, so why not add the listener to the label?

Because, like in the large example, the font is shared by multiple controls.

Beside that its hard to guess what you mean with "crash", will there be a core-dump?

No, just such an exception

java.lang.IllegalArgumentException
    at org.eclipse.swt.SWT.error(SourceFile:4899)
    at org.eclipse.swt.SWT.error(SourceFile:4833)
    at org.eclipse.swt.SWT.error(SourceFile:4804)
    at org.eclipse.swt.graphics.TextLayout.setFont(SourceFile:3099)
    at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(SourceFile:1196)
    at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(SourceFile:911)
    at org.eclipse.swt.custom.StyledTextRenderer.calculate(SourceFile:301)
    at org.eclipse.swt.custom.StyledTextRenderer.calculateClientArea(SourceFile:331)
    at org.eclipse.swt.custom.StyledText.resetCache(SourceFile:7831)
    at org.eclipse.swt.custom.StyledText.setSelectionBackground(SourceFile:9744)
    at com.syntevo.q.gui.theme.QTheme.updateSelectionColors(SourceFile:420)
    at com.syntevo.q.gui.theme.QTheme.handleStyledText(SourceFile:356)
                                     .lambda$handleStyledText$0
    at org.eclipse.swt.widgets.EventTable.sendEvent(SourceFile:89)
    at org.eclipse.swt.widgets.Display.sendEvent(SourceFile:4262)
    at org.eclipse.swt.widgets.Widget.sendEvent(SourceFile:1066)
    at org.eclipse.swt.widgets.Widget.sendEvent(SourceFile:1090)
    at org.eclipse.swt.widgets.Widget.sendEvent(SourceFile:1071)
    at org.eclipse.swt.widgets.Control.sendFocusEvent(SourceFile:2941)
    at org.eclipse.swt.widgets.Widget.wmSetFocus(SourceFile:2321)
    at org.eclipse.swt.widgets.Control.WM_SETFOCUS(SourceFile:5425)
    at org.eclipse.swt.widgets.Canvas.WM_SETFOCUS(SourceFile:437)
    at org.eclipse.swt.widgets.Control.windowProc(SourceFile:4816)
    at org.eclipse.swt.widgets.Canvas.windowProc(SourceFile:340)
    at org.eclipse.swt.widgets.Display.windowProc(SourceFile:5031)
    at org.eclipse.swt.internal.win32.OS.SetFocus
    at org.eclipse.swt.widgets.Control.forceFocus(SourceFile:1092)
    at org.eclipse.swt.widgets.Control.setFocus(SourceFile:3440)
    at org.eclipse.swt.widgets.Composite.setFocus(SourceFile:1081)
    at org.eclipse.swt.custom.StyledText.setFocus(SourceFile:8742)
    at org.eclipse.swt.widgets.Decorations.restoreFocus(SourceFile:759)
    at org.eclipse.swt.widgets.Decorations.WM_ACTIVATE(SourceFile:1523)
    at org.eclipse.swt.widgets.Shell.WM_ACTIVATE(SourceFile:2312)
    at org.eclipse.swt.widgets.Control.windowProc(SourceFile:4741)
    at org.eclipse.swt.widgets.Canvas.windowProc(SourceFile:340)
    at org.eclipse.swt.widgets.Decorations.windowProc(SourceFile:1478)
    at org.eclipse.swt.widgets.Shell.windowProc(SourceFile:2284)
    at org.eclipse.swt.widgets.Display.windowProc(SourceFile:5031)
    at org.eclipse.swt.internal.win32.OS.DestroyWindow
    at org.eclipse.swt.widgets.Tree.releaseWidget(SourceFile:4045)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:833)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Composite.releaseChildren(SourceFile:925)
    at org.eclipse.swt.widgets.Canvas.releaseChildren(SourceFile:168)
    at org.eclipse.swt.widgets.Decorations.releaseChildren(SourceFile:711)
    at org.eclipse.swt.widgets.Shell.releaseChildren(SourceFile:1376)
    at org.eclipse.swt.widgets.Widget.release(SourceFile:821)
    at org.eclipse.swt.widgets.Widget.dispose(SourceFile:428)
    at org.eclipse.swt.widgets.Decorations.dispose(SourceFile:392)
    at com.syntevo.q.gui.QFrame.dispose(SourceFile:353)

How would one need to run the snippet to see the behavior?

Just run the large snippet and close the shell.

[mickaelistria]

as a workaround, you might trydisposing resources asynchronously: composite.addListener(SWT.Dispose, event -> font.getDisplay().asyncExec(font::dispose)); which should queue the disposal after other sync events (actual disposal of children) are process.

[laeubi]

I think it must be fixed in TextLayout or StyledTextRenderer so the down't try to use disposed font

[mickaelistria]

I think it must be fixed in TextLayout or StyledTextRenderer so the down't try to use disposed font

I'm not sure. In the API, it says "use that font" (and doesn't give much room for fallback solutions), and one could expect that if a font is set, it has to be used; and crashing if font is incorrect is a consistently good behavior. Also, this is one specific stacktrace, but the case of a font being disposed at the wrong time is per-se a bug that really needs to be fixed (is client code, and maybe introducing a solution in SWT if there is nothing better), so the error must pop-up in some way. Otherwise, it's more putting lipstick on a pig.

[laeubi]

In SWT the font is not managed by the control so a control can never assume the font is there "forever" (especially in disposal) given that fonts are even null at the beginning it seems valid to assume a disposed font == null font (I think thats similar to how org.eclipse.swt.widgets.Control.setFont(Font) and others work, they only assert that the font is not dispose ad setter time.

[tmssngr]

as a workaround, you might trydisposing resources asynchronously: composite.addListener(SWT.Dispose, event -> font.getDisplay().asyncExec(font::dispose)); which should queue the disposal after other sync events (actual disposal of children) are process.

Yes, I wrote that in the Describe alternatives you've considered section.

What about the SWT.Disposed listener idea? Yay or nay?

[laeubi]

What about the SWT.Disposed listener idea? Yay or nay?

I'm not sure if it is a good idea to add more complexity to this already brittle concept of disposing widgets. As said widget should be able to work for cases when resources not managed by them getting disposed and this is then more a usage problem than something solvable with more events.

I would even say that the whole test-case has a smell, e.g. this one:

text.addListener(SWT.Dispose, event -> st.setFocus());

seems to be an indication that in the real code there is something going on that performs actions on an already disposed event (e.g. in an sync or sync exec).

[tmssngr]

The line

text.addListener(SWT.Dispose, event -> st.setFocus());

is for making the stacktrace above reliably reproducible. Under very special conditions (we haven't found out) it can happen - as the long stacktrace proves.

Why not have an event that can be used to indicate, that this control and all children were disposed? Control hierarchies are tree structures. Traversing tree structures usually means entering a node and leaving a node - look at Files.walkFileTree(Path, FileVisitor). For what kind of code do we need a beginning-to-dispose event? IMHO, the a disposing-finished event would be more helpful.

[laeubi]

Why not have an event that can be used to indicate, that this control and all children were disposed?

Because that's not what happening, see javadoc of dispose() disposal only happens for the control where dispose is called, children might or might not get disposed (but often released) and even a control can have children that are disposed this can happen in any order and even multiple times (as far as I understand the code and documentation). So there is only a guarantee that you get a dispose event not when and in what orders.

As I see some custom widgets in the stack, some might violate the contract of SWT, e.g. not call checkWidget(), rely on dispose() being called or disposing resources that are actually in use.

[tmssngr]

In our own clone of the SWT repository I've added a check in Control.releaseChildren(boolean) whether the set font is disposed. That way I had found a couple of code places where a font was disposed though it still was set to a non-disposed control and potentially could cause a font-disposed exception under rare conditions. In other words, now the check causes always a crash for buggy code instead of very rarely. Would you be interested in having this check in the official SWT?