Community
Participate
Working Groups
Using a validator on a TextCellEditor and entering invalid text into a cell results in ICellModifier.modify() being called with a null value, rather than an error being reported and the value being discarded. TableEditorImpl.saveEditorValue() *does actually check for editor validity*, but in the case of invalid values does nothing but no-op with comment "///Do what ???" and then calls cellEditor.getValue() which, for TextCellEditor's anyway, always returns null if its validator has invalidated the current value.
Created attachment 50927 [details] Testcase: ExampleViewer.java It seems that when I return an non-empty error string from ICellEditorValidator.isValid(), two things go wrong: 1. The error string is not being displayed as suggested by the JavaDoc for ICellEditorValidator.isValid(). 2. ICellModifer.modify() is being called with a null value which messes up my model object. I have attached a testcase that ilustrates the problem.
We should also look at the new in 3.3 EditingSupport for the same defect (or missing functionality?).
(In reply to comment #2) > We should also look at the new in 3.3 EditingSupport for the same defect (or > missing functionality?). > 3.3 EditingSupport has the same missing functionality.
Tom, this looks like it should go to you. As usual, punt it back if you think otherwise...
no time for 3.4
removing 3.5 because we are in RC-Mode
Created attachment 155469 [details] One possible fix. I'm sure there's room for improvement in this patch, but I thought it would be a useful conversation starter. I'm not certain if a ToolTip would be a reasonable way to display the error message, or if I'm leaking resources by not explicitly disposing of the ToolTip.
Created attachment 155470 [details] Minor patch to snippet 26 used to test my patch
Created attachment 155947 [details] Second possible fix Made a couple updates. First, timerExec to dispose of the created ToolTip. Still open to hearing other ideas for communicating the invalid state to the user. Second, modified TextCellEditor to avoid deactivating the editor if the contents was invalid. I'm not sure what the schedule for 3.5.2, but I was hoping to get *something* integrated in time for that release. I wouldn't mind scrapping the notification entirely, and just applying the change to make sure saveEditorValue isn't called if the contents is invalid, and exploring notification for the 3.6 release train. Anyone with commit access available to review/comment on this patch?
My apologies for not doing anything about this for so long. Calling ICellModifier.modify with a null value seems really wrong. (In reply to comment #9) > I'm not sure what the schedule for 3.5.2, but I was hoping to get *something* > integrated in time for that release. There is virtually no time left for 3.5.2 unfortunately, we are producing our first release candidate for 3.5.2 tomorrow morning. See http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_5_2.php We could try to escalate this and perhaps get a fix into 3.5.2 RC2, but for that it would be important to prove that there is no way to work around this in client code, and that existing workarounds (that likely exist) would not be affected by the fix. Have you looked at how this infrastructure is currently used in the SDK, in PropertySheetEntry.java?
Created attachment 156058 [details] Absolute minimum patch. No notification, but doesn't attempt to apply an invalid value to the editor. Here's another revision of the patch, this one as small as possible. There's no UI notification, but if the cell content isn't valid, we don't call saveEditorValue.
multi change because of intenion of stepping back as platform-ui committer
Still nothing happend with this bug? The problem is, that now as I implement a workaround in all my plugins for the editing support to display messages itself, then when this bug gets fixed it will display two messages.
Coming in the discussion as I face the same problem here with 3.6.1. I was trying a workaround which fires a RuntimeException from the ICellEditorValidator.isValid() method to avoid the modify call. Worse than expected : The validation is not safe for RuntimeException : the exception is correctly thrown to platform error handlers but in this case the modifier is still called with the INVALIDATED value ! This is bad : * Invalid values are passed as null to the modifier * Exception in the validation causes the erroneous data to be passed to the modifier Because you were fearing regressions with people who already work around this problem I was thinking that fixing the RuntimeException problem could be a good alternative : * Properly catch runtime exceptions * Avoid modify calls on exceptions * Maybe plug the tooltip display on exceptions With this approach, people with "exception safe" validators will work as before. Others willing to ensure no modify() calls could throw exceptions. This would require to be able to adjust validation strategy, because validation is called on every key pressed in the editor, which may not be good when dealing with exceptions. I know this is still not clean to work with runtime exception, but maybe it could provide some workaround of existing exception while providing a way of solving this problem. Hope this helps. Christophe
So will the issue be fixed in 3.7 ?
Created attachment 191754 [details] Updated "Absolute minimum patch" Updated "Absolute minimum patch" with current code changes and copyright header.
Patch "Updated Absolute minimum patch" applied to CVS Head. Thanks Tony!
On the subject of displaying the error message: The Javadoc on ICellEditorValidator#isValid() says: "..with the result being the error message to display to the end user." It is confusing and better wording is welcome, but it does not say that the error message will be displayed by the framework. Actually, the only place in the SDK where the error message is displayed is in the Properties view - the error goes on the status line. It is not the only way, tooltips are mentioned above and you can imagine a message box or a status field being used as well. It is up to the implementer to decide which way to display the error message; the framework only provides a way to pass it around. For the ExampleViewer the following code can be used to display the error message in the status line: ICellEditorListener cellEditorListener = new ICellEditorListener() { public void editorValueChanged(boolean oldValidState, boolean newValidState) { if (newValidState) setErrorText(null); else setErrorText(lastNameEditor.getErrorMessage()); } public void cancelEditor() { setErrorText(null); } private void setErrorText(String text) { IStatusLineManager lineManager = getViewSite().getActionBars().getStatusLineManager(); lineManager.setErrorMessage(text); } public void applyEditorValue() { setErrorText(null); // optional - do you want the error message to stay after editor is closed? } }; lastNameEditor.addListener(cellEditorListener);
Verified in I20110425-1800.
