Community
Participate
Working Groups
Quoting Stefan Xenos from bug 215531 comment #29: > If you're messing about with the known elements set, you may want to fix > something I broke when I wrote the first version. When I wrote the first cut, I > wasn't sure whether add events should be fired from the known elements set > before or after the element was added to the viewer. I decided arbitrarily and > fired the events before adding it to the viewer. I was wrong -- this prevents > clients from observing the known elements and using them to control -- say -- > the expansion state in a tree. > > That's a long winded way of saying the known elements set should always fire its > events AFTER the viewer is updated. Quoting myself from bug 215531 comment #32: > If the viewer initializes the labels for an element before the > element has change listeners added, then there is a small window of time during > which any changes to that element would not be seen by the label > provider--leaving stale labels in the viewer. I think the best option is to add > a constructor argument to each content provider specifying when the known > elements should be updated. The current behavior is safer for for asynchronous environments and will remain the default for the no-arg constructors.
> If the viewer initializes the labels for an element before the > element has change listeners added, then there is a small window of time during > which any changes to that element would not be seen by the label > provider--leaving stale labels in the viewer I'm not sure if I understand you correctly, but I don't think this could happen. The element change events would be fired in the SWT display thread/realm, and would be fired immediately after updating the viewer. Since nobody would be processing asyncExecs in the meantime, there would be no opportunity for any label provider to fire change events in the meantime. By running in the Display thread, we're essentially holding a lock on all UI entities. No matter what any thread did in the meantime, they would need to use an asyncExec in order to fire events from the label provider and therefore there would be no way for the label provider to change its state in the critical moments between when the element was added to the viewer and when the element set fired its event.
(In reply to comment #1) > I'm not sure if I understand you correctly, but I don't think this could > happen. The element change events would be fired in the SWT display > thread/realm, and would be fired immediately after updating the viewer. Since > nobody would be processing asyncExecs in the meantime, there would be no > opportunity for any label provider to fire change events in the meantime. By > running in the Display thread, we're essentially holding a lock on all UI > entities. > > No matter what any thread did in the meantime, they would need to use an > asyncExec in order to fire events from the label provider and therefore there > would be no way for the label provider to change its state in the critical > moments between when the element was added to the viewer and when the element > set fired its event. JDB does not require changes to model objects to originate from the SWT realm. They can originate anywhere, but they are only handled on the given realm. This is why JavaBeanObservable(Value|List|Set|Map) all fire change events within a Runnable passed to realm.exec(). So the following scenario is quite possible: Scenario: A table viewer with ObservableListContentProvider has an IObservableList obtained from BeansObservable.observeList(bean, "items"). 1. SWT thread: content provider retrieves contents of collection 2. Worker thread: An item is added to the bean's items property, bean fires property change event. However the JavaBeanObservableList has no listeners yet, so no list change event is fired. 3. SWT thread: content provider adds change listener to observable collection, which is a List typed property of a bean. Thus the intervening change from the worker thread is missed by the content provider. Now that I think about it, it is possible for the opposite problem to occur.. 1. SWT thread: content provider adds change listener to observable collection. 2. Worker thread: An item is added to the bean's items property, bean fires property change event. 3. Worker thread: JavaBeanObservableList receives property change event from bean, calls realm.exec() with Runnable that fires a list change event. This ensures that event is fired on the correct realm, no matter the originating thread. 4. SWT thread: content provider retrieves contents of list, including item added in (2). 5. SWT thread: Event pump executes runnable from (3), and according adds the item in (2), notwithstanding it was already in the observable list in (4). So the change is recognized twice. The viewer is now out of sync with the observable list. One way to get around this is to use a WritableList as the input, and use a list binding to keep it in sync with the JavaBeanObservableList. However I'm not familiar with ListBinding's internals, so this may be subject to the same set of problems.
(In reply to comment #2) > One way to get around this is to use a WritableList as the input, and use a > list binding to keep it in sync with the JavaBeanObservableList. However I'm > not familiar with ListBinding's internals, so this may be subject to the same > set of problems. It should work if the model-side observables are in a different realm, and the model-side changes happen from within that realm.
See the patch in bug 237359 for a possible solution - I added an additional method getDeferredKnownElements() to the content provider classes. I needed this for a similar reason - being able to set the checked state of elements, *after* they are added to the viewer.
Re: comment 2 These problems are well understood, and in fact they were the entire reason for the existence of the realms pattern. Basically, Realms fix this by making two guarantees: 1. An observable may not be accessed outside its realm. 2. An observer always fetches its initial state and hooks its listener in the same atomic operation. Both your examples demonstrate real concurrency bugs because they are breaking the Realm contracts. Your first example violates #1. This would only occur if the viewer was accessing an observable in another realm. In step 1, the viewer obtained its elements directly from an observable in a different realm. That is not permitted, and a correctly-written observable would throw an exception saying it was being accessed from the wrong realm. Your second example also violates #1 (by calling methods on an observable in a different realm in steps #1 and #4). > One way to get around this is to use a WritableList as the input, and > use a list binding to keep it in sync with the JavaBeanObservableList. Yes. This is the right way to do things. In fact, if the observables are working correctly, this is the only possible way to implement it since anything else would throw exceptions when you attempted to call observable getters in the wrong realm. This is how the observables protect programmers from accidentally introducing these not-so-obvious concurrency bugs in their code. By creating a binding between realms, you end up maintaining a separate copy of the list/set in each realm (the WritableSet). The one in the SWT realm is updated by asyncExecs whenever the original observable fires events. So. In both examples there should have been an extra observable being bound in the UI thread. In example 1, the event fired by step 2 would not have been propogated to the UI thread yet since the content provider was still running in the UI thread and it was not processing *syncExecs. After step 3 is finished, the syncExec in the binding would run, copy the observable state to the WritableList, and the event would be correctly observed and handled by the content provider. In example 2, the event would have been fired by the observable on step 3 but not yet processed by the WritableList in the SWT realm. That means that on step 4, the copy in the SWT realm *would not* contain the element added on step 2. That element would only be added when the event pump runs in step 5 - but that's okay, because then the element would only be added once. You should discuss this more with Boris. What you've described are exactly the use-cases that led to the development of the Realms pattern. The original bug report for the realms linked to some good material that covered these use-cases in depth.
(In reply to comment #4) > See the patch in bug 237359 for a possible solution - I added an additional > method getDeferredKnownElements() to the content provider classes. I needed > this for a similar reason - being able to set the checked state of elements, > *after* they are added to the viewer. It seems that we are going to a lot of trouble with the known elements set when a simple listener pattern would do the trick, e.g.: interface IViewerElementListener { public void beforeElementAdded(Object element); public void afterElementAdded(Object element); public void beforeElementRemoved(Object element); public void afterElementRemoved(Object element); }
> It seems that we are going to a lot of trouble with the known elements set > when a simple listener pattern would do the trick, e.g.: If we don't have an IObservableSet, then: - The user can't use the standard databinding tools for transforming it, computing its union, etc. - The user can't ask for the current set of known elements - The implementation can't make use of the standard databinding tools (check out Boris' use of deferred sets in the implementation of getDeferredKnownElements). I believe Boris' getDeferredKnownElements() will be both the most useful to clients and have the simplest implementation. My point was merely that we could simplify further by removing the non-deferred version getKnownElements().
How about getRealizedElements() instead of getDeferredKnownElements()? Let's spell out when and how each set is updated as elements are added / removed, just to make sure we're all on the same page: When an element is added: knownElements.add(newElement); viewerUpdater.add(newElement); realizedElements.add(newElement); When an element is removed, the updates are in reverse: realizedElements.remove(oldElement); viewerUpdater.remove(oldElement); knownElements.remove(oldElement); Thus realizedElements will always be updated while the element is realized in the viewer, and knownElements will always be updated while the element is *not* in the viewer.
Created attachment 106953 [details] Patch to JavaBeanObservableMap (In reply to comment #7) > I believe Boris' getDeferredKnownElements() will be both the most useful to > clients and have the simplest implementation. > > My point was merely that we could simplify further by removing the non-deferred > version getKnownElements(). We need both. The problem with changing the timing when the known elements set is updated is masked by a bug in JavaBeanObservableMap. See the test in the attached patch. Currently JavaBeanObservableMap will give the value of the observed property even if the bean passed to the get() method is not part of the known elements set yet. Thus when the item is realized in the viewer, ObservableMapLabelProvider is able to initialize the item properly regardless of whether the element is in the known elements set. We should not depend on this kind of behavior because clients are free to use other observable map implementations that do not share this bug.
I like the name getRealizedElements().
Created attachment 107002 [details] getRealizedElements prototype with demo of problem explained earlier To see the problem I described earlier, apply this patch and run Snippet013TableViewerEditing.java. Around line 228 I've modified the patch so there are two ways to set up the label provider: using getKnownElements() or using getRealizedElements(). With the JavaBeanObservableMap bug covered the labels now come up blank. This is because ObservableMapLabelProvider listens for changed entries, but not for added or removed entries, thus the labels are initially blank and stay that way even after the elements are added to the realized elements set. However changing any of the names in a cell editor will cause the changed value to show up in the viewer. A similar, but more complicated fix would need to be applied for ObservableCollectionTreeContentProvider and friends as well.
> Thus realizedElements will always be updated while the element is realized > in the viewer, and knownElements will always be updated while the element > is *not* in the viewer. Hmm. I agree with the addition semantics, but I'm not sure how I feel about the removal semantics. By saying that additions happen before updating the viewer and removals happen afterward (or visa-versa), it means that a single event that started as a SetDiff with both additions and removals would get broken into two events - one containing additions and the other containing removals. The events would be processed at different times, which may cause flicker in any widget that is reacting to them. On the other hand, if we had one set that always updates before the viewer and another set that always updates after then both sets could process any combination of additions and removals in the same atomic event. That is: When an element is added: knownElements.add(newElement); viewerUpdater.add(newElement); realizedElements.add(newElement); When an element is removed: knownElements.remove(oldElement); viewerUpdater.remove(oldElement); realizedElements.remove(oldElement); Or a combination of additions and removals: knownElements.fireEvent(diff); viewerUpdater.remove(diff.removals); viewerUpdater.add(diff.additions); realizedElements.fireEvent(diff);
(In reply to comment #12) > By saying that additions happen before updating the viewer and removals happen > afterward (or visa-versa), it means that a single event that started as a > SetDiff with both additions and removals would get broken into two events - one > containing additions and the other containing removals. The events would be > processed at different times, which may cause flicker in any widget that is > reacting to them. I assume you're talking about knownElements here. I agree that this set could have removals applied before or after the viewer update with no adverse effects to label providers (the intended consumer of this feature). However we have already released the API with javadoc specifically stating that additions happen in the set before they happen in the viewer, and removals happen in the set after they happen in the viewer. I am unsure if anybody depends on this behavior, but if the API usage assumption ("Every aspect of the API matters to some Client") is to be believed then I would rather leave it alone. > On the other hand, if we had one set that always updates before the viewer and > another set that always updates after then both sets could process any > combination of additions and removals in the same atomic event. I thought the purpose of the realized elements set is to be able to set the state of an element (e.g. checkboxes, selection) after it has been added to the viewer, or query the state of an element (e.g. remembering checked/selected elements before a filter is applied so they can be restored when the filter is later removed).
(In reply to comment #12) > The events would be > processed at different times, which may cause flicker in any widget that is > reacting to them. Could you spell this out more? I don't see how this could cause flicker.
Copying chat between Boris and myself on July 21: (10:14:46 AM) Matthew Hall: Have you looked over my patch on 222991 (getRealizedElements) (10:16:28 AM) Matthew Hall: It seems the lack of this feature is causing grief for everybody--do you think we could make some time to work together on this? (10:16:40 AM) Matthew Hall: My patch is still missing an implementation for the tree content providers (10:39:40 AM) Boris Bokowski: Did you have a look at my patch from bug 237359? (10:39:50 AM) Matthew Hall: I did (10:39:57 AM) Boris Bokowski: It's been a while but I think I had an implementation for the tree content providers. (10:40:15 AM) Matthew Hall: let me look again (10:48:07 AM) Matthew Hall: There's a difference in our implementation strategies, and I'd like your opinion: in getDeferredKnownElements(), all additions and removals happen after all the viewer updates. Whereas in getRealizedElements() additions happen after the viewer is updated, and removals happen before the viewer is updated (the reverse of getKnownElements()). The idea with my approach is that you can query the state (i.e. checkbox or selection or whatever) of the removed elements before they are removed. Thus you could save the state of elements as they are filtered out of the viewer, and restore state as they are reinserted into the viewer. (10:50:29 AM) Matthew Hall: Hence the name "realizedElements," because updates (additions and removals alike) always happen while the element is realized in the viewer (10:56:55 AM) Boris Bokowski: Makes sense to me. (10:57:52 AM) Matthew Hall: the tradeoff is that without some optimizations, there will be lots of individual updates to each set instead of bulk updates (10:58:00 AM) Boris Bokowski: I don't think my snippet relied on how removes are handled. (10:58:11 AM) Matthew Hall: it didn't (10:59:10 AM) Boris Bokowski: What do you mean by lots of updates? Adds and removes would have to be separated, is there anything else? (11:00:32 AM) Matthew Hall: In the case of ObservableListContentProvider and ObservableListTreeContentProvider (where we are iterating the diff using ListDiffVisitor it's hard to do bulk updates because we are only seeing one event at a time (11:00:50 AM) Matthew Hall: So we're doing an individual add/remove to the sets for each list diff entry (11:01:30 AM) Boris Bokowski: right :( (11:01:38 AM) Matthew Hall: Stefan voiced a concern that if a single bulk list change were split up into lots of little updates to the realized elements set, that this might cause flicker (11:01:50 AM) Matthew Hall: I'm not sure what he meant by that (11:03:18 AM) Matthew Hall: Although in theory we could do bulk updates to deferredElements, I'm just concerned it will turn out to be really tricky (11:03:50 AM) Boris Bokowski: Probably not trivial to write, but is it impossible? (11:03:54 AM) Matthew Hall: no (11:04:03 AM) Matthew Hall: handleReplace is a problem though (11:04:50 AM) Matthew Hall: We stand to lose the handleReplace optimization on the viewer update in favor of doing bulk updates on realizedElements (11:10:37 AM) Boris Bokowski: We could still do the handleReplace optimizationm, as long as nobody asked for the realized elements set... (11:11:37 AM) Matthew Hall: I was thinking about lazy init on realizedElements too (11:12:44 AM) Matthew Hall: If we lazy init realizedElements, I think we should just focus on making it work correctly for now and revisit this later for optimizations if it turns out to cause performance problems (11:13:18 AM) Matthew Hall: Hopefully with real performance data to steer our efforts (11:19:39 AM) Boris Bokowski: +1
Created attachment 108139 [details] Add getRealizedElements() to tree content providers Needs tests.
Created attachment 108140 [details] mylyn/context/zip
> Stefan voiced a concern that if a single bulk list change were split up > into lots of little updates to the realized elements set, > that this might cause flicker I was basically observing that the adds and deletes would be broken apart, which is never as efficient. I guess that doesn't directly cause flicker - it would only do so if a listener kicks off an asyncExec that touches a widget or does something that forces an immediate repaint (like moving a widget on gtk or creating/destroying something). Since these cases are common, I tend to be pessimistic and assume that more updates = more flicker. However, you'd be right in observing that this is often not the case. I like your use-case for remembering checkbox states, though - and I agree it would be nice to support it. That's why I like having two sets - one that updates before and one that updates after - that way both sets could batch their adds and removes while still having a place to hook your remove listener if you want to remember checkbox states... but I agree that if we can't do both then your checkbox use-case is probably more important than my optimization. I suppose we could have three sets - one with the old semantics, one with "before" semantics and one with "after" semantics... but perhaps that's excessive.
(In reply to comment #18) > I was basically observing that the adds and deletes would be broken apart, > which is never as efficient. I guess that doesn't directly cause flicker - it > would only do so if a listener kicks off an asyncExec that touches a widget or > does something that forces an immediate repaint (like moving a widget on gtk or > creating/destroying something). Since these cases are common, I tend to be > pessimistic and assume that more updates = more flicker. However, you'd be > right in observing that this is often not the case. In the latest version of the patch, I have the code fully calculating the added and removed known elements first, which allows me to consolidate the changes as follows: Set additions = ... Set removals = ... knownElements.addAll(additions); realizedElements.removeAll(removals); // iterate the diff and apply all updates in the viewer realizedElements.addAll(additions); knownElements.removeAll(removals); Thus each diff will at most be broken into two change events on each element set.
Planned for 3.5M2
Released to HEAD > 20080904
Verified in I20081209-0100 by code inspection
