Bug 495789

Comment 1 Lars Vogel 2016-06-09 10:31:35 EDT

Simon, please review.

Comment 2 Mickael Istria 2016-09-07 05:09:14 EDT

@Conrad: can you please have a look at this issue, related to a change you've made?

Comment 3 Conrad Groth 2016-09-08 16:50:00 EDT

(In reply to Simon Moosbrugger from comment #0)
> When binding validation results to a wizard page with the utility class
> org.eclipse.jface.databinding.wizard.WizardPageSupport you rely on an
> IConverter. If the IConverter throws a exception during conversion a error
> markers can be placed on the wizard page. 
> 
> Through the change https://git.eclipse.org/r/#/c/63294/5
> (org.eclipse.core.databinding.UpdateStrategy.convert(Object):717) this
> functionality has been altered. The exception is just logged and not thrown
> anymore as before. Therefore when the conversion fails there are no error
> markers placed on the wizard page.
> 
> As a solution I would suggest to just rethrow the caught exception. I have
> already created a change:
> 
> https://git.eclipse.org/r/74973

In case of ValueBindings there are several validators, that validate the value in different stages of the binding (e.g. after get, after convert, before set). These validators are responsible for the validation status of a binding.

For SetBinding and ListBinding an exception from the convert method causes the abortion of the update method, which means, that some differences are not applied to the model set or list. This was the reason for my change 63294.

But you are right, no conversion exception comes out of the Binding anymore. Do you know a wizard page in Eclipse, that has a problem now? Or do you have some other sample code, that has a problem now?

If yes, I would suggest to rethrow the exception in the convert method, and ALSO catching that again in the several update methods, to set the ERROR status.

Comment 4 Andreas Buchen 2017-01-21 04:16:52 EST

+1 on the proposed change

(In reply to Conrad Groth from comment #3)
> But you are right, no conversion exception comes out of the Binding anymore.
> Do you know a wizard page in Eclipse, that has a problem now? Or do you have
> some other sample code, that has a problem now?

I don't know about Eclipse, but my e4 application breaks as well. There are no error markers generated for conversion errors anymore.

The effect is very easy to see if you slightly modify the Snippet014 - no message

https://github.com/buchen/eclipse.platform.ui/blob/patch-1/examples/org.eclipse.jface.examples.databinding/src/org/eclipse/jface/examples/databinding/snippets/Snippet014WizardDialog.java#L78-L104

Comment 5 Lars Vogel 2017-02-09 03:50:12 EST

Simon, can you please do the review here?

Comment 6 Lars Vogel 2017-03-03 03:48:50 EST

Mass move. Please move back to M6, if necessary

Comment 7 Dani Megert 2017-05-12 10:31:39 EDT

Please set a target milestone again when you plan to fix the bug.

Comment 8 Jens Lideström 2019-09-24 15:23:56 EDT

Lars Vogel wrote (in Gerrit):

> Jens, please still have a look and create a new change on behave of Simon, if the change is good and still relevant.

I think Simons' old change is good. I will re-create it.

It is a bit weird for clients to have the behaviour of exceptions first change, then change back a few years later. But apart from that I think the behaviour was better the way it was to start with, when exceptions from converter were re-thrown. So I'm fine with restoring that behaviour.

Comment 9 Andreas Buchen 2019-09-25 01:38:48 EDT

> I think Simons' old change is good. I will re-create it.

+1

Comment 10 Dani Megert 2019-10-10 09:38:07 EDT

Removed milestone as no one reacted to https://www.eclipse.org/lists/eclipse-dev/msg11217.html.

Comment 11 Jens Lideström 2019-10-12 14:12:32 EDT

This problem is worse than described earlier, because if the converter throws then the original, unconverted value is returned!

public D convert(S value) {
  if (converter != null) {
    try {
      return converter.convert(value);
    } catch (Exception ex) {
      Policy.getLog().log(new Status(IStatus.ERROR, Activator.PLUGIN_ID, ex.getMessage(), ex));
    }
  }
  return (D) value;
}

I will push a fix for this shortly.

Comment 12 Eclipse Genie 2019-10-12 15:13:05 EDT

New Gerrit change created: https://git.eclipse.org/r/150984

Comment 13 Eclipse Genie 2019-10-12 15:13:08 EDT

New Gerrit change created: https://git.eclipse.org/r/150983

Comment 14 Jens Lideström 2019-10-13 04:00:59 EDT

I have submitted one change to rethrow the exception in convert, and another change to catch exceptions in ListBinding and SetBinding.

But I'm thinking about change the approach:

It might be better to remove the catch from the convert method altogether, and instead log all exceptions in the calling methods (doUpdate).

Comment 15 Jens Lideström 2019-10-13 05:24:11 EDT

(In reply to Jens Lideström from comment #14)
> But I'm thinking about change the approach:

Never mind. Let's consider a change of approach at another time.

I'll wait for feedback on these changes for a day or two, then I'll merge.

Comment 16 Eclipse Genie 2019-10-27 14:24:42 EDT

Gerrit change https://git.eclipse.org/r/150984 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3fea315ff0980ea91a5936eed57f26183e6bf039

Comment 17 Eclipse Genie 2019-10-27 14:24:46 EDT

Gerrit change https://git.eclipse.org/r/150983 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e83717f337323ce38d6d39bc143f714e320448fb