Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 360513 - [Preferences] IPreferencePage.performCancel() does not stop PreferenceDialog.cancelPressed() if it returns false
Summary: [Preferences] IPreferencePage.performCancel() does not stop PreferenceDialog....
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.7.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.8 M4   Edit
Assignee: Oleg Besedin CLA
QA Contact: Oleg Besedin CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-11 04:30 EDT by True Soft CLA
Modified: 2011-11-07 16:34 EST (History)
4 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description True Soft CLA 2011-10-11 04:30:00 EDT 
Build Identifier: 20110916-0149

The code of PreferenceDialog.cancelPressed() is the following:

	protected void cancelPressed() {
		// Inform all pages that we are cancelling
		Iterator nodes = preferenceManager.getElements(PreferenceManager.PRE_ORDER).iterator();
		while (nodes.hasNext()) {
			final IPreferenceNode node = (IPreferenceNode) nodes.next();
			if (getPage(node) != null) {
				SafeRunnable.run(new SafeRunnable() {
					public void run() {
						if (!getPage(node).performCancel()) {
							return;
						}
					}
				});
			}
		}
		
		// Give subclasses the choice to save the state of the preference pages if needed
		handleSave();

		setReturnCode(CANCEL);
		close();
	}


Notice that if getPage(node).performCancel() nothing will happen, and it will continue with handleSave(), setReturnCode(CANCEL) and close() the dialog.

My question is: If the page returns false in performCancel(), should the dialog not close? because it closes anyway.

Reproducible: Always
Comment 1 True Soft CLA 2011-10-11 04:31:19 EDT 
I wanted to say 

Notice that if getPage(node).performCancel() RETURNS FALSE nothing will happen, and it will
continue with handleSave(), setReturnCode(CANCEL) and close() the dialog.
Comment 2 Curtis Windatt CLA 2011-10-11 09:33:17 EDT 
Moving to platform UI. 

This may be intentional.
Comment 3 Remy Suen CLA 2011-10-11 16:26:02 EDT 
This code definitely looks wrong and should be fixed. The safety net was originally inserted to fix bug 29107.
Comment 4 Eric Moffatt CLA 2011-10-12 09:04:53 EDT 
Do you think we should restore the original intent (which was likely a return from 'cancelPressed') ? This code has been there since 2005.

True, do you know of any situation where this is causing an actual issue or did the code's oddness just pique your interest ?
Comment 5 Remy Suen CLA 2011-10-12 11:06:10 EDT 
(In reply to comment #4)
> Do you think we should restore the original intent (which was likely a return
> from 'cancelPressed') ?

Yes, the code needs to be fixed because the current implementation goes against the API. The user cannot abort the cancel operation by returning 'false'.

/**
 * Notifies that the container of this preference page has been canceled.
 * 
 * @return <code>false</code> to abort the container's cancel 
 *  procedure and <code>true</code> to allow the cancel to happen
 */
Comment 6 True Soft CLA 2011-10-12 12:37:34 EDT 
(In reply to comment #4)
> True, do you know of any situation where this is causing an actual issue or did
> the code's oddness just pique your interest ?

I was actually doing a Dialog similar to PreferenceDialog, and I noticed that it didn't work like the javadoc specifies.
Comment 7 Eric Moffatt CLA 2011-10-18 16:25:15 EDT 
(In reply to comment #5)
> 
> Yes, the code needs to be fixed because the current implementation goes against
> the API. The user cannot abort the cancel operation by returning 'false'.
> 
> /**
>  * Notifies that the container of this preference page has been canceled.
>  * 
>  * @return <code>false</code> to abort the container's cancel 
>  *  procedure and <code>true</code> to allow the cancel to happen
>  */

ok, I'll buy that.