Bug 180746 - [DataBinding] TextObservableValue should take a delay-argument
Summary: [DataBinding] TextObservableValue should take a delay-argument
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.4 M4   Edit
Assignee: Boris Bokowski CLA
QA Contact:
URL:
Whiteboard:
Keywords: api, contributed
: 120406 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-04-03 12:08 EDT by Sebastian Fuchs CLA
Modified: 2008-05-15 17:33 EDT (History)
8 users (show)

See Also:


Attachments
Implemented a delay for fireValueChange() (7.37 KB, application/octet-stream)
2007-09-04 06:17 EDT, Sebastian Fuchs CLA
spacehorst: review?
Details
demo application demonstrating the difference between event propagation delay and standard behaviour (7.25 KB, application/octet-stream)
2007-09-05 06:23 EDT, Sebastian Fuchs CLA
no flags Details
SWTObservables.delayValueChanges() patch (11.63 KB, patch)
2007-10-30 20:10 EDT, Matthew Hall CLA
no flags Details | Diff
Snippet015DelayTextModifyEvents snippet demonstrating usage of SWTObservables.delayValueChanges (2.19 KB, text/plain)
2007-10-30 20:16 EDT, Matthew Hall CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Fuchs CLA 2007-04-03 12:08:54 EDT
Sometimes SWT.Modify is needed as update event type but the performance (number of propagated update-events) should be similar to SWT.FocusOut.
Therefore I suggest to create an additional constructor with a parameter
int delayInMilliseconds
which defines a delay between the time the last SWT.Modify-Event occured and fireValueChanging() is called.
This will allow the customer to enter multiple characters without starting a validation each time.
A further advance is when binding the validation result to a label or similar, the label won't change confusingly while you still enter your text.

The current structure of TextObservableValue doesn't provide a way to achieve the described behaviour by subclassing without reimplementation.

Maybe this behaviour can even be useful whit SWT.FocusOut ? 

More information:
Comment 1 Sebastian Fuchs CLA 2007-09-04 06:17:06 EDT
Created attachment 77634 [details]
Implemented a delay for fireValueChange()

Please have a look at the ettachement.
I implemented a delay for fireValueChange().
It works but has some threading issues I need help with.

1. I assumed that fireValueChange() should be run in the same Thread as the Observable was created. Since we are an ISWTObservableValue, I wrapped the call in a Display#asyncExec(). Are there other scenarios where we are not in the UI-Thread ?

2. Because I introduced a delay / thread, I cannot make sure that when fireValueChange() is called, clients find their context in a consistent state -
e.g. widgets could be disposed.
Any suggestions not to saddle clients with that inconsistent state ?

3. Clients must be aware that the model state does not reflect the UI state while the delay is active. This is similar to using SWT.FOCUS_OUT while the cursor is still in a modified text field.
Comment 2 Boris Bokowski CLA 2007-09-04 10:09:25 EDT
Have a look at Display.timerExec(), this should allow you implement this without using a Thread.  Can you also try to write this as a patch to the existing class, and without using Java 5 features?

A snippet that demonstrates how this is supposed to be used would be good too.
Comment 3 Sebastian Fuchs CLA 2007-09-05 06:23:21 EDT
Created attachment 77704 [details]
demo application demonstrating the difference between event propagation delay and standard behaviour

Boris,
thank you for your hints.
I am going to rework the class and provide a patch.

Meanwhile one can test class from comment# 2 by this demo application.
It shows the number of propagated events for a delayed and a non-delayed text field.
In case that validation, converting, propagate model changes and displaying validation results are expensiv, performance could be increased by reducing the number of propagations.
Comment 4 Matthew Hall CLA 2007-10-18 11:50:51 EDT
You may want to consider two approaches to delaying firing events:
* minimum interval between event firings (as described above)
* minimum interval between last keystroke and event firing.

Here is a simplified version of my implementation:

class CancelableRunnable implements Runnable {
  boolean cancel = false;
  public void run() {
    if (cancel) return;
    fireValueChanging();
    if (runnable == this) runnable = null;
  }
}

CancelableRunnable runnable;

public void handleModify(ModifyEvent e) {
  if (runnable != null)
    runnable.cancel = true;
  display.timerExec(delay, runnable = new CancelableRunnable());
}

This approach worked well for me and drastically improved UI responsiveness in my own project.
Comment 5 Sebastian Fuchs CLA 2007-10-24 03:56:15 EDT
Matthew,
I hadn't time to try out your snippet, but I believe it's doing well.

Do you think that making it API (optional to the standard behaviour) makes sense ?
I think of theoretical issues which can occure to the client, during delay time: e.g. inconsistent model. probalby they never happen when using a fine tuned delay.
Comment 6 Matthew Hall CLA 2007-10-24 11:08:17 EDT
DataBinding has done an excellent job of keeping concerns modularized so you can combine the behaviors you need easily.  This has served to make your intentions very clear when you read the code.  

For this reason I believe it's better to wrap the observeText() result in another observable which would implement the delay:

dbc.bind(SWTObservables.delayValueChanges(400, SWTObservables.observeText(text, SWT.Modify)),
         BeansObservables.observeProperty(...),
         null,
         null);

This approach would also allow the delay to be applied in situations we haven't contemplated.
Comment 7 Benjamin Cabé CLA 2007-10-24 11:14:36 EDT
(In reply to comment #6)
Matthew, +1 for your approach ! I like it ! 

Comment 8 Matthew Hall CLA 2007-10-24 11:25:27 EDT
I'd be happy to implement this and donate the code, if somebody could explain the "staleness" concept to me.  This one aspect of DataBinding just eludes me.
Comment 9 Boris Bokowski CLA 2007-10-25 07:57:40 EDT
(In reply to comment #5)
> I think of theoretical issues which can occure to the client, during delay
> time: e.g. inconsistent model. probalby they never happen when using a fine
> tuned delay.

Well, it sounds like there is a real chance to miss updates if you blindly apply a delay.  Wouldn't you want to hook listeners for SWT.Modify *and* SWT.FocusOut in cases like this?
Comment 10 Boris Bokowski CLA 2007-10-25 09:25:54 EDT
(In reply to comment #8)
> I'd be happy to implement this and donate the code, if somebody could explain
> the "staleness" concept to me.  This one aspect of DataBinding just eludes me.

Here is an attempt to explain it: 

An observable may become stale when its implementation knows that the current value (or state) is out of date and will change soon. When we added this, we were thinking of cases where on the model side, you talked to a server or a database, but wanted to avoid blocking the UI while you ran a query. So for example, an observable list representing the children of a node in a tree could pretend that it is empty (or have one "Pending..." child) while it spawned a background thread to retrieve the real children of the parent node. It would do that by firing a stale event, and then firing a change event when the background thread has received the information. Firing the stale event enables UIs that tell the user about on-going background work; for example, values could be displayed in an italic font.

We haven't seen uses of this other than in our (few) examples. The reason we put it in was that this seemed to be something that you cannot add to the API at a later time, so we went ahead with the best guess we had at the time about how this could be exposed.

It seems to me that a general-purpose "delayed value" observable should fire a stale event at the time its underlying observable changes for real, and it should then become non-stale after the specified delay. The staleness part of observables is designed in a way that clients who don't care about staleness can just ignore the staleness event. Note that there is no "handleUnstale" - an observable that becomes unstale just fires a normal change event, and clients who care about staleness are expected to call isStale() to find out if an observable is no longer stale when they process the change event.

Now the interesting (open) question is whether staleness can be used to solve the problem from comment #9 in a general way. I don't see a simple solution, but maybe you do?
Comment 11 Sebastian Fuchs CLA 2007-10-25 17:09:25 EDT
(In reply to comment #6)

Great idea !
I like it.
Comment 12 Sebastian Fuchs CLA 2007-10-25 17:41:45 EDT
(In reply to comment #9)
> Well, it sounds like there is a real chance to miss updates if you blindly
> apply a delay.  Wouldn't you want to hook listeners for SWT.Modify *and*
> SWT.FocusOut in cases like this?

Boris, I hope I got you right:

For the current behaviour, there was mostly one reason to prefer SWT.Modify over SWT.FocusOut:
by FocusOut the user *must* leave the textfield to trigger an update. Under some circumstances this is not acceptable, e.g.:
binding the editor's dirty state to model changes or the case that the user saves the editor (CTRL-S) before leaving the last edited textfield.
So missing updates is even possible without a delay.

When introducing the delay, much more care must be taken not to trigger actions like editor save or disposing widgets during an active delay. (see comment#1).
To make the delay-API consistent we need some mechanism where clients can check or being notified, that no further delay is active.

Comment 13 Matthew Hall CLA 2007-10-25 17:52:51 EDT
IStaleListener seems a good candidate for that.
Comment 14 Matthew Hall CLA 2007-10-30 20:10:53 EDT
Created attachment 81676 [details]
SWTObservables.delayValueChanges() patch

To get around the concerns raised by Boris, it takes the observable's widget, tests whether it is a Control, and if so registers a SWT.FocusOut listener which forces any pending value changes to fire immediately.

I will attach a snippet patch next demonstrating usage.

This was actually a lot of fun to write. :)
Comment 15 Matthew Hall CLA 2007-10-30 20:16:43 EDT
Created attachment 81678 [details]
Snippet015DelayTextModifyEvents snippet demonstrating usage of SWTObservables.delayValueChanges
Comment 16 Boris Bokowski CLA 2007-10-30 21:57:15 EDT
Matthew, thanks for your contribution!

+  public Object getValueType() {
+    return observable.getValue();
+  }

Tests would be good... anyone?
Comment 17 Boris Bokowski CLA 2007-11-22 16:55:46 EST
Released to HEAD. Thanks a lot for the patch!

I extended the snippet to show visual feedback for pending changes, based on the staleness of the DelayedObservableValue. To get this to work, I had to change the implementation of isStale(). Matthew - could you have a look at the final class, double-checking that what I did was not stupid? Thanks!
Comment 18 Matthew Hall CLA 2007-11-23 00:11:09 EST
Looks good to me.  I like the new Observables.observeStale()--very nice.
Comment 19 Boris Bokowski CLA 2007-11-26 14:58:50 EST
I don't think this needs to be IP-reviewed: The actual patch - attachment 81676 [details] - had less than 250 lines of code (and a good part of it was copied from another existing EPL'ed class from the data binding framework), and the snippet - attachment 81678 [details] - is separate from the actual code contribution in that it is not being shipped as part of any download we offer.
Comment 20 Matthew Hall CLA 2007-11-26 15:04:30 EST
Boris, I'm willing to sign any agreement that will streamline this process for my contributions.  I'm a company officer so I have authority to make binding agreements on behalf of the company wrt Eclipse contributions.  Would this help?
Comment 21 Boris Bokowski CLA 2007-11-26 15:29:46 EST
(In reply to comment #20)
Matthew, as far as I know, all the Eclipse Foundation has is the distinction between a non-committer and a committer.  Agreements for non-committers don't show up in the picture, although I hope that we can vote you in as a committer soon.

At that time, you will be asked to sign an agreement which will streamline this process. ;-)
Comment 22 Boris Bokowski CLA 2007-12-11 11:06:26 EST
Verified using I20071211-0010 by running the snippet.
Comment 23 Matthew Hall CLA 2008-05-15 17:33:10 EDT
*** Bug 120406 has been marked as a duplicate of this bug. ***