Community
Participate
Working Groups
Build Identifier: 20100617-1415 When ComboViewers and other ContentViewers are disposed, they clear their content provider. If setInput is called later, instead of failing with a widget disposed error, they throw an error related to having no content provider. This sets the developer down the wrong track when debugging it. Instead, we should add an additional check in ContentViewer to throw an SWT exception right before the line Assert.isTrue(getContentProvider() != null, "ContentViewer must have a content provider when input is set."); //$NON-NLS-1$ Reproducible: Always Steps to Reproduce: Create a ComboViewer with ArrayContentProvider, dispose it, then set a list of strings as input.
Created attachment 178310 [details] Patch v01 Some of the other methods do check for the control's state. Boris, I gather that getControl()==null implies that the control has been disposed. Just had a mild doubt about it after looking at the patterns in the code... Thanks.
The check itself looks good to me, but we shouldn't use "SWT.error(SWT.ERROR_WIDGET_DISPOSED);" because that's usually a sign that you are using SWT in an invalid way. In this instance, I would do something like: throw new IllegalStateException("Need an underlying widget to be able to set the input (has the widget been disposed?)");
(In reply to comment #2) > The check itself looks good to me, but we shouldn't use > "SWT.error(SWT.ERROR_WIDGET_DISPOSED);" because that's usually a sign that you > are using SWT in an invalid way. In this instance, I would do something like: > > throw new IllegalStateException("Need an underlying widget to be able to set > the input (has the widget been disposed?)"); Thanks Boris. Sounds good to me, will make a new patch on monday.
Created attachment 178626 [details] Patch v02 (In reply to comment #3) > > Thanks Boris. Sounds good to me, will make a new patch on monday. On second thought, the patch was rather simple; released it to CVS HEAD.
Marking as Fixed.
Verified in I20101012-0800.
I'm curious why checking the control was implemented in this change. The description talks about the ContentProvider being disposed, not the Control. Was any consideration given to backwards compatibility when this patch was accepted? I'm asking because I'm upgrading an application to be based on Eclipse 3.7 (from 3.4 or 3.5) and some of the code used custom a "NullViewer" that had no visible Control. It worked perfectly before, but now runs into this IllegalStateException. It seems to me that the contract of ContentViewer was changed as a result of this change, breaking existing subclasses of ContentViewer.
As the Eric Rizzo remarks, I'm also surprise by this fix because, because the SWT Control is optional as it's written in javadocs : "create SWT controls for viewer (in constructor) (optional)" So why check in setInput method if the control exist or not and if it disposed ? I'm checking compatibilty of tools developped on version 3.4, and it's the only blocking point for us. So this fix definitly change the contract of this class.
(In reply to comment #7) > I'm curious why checking the control was implemented in this change. The > description talks about the ContentProvider being disposed, not the Control. The problem was (as I read it) the widget was disposed (because the viewer was disposed) but the viewer was throwing an error about the content provider, causing people to look for incorrect setContentProvider(*) instead of the eager dispose. But it seems to me that the current solution also causes problems. I'll take any patches to fix. PW
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
