Rethink the concept of disposal checks when calling widget methods

[laeubi]

Considerable effort in client code is taken to check for disposal, and also there is again check in the code is self for handling a case that is (at least for me and others I talked about it) rather strange:

1. Most of the time of course I assume the widget is not disposed (otherwise I won't call a method on it) and this also holds true most of the time as an UI is not just flickering around but is live for a very very very very long time (from the point of the computer), still to be sure I need to often check for disposal to not get ugly and disturbing errors in the log or somewhere else.
2. On the other hand, the question is: What should I literally do if the widget is disposed? Scream out loud? Write a message to the log? Kernel panic? Realistically all I can do (and I assume 99% of the time client code do) is nothing and simply not perform the action I'm supposed to perform...

So what I'd like to discuss here is if we should not relax the rules and simply allow widgets to do exactly this (e.g. JFace do it on some places) to check but ignore the disposal.

Lets assume a very basic example of a the 'Label#setText` (quite common also for shells, text, buttons,...) it is currently implemented as (for gtk):

public void setText (String string) {
	checkWidget ();
	if (string == null) error (SWT.ERROR_NULL_ARGUMENT);
	if ((style & SWT.SEPARATOR) != 0) return;
	text = string;
	... some gtk specific stuff ..
}

and corresponding getter:

public String getText () {
	checkWidget ();
	if ((style & SWT.SEPARATOR) != 0) return "";
	return text;
}

So why not simply change it to

public void setText (String string) {
	checkWidgetWithoutDisposeCheck ();
	if (string == null) error (SWT.ERROR_NULL_ARGUMENT);
	if ((style & SWT.SEPARATOR) != 0) return;
	text = string;
        if (disposed) {
           return;
        }
	... some gtk specific stuff ..
}

public String getText () {
	checkWidgetWithoutDisposeCheck ();
	if ((style & SWT.SEPARATOR) != 0) return "";
	return text;
}

It is often argued that this might "hide" errors, but I always asking how is something like

if (!label.isDisposed() {
  label.setText("My new text");
}

any better?
Won't I notice the bug (if there is any) even better in the UI because my label is not shown than I ever could by the disposal checks? Do we really help the developer here? My impression is that most of these missing disposal checks are not discovered at development time but on runtime in situations where e.g there is something shutting down, an event is getting late and I actually can't do anything and it is not an programming error at all (unless I want to write a program that has one strict linear workflow).

[iloveeclipse]

Your both basic assumptions are wrong, so the rest is just the result of wrong assumptions.

Most of the time of course I assume the widget is not disposed

That's wrong. Many UI's are dynamic, not static. Dialogs are created and closed, editors are opened and closed etc. All non trivial UI code usually has some tasks that should be done asynchronous (listeners, jobs etc). All these are calling back to UI that can be not there anymore because user decided to close dialog / editor / switch off some toggle etc.

What should I literally do if the widget is disposed? Scream out loud? Write a message to the log? Kernel panic?

Throw an exception, as it is done today.

Realistically all I can do (and I assume 99% of the time client code do) is nothing and simply not perform the action I'm supposed to perform...

Wrong again. If you don't do anything in a non-trivial code, you might not notice that the "XYZ" button didn't do anything useful because the respective UI element was disposed meanwhile. Today, if you done your work right, user will get an error dialog saying that XYZ operation failed, or at least and "unhandled execution error in the log"...

My impression is that most of these missing disposal checks are not discovered at development time but on runtime in situations where e.g there is something shutting down

Wrong impression. As said, UI's aren't always static and only dispose on shutdown.

[laeubi]

That's wrong. Many UI's are dynamic, not static. Dialogs are created and closed, editors are opened and closed etc.

That's not very 'dynamic' as it happens mostly on users interaction, so lets assume I close a dialog and the label is still updated a few ms after that for a last time, so how cares? The label is not visible anymore ...

If you don't do anything in a non-trivial code, you might not notice that the "XYZ" button didn't do anything useful because the respective UI element was disposed meanwhile

Never noticed that and never ever seen a disposal message in such a case to be honest ...

[laeubi]

By the way just wondering, can you think of any fix where such a "already disposed" report was not fixed by adding an if (disposed ) check? I'm really curious about that but don't know a good way to search for such a thing...

[jukzi]

When i call setText(s) i would expect getText() to return s. That contract would be broken with silent ignoring calls. I prefer to get the code fixed avoiding calls to already disposed widgets.

[laeubi]

That contract would be broken with silent ignoring calls

It won't please check the example above, exactly that will happen.

[mickaelistria]

I think the net effect would be fewer visible "widget is disposed" errors. But what does that bring? How much development effort would be saved? Somehow, we can also wonder why Java sends a NullPointerException instead of just being silent.
Some errors are useful, and the fact that they force a check can be seen as a trigger for higher quality consumer code.
I'm not sold on one way or the other, but I sympathize than shutting up some errors may not actually be an improvement.
For this particular case, sent exception for disposed widget are part of API contract via Javadoc. So it would be a breaking change on all API methods. Something to consider for SWT 4 maybe in the best case.

[laeubi]

I think the net effect would be fewer visible "widget is disposed" errors. But what does that bring?

Just search for references of isDisposed() (> 3000) I have just clicked some and gave up to find any reference where it is not:

if (!disposed) {
//  do something
} else {
 //do nothing
}

Just an example from JFace:

	/**
	 * Sets the title for this dialog.
	 * @param title the title.
	 */
	public void setTitle(String title) {
		Shell shell= getShell();
		if ((shell != null) && !shell.isDisposed()) {
			shell.setText(title);
		}
	}

How much development effort would be saved?

I opened this as @iloveeclipse and @jukzi multiple times has request a method that is only executed when a widget is not disposed, also there are sever util methods and hundred of check... actually if one checks the dispose is checked twice. Even if its cheap.

Some errors are useful, and the fact that they force a check can be seen as a trigger for higher quality consumer code.

I don't see how such checks add any quality but boilerplate to the code, just take the JFace example, why is this "higher" quality than just:

	/**
	 * Sets the title for this dialog.
	 * @param title the title.
	 */
	public void setTitle(String title) {
		Shell shell= getShell();
		if ((shell != null) {
			shell.setText(title);
		}
	}

By the way a very good example, if the shell is disposed and I set the text on it and it simply would be a noop what will ever be hidden/erroneous here? How does the dispose check improves anything here?

For this particular case, sent exception for disposed widget are part of API contract via Javadoc. So it would be a breaking change on all API methods. Something to consider for SWT 4 maybe in the best case.

Sure, but if we not starting to think about such things now this wont happen for SWT 5 either :-)
I also suggest that we don't do any checks but allowing more graceful behavior would actually be beneficial.

[merks]

I think the direction "rethink the dispose check" will go down a long and frustrating path that you won't like and that no one else will like. It's likely one of those things that everyone will find frustrating including and perhaps especially you.

And of course, as @mickaelistria suggests, it's in the API

 *
 * @exception IllegalArgumentException <ul>
 *    <li>ERROR_NULL_ARGUMENT - if the string is null</li>
 * </ul>
 * @exception SWTException <ul>
 *    <li>ERROR_WIDGET_DISPOSED - if the receiver has been disposed</li>
 *    <li>ERROR_THREAD_INVALID_ACCESS - if not called from the thread that created the receiver</li>
 * </ul>

So this is simply water under the bridge and there is no pushing the water back upstream to the year 2000 to set it on a different course...

[jukzi]

The typical usecase of isDisposed should be eager stop work on something that is not visibly anymore anyway
if (x.isDisposed()) return
and can safe a lot of work which would be done when the view is still visible.

[laeubi]

Sure this could be useful even though I think its better checking a "stopped" or "closed" than tying that to a disposal of a widget, anyways this option won't vanish if we allow to do some interactions on a disposed widget.

[basilevs]

An action on an UI control leads to tons of side effects - listeners are called, focus is shifted, scroll moved, data updated etc. The assumption is that unless an exception is thrown, all side effects have completed successfully. This will no longer be the case without disposal checks - some side effects would be quietly skipped. This can't be worked around in Java code as some side effects are processed by windowing system.