Bugzilla – Full Text Bug Listing
| Summary: | [Preferences] IPreferencePage#performOk() returning false should prevent the dialog from closing | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Genady Beryozkin <eclipse> |
| Component: | UI | Assignee: | Tod Creasey <Tod_Creasey> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | cebarne2, christof_marti, eckart.langhuth, jamie.beznoski, JWootton, martinae |
| Version: | 3.0 | ||
| Target Milestone: | 3.1 M5 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
|
Description
Genady Beryozkin
The purpose of this method is to an abort a save - so it is called after OK is pressed which means disabling OK would have no effect. You should implement isValid to disable the OK button. There is a problem with it. Suppose some pages were closed ok, then you have a page that returns "false" in its "performOK" method, and after that you have pages with valid values. If the save is aborted, some values (of the last valid pages) are discarded and the user does not have a chance to commit them. If you suggest relying on "isValid()" then I believe performOK should return "void". If it is not possible without breaking an API, maybe it is possible to use "canPerformOk" method that will ask all pages regarding their state. I have a serious problem with the "isValid" method, because I it requires me to have a listener for every field in the preference page that will call setValid(). isValid() values are cached in some way, and when I tried to override it, the method was not called after the page contents has changed. performOK usually only returns false in cases that cannot be determined during field entry (i.e. errors). We can't do an isValid check for you as we are not sure how you are going to structure your page - this is one of the problems with a framework - we have to leave some functionality to the developer as we can't make potentially bad decisions for you. Could you use a FieldEditorsPreferencePage? I mean that the availability of the "OK" button is determined by a call to #setValid() method, which must be called explicitly by the developer when one of the fields changes. Is it possible for eclipse preferences code to call #isValid() each time something changes on the page? Consider for example how IWizard#performFinish() is documented. If I get it right, if the method returns false, the wizard is not closed. Sadly it isn't because there is no way for me to know what has changed unless I crawl the entire widget hierarchy and add selection and key listeners to everything that it is able to take them - the issue is that the preference page is very open ended and so there is little knowledge we have about them. Why has this changed since 2.1.x? I had a set of properties pages working great in 2.1.x, and now they always close when PerformOk() returns false. I do not want to set isValid on every field modification. I would rather check for errors once when performOk() is called. Doing it any other way seriously degrades performance of the UI. I don;t think this bug should be resolved yet. We haven't made any changes in this behaviour since 2.1 to the best of my knowledge. If you think there is a regression please log a report with some steps. Here's is a way to duplicate this behavior.
1) Create a new Plug-in project w/ Property Page wizard
2) Replace the appropriate property page code with following methods:
protected Control createContents(Composite parent) {
Composite composite = new Composite(parent, SWT.NONE);
GridLayout layout = new GridLayout();
composite.setLayout(layout);
GridData data = new GridData(GridData.FILL);
data.grabExcessHorizontalSpace = true;
composite.setLayoutData(data);
text = new Text(composite, SWT.BORDER);
text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL));
return composite;
}
public boolean performOk() {
// store the value in the owner text field
return (text.getCharCount() > 0);
}
Now, in Eclipse 2.1.3 if I leave the text field blank and press OK, the
property page stays open. If I put text in the field, the property page will
close when I press OK.
In Eclipse 3.0.1, if I press OK with the field empty, the property page closes
(even though I am returning false).
This shows a definite change from the behavior of 2.x.
One person I have been discussing with in the community had mentioned he
tracked the problem to PreferenceDialog, and that the close() reference was
inside a finally block. Perhaps this is why it ALWAYS closes in 3.0.1.
Here is the change I made to jface to get this to work like it did in 2.1.3. It seems 3.0.1 introduced a try-catch-finally block into this method and the close() reference got put into the finally block (where it is always called regardless of a previous return). In 2.1.3 close() was not called if the previous return was reached. Index: PreferenceDialog.java =================================================================== RCS file: h:/cvsrepo/dev/org.eclipse.jface/src- jface/org/eclipse/jface/preference/PreferenceDialog.java,v retrieving revision 1.1 diff -u -r1.1 PreferenceDialog.java --- PreferenceDialog.java 1 Nov 2004 18:38:56 -0000 1.1 +++ PreferenceDialog.java 1 Nov 2004 18:39:57 -0000 @@ -755,8 +755,9 @@ if (!errorOccurred) handleSave(); // Need to restore state - close(); + } + close(); } /* * (non-Javadoc) Reopening to investigate the regression I think this is a better patch. In the previous, I wasn't re-enabling the OK
button if "return;" was being reached.
Index: PreferenceDialog.java
===================================================================
RCS file: h:/cvsrepo/dev/org.eclipse.jface/src-
jface/org/eclipse/jface/preference/PreferenceDialog.java,v
retrieving revision 1.1
diff -u -r1.1 PreferenceDialog.java
--- PreferenceDialog.java 1 Nov 2004 18:38:56 -0000 1.1
+++ PreferenceDialog.java 1 Nov 2004 19:41:58 -0000
@@ -743,8 +743,9 @@
IPreferenceNode node =
(IPreferenceNode) nodes.next();
IPreferencePage page =
node.getPage();
if (page != null) {
- if (!page.performOk())
- return;
+ if (!page.performOk()){
+ getButton
(IDialogConstants.OK_ID).setEnabled(true);
+ return;}
}
}
} catch (Exception e) {
@@ -755,8 +756,9 @@
if (!errorOccurred)
handleSave();
// Need to restore state
- close();
}
+ close();
+
}
/*
* (non-Javadoc)
*** Bug 77652 has been marked as a duplicate of this bug. *** I came across the same problem while writing an interface that allows implementers to contribute part of a preference page. A problem the patch from comment 11 does not solve is that after one preference page returned false, there are some preference pages that were allowed to save their state, one that failed to save its state and some preference pages that were never asked to save their state. As stated earlier, a possible solution might be to add a 'boolean canPerformOk()' to determine whether all preference pages will successfully (to their current knowledge) performOk(). If at least one canPerformOk() returns false no additional action would be taken. The return value of performOk() would only be false in exceptional cases and PreferenceDialog would continue to call performOk() on each page such that as much as possible of the preferences are stored, the dialog would be kept open and a message box could inform about which pages failed to performOk(). This or something similar would make sense even in the absence of canPerformOk() in order to avoid inconsistently saved preferences to some degree and to inform the user about what went wrong. I agree this has a compatibility issue, but currently clients are well adviced to always return true on performOk() anyway. Bug 65134 is probably a duplicate. Changing the Java compiler settings shows a dialog on OK. When pressing Cancel on that dialog, the preference changes are lost because the preference dialog closes. *** Bug 65134 has been marked as a duplicate of this bug. *** Fixed in build >20040105. We now close if all of the performOKs are true and if there are no errors. We stop calling performOK if one of them returns false as we are not closing the dialog now. *** Bug 81644 has been marked as a duplicate of this bug. *** Verified in 20050215 |