Community
Participate
Working Groups
Right now, all the set methods on WorkbenchPart fire their own property change event. This means, for example, that if the part changes its name, title, and tooltip at the same time, the presentation needs update the widgets 3 times. The workbench already contains an optimization that allows it to process more than one property change with one UI update. If it were possible for the part to fire all the property changes after the last property had changed, the workbench would detect all of the changes in the first property change and only update the widgets once. Unfortunately, clients cannot benefit from this optimization since we do not expose API for deferring property change events. This PR requests the following new API on WorkbenchPart: /** * Calling deferEvents(true) will queue all property change events * until a subsequent call to deferEvents(false). All property changes * will be fired as a batch. Calls to this method should be protected * inside a try/finally block to ensure that each call to deferEvents(true) * has a matching call to deferEvents(false). * * @param defer if true, property change events are disabled. If false, * property change events are re-enabled. */ void deferEvents(boolean defer) It also requests the following new property constants in IWorkbenchPartConstants and IPresentablePart: /** * Indicates that subsequent property changes are part of an atomic operation. * The listener may, at its option, delay processing of subsequent property * changes until the matching PROP_DEFERRED_END event is received. */ int PROP_DEFERRED_START = 0x106; /** * Indicates the end of an atomic sequence of property changes. If the listener * has been deferring any events due to a PROP_DEFERRED_START event, * it should process those events immediately. */ int PROP_DEFERRED_END = 0x107;
Possible alternative: provide a method like deferEventsWhile(Runnable). This avoids the possibility of clients accidentially failing to reset the flag after their batch of changes.
Re: comment 1 Good idea. However, I'd suggest offering this in addition to (rather than instead of) the deferEvents method, since the extra Runnables would have some cost associated with them.
Is there an obvious place to clear the deferred events if a client accidentially forgets to call deferEvents(false)? (On the subject of expensive Runnables, is there a particular reason why WorkbenchPart.firePropertyChange() creates one SafeRunnable per listener?)
How big a performance win would this be? I don't know of any parts that frequently change multiple presentation properties.
Not a performance issue currently
Sorry - wrong report
Re: comment 3 (On the subject of expensive Runnables, is there a particular reason why WorkbenchPart.firePropertyChange() creates one SafeRunnable per listener?) ...because I didn't know about it. :-) It's about to change.
Re: comment 4 Most property changes happen in batches, but you're right that most parts don't change their properties frequently. I guess we can leave this until post-3.1 unless we actually find a part that needs it.
The SafeRunnable is gone from WorkbenchPart. I replaced it with a try/catch block.
Re comment 7, each listener was run in its own SafeRunnable to provide tighter isolation, and keep one listener's failure from affecting the notification of other listeners. A try/catch block is cheaper (free in the case of no failure), but it should still be around an individual notification. Note that Platform.run is very careful in which throwables it handles. It does: try { code.run(); } catch (Exception e) { handleException(code, e); } catch (LinkageError e) { handleException(code, e); } It allows Errors other than LinkageErrors to propagate. In particular, ThreadDeath should not be caught (although that would be bad in this case because we're in the UI thread).
Nick, the code in head works as you suggest. The try/catch block is around individual notifications, and it allows Errors to propogate (as you suggest). However, there is no special treatment for LinkageErrors. All errors are allowed to propogate.
Jeff, do you know the rationale for Platform.run's handling of LinkageErrors? I assume it's just so that if a plug-in's classes are not found, then it's isolated to the individual run. We might want to do the same here.
Doesn't SafeRunnable display a message dialog if there is an error? What does WorkbenchPlugin.log(..) do?
You know, I happened upon that code the other night and was wondering why it was just linkageErrors? I believe that in the past we looked at more things. There have been many (long) discussions about what we should catch and eat vs. let flow through. Actually, if pressed before seeing the code I would have said that we would catch/log/eat everything except for VirtualMachineErrors and ThreadDeath. Not that I want to reopen those discussions but it is curious.
Re: comment 13 The fact that it was opening a modal dialog was a bug. The possibility of running an event loop is not part of the specification for firePropertyChange, nor is it an obvious part of its function.
But telling the user that something bad happened is useful, even during development. Asyncexecs could be used as a way to report the error without running the even loop during the actual notification.
We already have error logs and log listeners that allow you to record and monitor errors in a pluggable manner (the PDE Error Log is an example). If the UI was to offer some sort of generic error-reporting facility in the future, we could also update our try/catch blocks at that time... but using a SafeRunnable in its current form is a bug. Keep in mind that an exception thrown in a listener is a bug in the thing that contributed the listener, not a bug in the part firing the event...and even if the user cared about the error, they would only want to be told once -- not on every property change.
> using a SafeRunnable in its current form is a bug Is SafeRunnable ever "safe" to use? I thought that in some cases naughty listeners were removed.
Re: comment 18 I'm not fond of it... but in its current form, it could be used in any method that permits the opening of modal dialogs. Anyway, we're getting somewhat off-topic. This PR regards the merging of property change events.
There are currently no plans to work on this feature. PW
Changes requested on bug 193523
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
