Bugzilla – Full Text Bug Listing
| Summary: | [Decorators] CVS decorations blink on build [package explorer] | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Andre Weinand <andre_weinand> | ||||||
| Component: | UI | Assignee: | Tod Creasey <Tod_Creasey> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | markus.kell.r, martinae, Michael.Valenta, mik.kersten, tobias_widmer | ||||||
| Version: | 3.0 | ||||||||
| Target Milestone: | 3.4 M6 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Andre Weinand
I20040914 Whenever a build occurs the CVS decorations in the package navigator "blink" that is they go away for half a second and then come back. I'm not sure whether this is Mac specific: Dirk has seen it on Win XP too. Do they also blink in the Resource Navigator? No, they don't blink in the resource navigator. There was API introduced in 3.0 to allow views to postpone decoration until the decorator was prepared by the background thread. The packages view should be using this API to avoid such blinking. The packages explorer is using this API but there must be a case that causes the item to be redrawn before the decorator is ready. Moving to JDT UI to investigate. P.S. I have not been able to reproduce this on Win XP. When I rebuilt (i.e. cleaned) a project, the only blinking I saw was the removal and return of the compile warning decorator. To reproduce, a *non*-lightweight decorator must be present in the system. I
verified that Andre has one in his system, and I can switch the blinking on and
off by enabling/disabling the plugin org.eclipse.jdt.ui.tests (which also
contributes such a "full" decorator, in DecoratorManager-speak).
The problem is somewhere in the interaction between a DecoratingLabelProvider,
the DecoratorManager, and non-lightweight decorators. In our case, the
ProblemsLabelDecorator listens for Resource changes, and sends a
ProblemsLabelChangedEvent (extends LabelProviderChangedEvent) to the
ProblemTreeViewer, which in turn updates the affected items.
DecoratingLabelProvider#updateLabel(..) calls
DecoratorManager.prepareDecoration(..), which
(a) scheduleds a background job to decorate the label with lightweight
decorators, and
(b) returns true ("decoration is ready") iff there are full decorators present
in the system.
Returning "true" causes updateLabel(..) to set new text/image without waiting
for lightweight decorations ("CVS decorator off"). After a while, the background
job adds the decorations ("CVS decorators on"). Since problems are reported
repeatedly, this manifests in a "blinking" icon.
I'm not sure what the right solution should be here, but I think the
DecoratorManager should handle "mixed" decorator situations more gracefully.
Currently, it doesn't obey the contract of
IDelayedLabelDecorator#prepareDecoration(..), which says: "If it is already
decorated and ready for update return true. If decoration is pending return
false." The DecoratorManager should always return false when it has scheduled a
background update.
A solution could be to store the full-decorated images and use them as the base
image for lightweight decorators.
Moving to Platform/UI.
The problem with this is that a full decorator needs to be sertviced right away (this is the API contract). Without actually doing the decoration we have no way of knowing that decoration will occur - the optimization that Michael and I put in in 3.0 to reduce the flashing can only work if there is no full decorators. I assume the full decorator is in a test suite - these are really ineffecient and should not be used if it can be avoided. Your idea is a good one but we want to discourage the use of these decorators as they are so heavyweight - we won't be doing anything to make them nicer to use as a result. The problem with "not making life easy for full decorators" is that users don't know who is to blame. This PR e.g. blames the good citizen "CVS Decorations", and the user has no way to know how to disable the "blinking". If the decoration algorithm won't be changed, we should at least publicly blame non-lightweight decorators on the Preferences > Workbench > Label Decorations page. Their name could be extended to read "<decorator_name> (heavyweight)", and something like "Note: Enabling this heavyweight decorator may cause flickering." could be appended to the description. We should likely mention this in the schema but I don't think we want to do it in the UI. Before 3.0 they all blinked - the current behaviour is an optimization for lightweight decorators. *** Bug 108368 has been marked as a duplicate of this bug. *** (In reply to comment #8) > We should likely mention this in the schema Reopening to at least put a big red warning into the schema. > but I don't think we want to do it in the UI. > > Before 3.0 they all blinked - the current behaviour is an optimization for > lightweight decorators. For the user, it's still a bug, no matter whether it's an old bug or something new. Just in case someone is worried: The blinking in the SDK in I20080212-0800 and later is not caused by full decorators, but by some other problem with the SimpleStyledCellLabelProvider (bug 219581). Created attachment 90320 [details]
patch
The following patch improves the blinking by at least test for enabled full decorators.
Other than that we need to find a way to make it clear to developers that full decorators are deprecated. Unfortunately it's the same extension point as for lightweight decorators. Maybe create a new one that only supports lightweight decorators and deprecate the old one?
And, as Markus suggested: annotate the label of a full label decorator on the label decorator preference page. Just before only when I debugged my code I saw that the 'Pessimistic Filesystem .. ' - decorator is a full label provider as well and caused the flickering.
As a last point, in the DecoratorManager, if we know that a decoration is pending and that the element already has a label (text.length > 0), can't we just wait for the pending decoration update to apply the full decoration?
Martin if we were to move either we would be breaking backwards compatibility. I think the best we can do is a full health warning on them and a deprecation. Thanks for the patch! Created attachment 90356 [details]
patch 2
This patch implements what I suggested as last point.
If we know that a light way label decoration is on the way and it guaranteed to update the label (force == true), then we wait with the full decorators.
The update will refresh the label and apply both light and full decorations.
The only change is for full label decorator that they are now not applied right on the update anymore, but with a small delay.
I'm not an expert if there is a spec that would guarantee a decorator to be applied exactly on update.
But, as full decorators are getting less and less (hopefully) this could, IMO, be risked.
The second solution looks like the way to go Martin - I concur that if a force is going to happen anyways we don't need to do it. Patch released for build >20080221 Verified in I20080323-2000 Marking VERIFIED |