Dirty widgets not updated when changing source model

[GoogleCodeExporter]

See attachment. This is an Eclipse project created with the Google Plugin. I 
hope that's OK - it's the quickest way for me to make a demo of the bug. Fire 
it up and browse to index.html and it will run some tests. Most of these are to 
clarify my understanding. But the final test fails. I've added comments on what 
the test is trying to achieve and why I think it fails.

Specifically, if you have a form field which is formatted (in this case using 
the IntegerFormatter) and you type text into a text box which cannot be parsed 
by the formatter, then you get into a state where the widget and the model are 
out of sync. This is understandable. However, if you then go on to change the 
source model (POJO) and the new model and the old model happen to have the same 
value for the field which is bound to the formatted widget, then the widget 
doesn't get updated with the new model value because the change event isn't 
fired because the model value hasn't changed - and the widget stays out of sync.

The explanation is a bit unclear, but take a look at the failing test and the 
comments in the source.

Cheers!

Original issue reported on code.google.com by mail4da...@gmail.com on 11 May 2011 at 11:08

Attachments:

• pectin-issue.tgz

[GoogleCodeExporter]

Oh, and...

The archive includes a version of Pectin built against GWT 2.3. It's HEAD from 
a few days ago, not the actual 0.8 release. 

And revert() has the same problem. I haven't looked at the code because it's 
now past midnight here, but I imagine it's pretty much the same issue. 

Original comment by mail4da...@gmail.com on 11 May 2011 at 11:27

[GoogleCodeExporter]

I stumbled across the same problem yesterday. Haven't been able to check it in 
detail yet, but my impression is, that the formatted field - once in a corrupt 
state - doesn't get refreshed at all, regardless of the new value.

Original comment by RAlfoeldi on 12 May 2011 at 8:57

[GoogleCodeExporter]

So the fix is quite simple. In 
BeanPropertyValueModel.DefaultUpdateStrategy.setValueInternal() remove the 
first argument (oldValue) to the call to fireValueChangeEvent(). This means the 
event will always fire - not just when it thinks the model is changed.

Original comment by mail4da...@gmail.com on 13 May 2011 at 7:39

[GoogleCodeExporter]

Hi guys, sorry for the lack of comment but I haven't been able to look at 
pectin in quite a while (and not sure when I'll be able to in the future).  To 
compound it my local copy of gradle has died... I'd upgrade to the latest but 
that would involve migrating the plugin... which will be more than a couple of 
hours.  

This bug could also be fixed by ensuring that the onSourceModelChanged() -> 
readFromSource() sequence always fires events (which it should).  I.e. 
DefaultUpdateStrategy.setValueInternal could be overridden with a 
setValueInternal(T value, boolean forceValueChangeEvent) version which is 
called when the source model changes.

I tend to be slightly cautious about always firing events... can't think of any 
concequences off the top of my head, more of a gut feel thing (always means 
always (c;).  Having said that I did make ValueHolder always fire events for a 
similar reason (calling setValue twice with the same bean instance has the same 
issue).

Since I've been unable to spend any time on pectin (it requires quite a lot of 
brain cycles) would moving it to a DVCS be of any help in creating patched 
versions?

Cheers
Andrew

Original comment by andrew.pietsch on 13 May 2011 at 11:56

• Changed state: Accepted

[GoogleCodeExporter]

Ideally, there would be a way to only fire events when using a formatted field 
(since I suspect this issue only occurs when the model and widget get out of 
sync due to parse failures), but I don't understand the object relationships 
well enough yet. 

Oddly, I was contemplating forking to GitHub the other day, due to low 
activity! :-)

Personally, the most useful thing right now would be to get the build working 
with Gradle 0.9, and shipping a GWT >= 2.2 version (you just need to update the 
dependency version and re-build)

Original comment by mail4da...@gmail.com on 14 May 2011 at 5:56

[GoogleCodeExporter]

I've finally released 0.8.1 that has been compiled against GWT 2.2

Cheers

Original comment by andrew.pietsch on 30 Jun 2011 at 2:19

[GoogleCodeExporter]

Thanks Andrew. Does it fix this bug, or is it just a re-build of 0.8.0?

Original comment by mail4da...@gmail.com on 30 Jun 2011 at 8:39

[GoogleCodeExporter]

No, it's just a rebuild of 0.8 against GWT 2.2.  

I'm still thinking about what to do with the changes in my local repo that 
would have been 0.9.  If I decide to put those up I'll see if there's a quick 
fix for the issue that I can squeeze in.

Original comment by andrew.pietsch on 30 Jun 2011 at 11:17

[GoogleCodeExporter]

I'm interested in your local repo changes! :-)

Original comment by mathias....@gmail.com on 12 Jul 2011 at 8:57

[GoogleCodeExporter]

Ok I'll look at putting them up.  They're in two different projects at the 
moment so I'll have to merge them.  They're also more proof of concept at this 
stage so there's lots of things unfinished.

Cheers

Original comment by andrew.pietsch on 12 Jul 2011 at 10:10

[GoogleCodeExporter]

This is quite an old issue, but I just ran into this myself with Pectin 0.8_1 
and got it working with variations on the fixes suggested above.

I replaced the BeanPropertyValueModel class with a modified version using GWT 
super-source.  I went with the change suggested by Andrew, overloading the 
BeanPropertyValueModel$DefaultUpdateStrategy.setValueInternal() method with a 
version that can force the value change event.

      protected void setValueInternal(T value)
      {
          setValueInternal(value, false);
      }

      protected void setValueInternal(T value, boolean forceValueChangeEvent)
      {
         T oldValue = currentValue;
         currentValue = value;
         if (forceValueChangeEvent)
             fireValueChangeEvent(value);
         else
             fireValueChangeEvent(oldValue, value);
         afterMutate();
      }

Then I added the force flag in the readFromSource() method and also in the 
revertToCheckpoint() method (to handle the model.revert() case).

      public void readFromSource()
      {
         checkpointValue = (T) propertyDescriptor.readProperty(source.getValue());
         updateMutableState();
         setValueInternal(checkpointValue, true);
      }

      public void revertToCheckpoint()
      {
         setValueInternal(checkpointValue, true);
      }

That appears to have done the trick.

Original comment by timothy....@gmail.com on 28 Aug 2012 at 9:22