[Win32] Check widget for disposal before zoom change notification

[HeikoKlare]

In most scenarios, zoom change processing happens asynchronously. In consequence, widgets may evolve and even already be disposed until a notification for a zoom change event on them actually is processed. This can lead to disposed widget exceptions in case such a notification is sent on an already disposed widget.

This change adapts all places that send zoom change notification on DPI change to check for disposal of the widget to send the notification to first.

Fixes #3043

[laeubi]

Thanks for providing a fix for that!

This can lead to disposed widget exceptions in case such a notification is sent on an already disposed widget.

I'm not sure its relevant here but I mostly see it during remote-desktop sessions when connecting from different computers. So in this particular case it might be that the image needs to be refreshed.

[HeikoKlare]

So in this particular case it might be that the image needs to be refreshed.

As a DPI change event seems to be triggered in that case (according to the stack trace with the disposed error), images should automatically be refreshed via the DPI change handlers. If you notice that anything is wrong with the images, please let me know.

[laeubi]

It all seems to work except that ones get an error dialog on DPI change, after dismiss the dialog everything is working.

[github-actions]

Test Results

  176 files  ±0    176 suites  ±0   29m 7s ⏱️ + 1m 36s
4 684 tests ±0  4 662 ✅ ±0  22 💤 ±0  0 ❌ ±0 
  485 runs  ±0    479 ✅ ±0   6 💤 ±0  0 ❌ ±0 

Results for commit f6ef9c7. ± Comparison against base commit 3b12675.

♻️ This comment has been updated with latest results.

[HeikoKlare]

It all seems to work except that ones get an error dialog on DPI change, after dismiss the dialog everything is working.

The dialog pops up because of the error reported in #3043, or is there anything else?

[laeubi]

It all seems to work except that ones get an error dialog on DPI change, after dismiss the dialog everything is working.

The dialog pops up because of the error reported in #3043, or is there anything else?

Yes it seems to be a standard error dialog of Eclipse (with just text "Widget Disposed") then one can dismiss and application seem to work (but maybe have missed some updates of course). It might be become visible now the automatic scaling is the default?

[HeikoKlare]

It might be become visible now the automatic scaling is the default?

That might be the case, yes. If I understand the description in the issue correctly, it's a pure E4 application, which was (by accident) not affected by the default enablement via preference we had introduced more than half a year ago. So it might be affected by the recent change of SWT to use monitor-specific scaling by default.

The arising question is of course why there is a disposed TreeItem at all. It might be that this disposed item has been there by accident already before, but that it did not lead to any issues as no one has accessed it. That changed now with the DPI change processing being applied and accessing the item, which reveals that disposed state.

[laeubi]

The arising question is of course why there is a disposed TreeItem at all.

SWT has the concept of virtual Table/Tree (using SWT.VIRTUAL), in such case there is a model an items are only created when the actual thing becomes visible. It might be things get disposed as well ... e.g. because of the DPI change more/less becomes visible, but this is only a guess!

[HeikoKlare]

Sounds like a reasonable guess, but not that easy to verify. And assuming it's correct, just ignoring disposed elements for updates on DPI changes seems fine to me. So should we go with this change? You could then report if the error dialog still pops up for you or if you face any other issues with the trees in your application afterwards.

[laeubi]

Yes I think just ignore them would make sense here.

[fedejeanne]

The code looks good, I just have some questions:

1. Doesn't this call also need a check?

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java

Line 1775 in c891c43

item.notifyListeners(SWT.ZoomChanged, event);

2. Would it make sense to check for event.doit like it's being done in Control::sendZoomChangeEvent just in case someone decides to stop the propagation (for whatever reason)?

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java

Line 6121 in c891c43

if (!this.isDisposed() && event.doit) {

3. Doesn't the call in Shell also need a check? I suppose shell is a special case but I just want to check:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java

Line 2534 in c891c43

notifyListeners(SWT.ZoomChanged, zoomChangedEvent);

[HeikoKlare]

Thank you for the feedback. The proposed extraction into a separate method definitely makes sense. I have adapted the PR accordingly.

1. Doesn't this call also need a check?

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java

Line 1775 in c891c43

item.notifyListeners(SWT.ZoomChanged, event);

I thought about whether we need a check here as well. The reason why I didn't add it is because the code around that refreshes (or rather recreates) the tool button would actually fail if the item was disposed. When taking a deeper look, I even think that this line of code might be completely useless. The buttons are recreated anyway and the handleDPIChange implementation of Item has no effect on an ToolItem anyway (as setting the same image leads to an early return). Thus I would rather completely remove this line, but for the sake of low risk would wait with that for 2026-06 M1 as keeping the line for now does not hurt and there might still be a risk of unexpected regressions.

2. Would it make sense to check for event.doit like it's being done in Control::sendZoomChangeEvent just in case someone decides to stop the propagation (for whatever reason)?

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java

Line 6121 in c891c43

if (!this.isDisposed() && event.doit) {

event.doIt is only (supposed to be) set to false by a concurrent DPI change event (see Shell.handleMonitorSpecificDpiChange()). Since the notifyListeners calls happen synchronously within the UI thread, there is no change that a concurrent DPI change event comes in and sets the event.doIt flag to false while synchronously processing a change. So the evaluation of event.doIt is only necessary when event queue processing has happened (i.e., when processing an asynchronous DPI change event), which happens in sentZoomChangeEvent where that check is already present.

3. Doesn't the call in Shell also need a check? I suppose shell is a special case but I just want to check:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java

Line 2534 in c891c43

notifyListeners(SWT.ZoomChanged, zoomChangedEvent);

This method is only called when a WM_DPICHANGED event is triggered on the shell, which can only happen if the shell is not disposed.

[fedejeanne]

Thank you for the clarifications!

I thought about whether we need a check here as well. The reason why I didn't add it is because the code around that refreshes (or rather recreates) the tool button would actually fail if the item was disposed. When taking a deeper look, I even think that this line of code might be completely useless. The buttons are recreated anyway and the handleDPIChange implementation of Item has no effect on an ToolItem anyway (as setting the same image leads to an early return). Thus I would rather completely remove this line, but for the sake of low risk would wait with that for 2026-06 M1 as keeping the line for now does not hurt and there might still be a risk of unexpected regressions.

Makes sense to postpone the change to M1 to avoid surprises if the current state doesn't hurt 👍

event.doIt is only (supposed to be) set to false by a concurrent DPI change event (see Shell.handleMonitorSpecificDpiChange()). Since the notifyListeners calls happen synchronously within the UI thread, there is no change that a concurrent DPI change event comes in and sets the event.doIt flag to false while synchronously processing a change. So the evaluation of event.doIt is only necessary when event queue processing has happened (i.e., when processing an asynchronous DPI change event), which happens in sentZoomChangeEvent where that check is already present.

Ah, ok. I was mistakenly assuming that the changes were split into several smaller async calls and that there was a (near-zero) chance that some newer DPI change event would happen while processing the older DPI change event.

This method is only called when a WM_DPICHANGED event is triggered on the shell, which can only happen if the shell is not disposed.

Makes sense 👍

[fedejeanne]

All good from my side.

@laeubi may we merge this one?

[laeubi]

You both have commit rights here and as there is no disagreement and its fixing a bug I think it can be merged as soon as you like.