Bug 19462

Comment 1 Dejan Glozic 2002-06-06 08:55:37 EDT

You probably meant to open this against the platform.

Comment 2 Karice McIntyre 2006-06-28 10:23:19 EDT

fixed in HEAD for build > 20060628.  

Comment 3 Karice McIntyre 2006-08-09 12:47:00 EDT

verified in I20060809-0010

Comment 4 Stefan Xenos 2006-12-01 15:03:53 EST

This fix is making all WizardDialogs huge on GTK. See bug 166544.

Comment 5 Stefan Xenos 2007-03-20 20:41:36 EDT

Please revert this patch until a fix can be found that does not cause regressions.

Comment 6 Tod Creasey 2007-03-20 21:01:58 EDT

Lets revert as suggested

Comment 7 Tod Creasey 2007-03-21 15:08:55 EDT

Created attachment 61592 [details]
Patch to roll back changes

Comment 8 Tod Creasey 2007-03-26 14:43:09 EDT

Fixed in build >20070326

Comment 9 Tod Creasey 2007-04-30 16:31:27 EDT

Verified in  I20070430-0010

Comment 10 Octavio Berlanga 2008-12-14 12:17:38 EST

This issue has not been fixed.  This problem is still present in build M20080911.

It seems like the roll back was verified after the fix that failed on GTK.  However, the original issue of space being reserved for the progress bar still persists.

Can we re-open this bug?

Comment 11 Remy Suen 2008-12-15 05:37:21 EST

(In reply to comment #10)
> This issue has not been fixed.  This problem is still present in build
> M20080911.
> 
> It seems like the roll back was verified after the fix that failed on GTK. 
> However, the original issue of space being reserved for the progress bar still
> persists.
> 
> Can we re-open this bug?

Sounds reasonable to me.

Comment 12 Boris Bokowski 2009-11-26 16:21:35 EST

Prakash is now responsible for watching bugs in the [Wizards] component area.

Comment 13 Prakash Rangaraj 2010-06-01 02:27:21 EDT

*** Bug 180311 has been marked as a duplicate of this bug. ***

Comment 14 Björn Kahlert 2011-06-17 20:52:31 EDT

I use Eclipse Classic release (Helios, up to date) on Mac OS X 10.6.8 and have the same problem.

Comment 15 Paul Webster 2013-02-11 15:01:43 EST

We need a new patch that doesn't break GTK+

http://wiki.eclipse.org/Platform_UI/How_to_Contribute

PW

Comment 16 Michal Kaczmarek 2013-08-23 09:21:26 EDT

This hasn't been fixed since 11 years!!! 

Is this: funny, ridiculous or pathetic?

I am solving this issue by copy-pasting entire WizardDialog class into my project and overriding method createDialogArea.

Cheers,
MK

Comment 17 Paul Webster 2013-08-23 09:29:05 EDT

(In reply to comment #16)
> Is this: funny, ridiculous or pathetic?

See above ... it's waiting for a contribution.

PW

Comment 18 Robin Stocker 2013-09-20 15:58:41 EDT

(In reply to Paul Webster from comment #17)
> See above ... it's waiting for a contribution.

Proposed fix: https://git.eclipse.org/r/16661

Comment 19 Paul Webster 2013-09-24 14:21:23 EDT

Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=612a50f73640e54157cd7213f1e867c65713e638

Thanks Robin.

PW

Comment 20 Robin Stocker 2013-09-24 17:11:38 EDT

(In reply to Paul Webster from comment #19)
> Thanks Robin.

Thanks for the review.

@Michal: For the record, fixing this took about an hour, with testing and fiddling around a bit to try other things. It's pretty easy to get started, see here: https://wiki.eclipse.org/Platform_UI/How_to_Contribute

Comment 21 Michal Kaczmarek 2013-09-25 03:17:40 EDT

Thanks for that fix!

@Robin,
Yes I know, the fix is exactly the same with 'patch' in my project. Next time i'll try to contribute the solution. I was not really familiar with the process.

Cheers.

Comment 22 Dani Megert 2013-10-07 09:21:27 EDT

I reverted the fix since it caused severe bug 418683.

Comment 23 Paul Webster 2013-10-07 09:22:23 EDT

(In reply to Dani Megert from comment #22)
> I reverted the fix since it caused severe bug 418683.

So we need a progress monitor no matter what wizard.needsProgressMonitor() says?

PW

Comment 24 Dani Megert 2013-10-07 09:30:37 EDT

(In reply to Paul Webster from comment #23)
> (In reply to Dani Megert from comment #22)
> > I reverted the fix since it caused severe bug 418683.
> 
> So we need a progress monitor no matter what wizard.needsProgressMonitor()
> says?
> 
> PW

Yes, eventually it will be required when calling ModalContext#run, where the monitor cannot be 'null'. At this point it's probably safer that #getProgressMonitor() always returns a monitor as it did for years, rather than creating one only when we execute the operation.

Comment 25 Paul Webster 2013-10-07 09:35:03 EDT

OK, maybe the way to approach this is to use gridData.exclude = true when the progress monitor is visible==false and set exclude to false when visible==true.

PW

Comment 26 Robin Stocker 2013-10-07 09:59:00 EDT

(In reply to Paul Webster from comment #25)
> OK, maybe the way to approach this is to use gridData.exclude = true when
> the progress monitor is visible==false and set exclude to false when
> visible==true.

Maybe I don't have the full picture, but why not change WizardDialog#getProgressMonitor to this?:

protected IProgressMonitor getProgressMonitor() {
	if (progressMonitorPart != null)
		return progressMonitorPart;
	else
		return new NullProgressMonitor();
}

Comment 27 Dani Megert 2013-10-07 10:06:23 EDT

(In reply to Robin Stocker from comment #26)
> (In reply to Paul Webster from comment #25)
> > OK, maybe the way to approach this is to use gridData.exclude = true when
> > the progress monitor is visible==false and set exclude to false when
> > visible==true.
> 
> Maybe I don't have the full picture, but why not change
> WizardDialog#getProgressMonitor to this?:
> 
> protected IProgressMonitor getProgressMonitor() {
> 	if (progressMonitorPart != null)
> 		return progressMonitorPart;
> 	else
> 		return new NullProgressMonitor();
> }

The main reason is that this could break subclasses who (wrongly) assumed that they get a valid (and so far visible) progress monitor. A second reason is, that it would look a bit ugly, given the Javadoc:
	 * @return the progress monitor, or <code>null</code> if this wizard
	 *         dialog does not have one

Comment 28 Robin Stocker 2013-10-07 12:27:38 EDT

(In reply to Dani Megert from comment #27)
> The main reason is that this could break subclasses who (wrongly) assumed
> that they get a valid (and so far visible) progress monitor.

Well, that would also be the case for Paul's proposed fix in comment 25. Or should we make it visible the first time someone calls getProgressMonitor?

> A second reason
> is, that it would look a bit ugly, given the Javadoc:
> 	 * @return the progress monitor, or <code>null</code> if this wizard
> 	 *         dialog does not have one

Yes, that would have to be changed. But as can be seen from the problem which caused the revert, callers already relied on the method never returning null.

Comment 29 Paul Webster 2013-10-07 12:48:23 EDT

(In reply to Robin Stocker from comment #28)
> (In reply to Dani Megert from comment #27)
> > The main reason is that this could break subclasses who (wrongly) assumed
> > that they get a valid (and so far visible) progress monitor.
> 
> Well, that would also be the case for Paul's proposed fix in comment 25. Or
> should we make it visible the first time someone calls getProgressMonitor?

using gridData.exclude it would be available just not visible.  There is code in specific places already to set the progress monitor to visible:true and to set it back to visible:false.  I was proposing we add the gridData.exclude changes in those locations, but really we should just need something like:

if (!wizard.needsProgressMonitor()) {
    gridData.exclude = true;
}

PW

Comment 30 Dani Megert 2013-10-08 03:59:58 EDT

(In reply to Robin Stocker from comment #28)
> > A second reason
> > is, that it would look a bit ugly, given the Javadoc:
> > 	 * @return the progress monitor, or <code>null</code> if this wizard
> > 	 *         dialog does not have one
> 
> Yes, that would have to be changed. 

I guess you already know my answer to this ;-). Changing this contract after the fact is a no go as because this would break subclasses that currently return 'null'.

Comment 31 Paul Webster 2013-10-08 10:12:32 EDT

I've pushed up the fix suggested in comment #29 which seems to remove the progress monitor from the new workingset wizard without causing the same NPE problem.

PW

Comment 32 Paul Webster 2013-10-08 10:12:42 EDT

Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=045aaed115c36464369b61197b8d619970c106f9

PW

Comment 33 Dani Megert 2013-10-08 11:05:24 EDT

The fix looks good.

Comment 34 Paul Webster 2013-10-08 11:11:31 EDT

(In reply to Robin Stocker from comment #18)
> 
> Proposed fix: https://git.eclipse.org/r/16661

Thanks for your fix, Robin.

PW

Comment 35 Robin Stocker 2013-10-09 18:26:05 EDT

I verified that the bug in 418683 no longer occurs. And this bug is also fixed with this.

(In reply to Dani Megert from comment #30)
> I guess you already know my answer to this ;-). Changing this contract after
> the fact is a no go as because this would break subclasses that currently
> return 'null'.

You're right, I didn't consider that it is protected.

Comment 36 Paul Webster 2013-10-29 14:16:05 EDT

In 4.4.0.I20131028-2000

PW

Comment 37 Teo Velicu 2014-01-31 04:12:17 EST

The simplest fix for this issue that works with eclipse 3.x also is:

class CustomWizardDialog extends WizardDialog {

    public CustomWizardDialog(Shell parentShell, IWizard newWizard) {
	super(parentShell, newWizard);
    }
		
    @Override
    protected ProgressMonitorPart createProgressMonitorPart(
				Composite composite, GridLayout pmlayout) {
	return super.createProgressMonitorPart(
              new Composite(getParentShell(), SWT.NONE), pmlayout);
    }
}