Bugzilla – Full Text Bug Listing
| Summary: | Still an unneeded refresh from the lightweight decorators | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Martin Aeschlimann <martinae> | ||||||||
| Component: | UI | Assignee: | Tod Creasey <Tod_Creasey> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | daniel_megert, jean-michel_lemieux, Michael.Valenta, michaelvanmeekeren, n.a.edgar, t.s.maeder | ||||||||
| Version: | 2.1 | ||||||||||
| Target Milestone: | 3.0 M8 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 2000 | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 55012 | ||||||||||
| Bug Blocks: | 40204, 53482 | ||||||||||
| Attachments: |
|
||||||||||
I don't see the flash of the icons myself - we only send updates if there is something interesting to decorate. Are you saying that there are no decorations whatsoever or that the decorations don't change? I'm seeing this all the time. There are no decorations on the elements, and a refresh can be seen that doesn't add anything. I debugged it by setting a breakpoint in TreeViewer.doUpdateItem: You can see that for the exoand node the same item is refresh twice. The second refresh comes from the decorator manager. Just checked and I can replicate - the issue is that with your setup we still get a decoration from the CVS decorator - it is an empty String as a prefix. The CVS decorator should not bother adding empty prefixes. When expanding in the packages view it is the decorator manager that asks the CVS decorator to decorate the listed items. The method ILightweightLabelDecorator::decorate(Object, IDecoration) doesn't spec what should be returned if the decorator doesn't have a decoration to add. Until the API is spec'd differently, the decorator manager could ensure that if a IDecoration contains emtpy decorations (e.g. prefix, postfix, overlay) it is simply ignored. Moving to UI. One of things I would like to do for 3.0 is to change the specification such that a return value of null from decoration indicates "pending" and to spec the API accordingly. This is not a breaking change as null is already a valid value to return accordning to the spec but we didn't want to change the current behaviour too close to a release as we were afraid developers may not be using it. Marking as an enhancement but I think this is a big performance win for us potentially - we just need to be sure to handle the "update with no change" case correctly. Better create a new bug report for the API improvement. The unneeded refresh is really a bug and I would be glad if you could fix that soon. I think CVS should fix this by avoiding dummy calls. The call to ILightweightLabelDecorator::decorate is a hook that is optional, no decorator is required to decorate. I thought that's obvious. Tod, I have done some investigation on preventing decorator flashing and have prototyped a few approaches. I am attaching the patches for the prototype that I feel is the most promising. The approach can be summarized as follows: - The interface IItemLabelProvider has been defined and extends IBaseLabelProvider. It has a single method updateItem(Item) which updates the SWT item directly. - The TreeViewer#doUpdateItem() has been changed to use this new interface if the view's label provider implements it. - The DecoratingLabelProvider implements IItemLabelProvider and only updates the label and image if the decoration is ready or the item is missing a label or image. This has eliminated the decorator flashing in all the testing I have done. The drawbacks I can see are: - code changes will be required in any client views that update items in custom ways (e.g. the ResourceToItemMapper classes of JDT and Search). The patch includes the changes for JDT and Search. - An update must still be fired when the calculated decoration is empty since the previous invocation of IItemLabelProvider#updateItem(Item) may have done nothing. This didn't seem to have a large effect on performance in my tests but I didn't do any stress testing. Also, I'm not sure what you mean above when you mention returning null to indicate that a decoration is pending. We can discuss this when you are available. Created attachment 6594 [details]
Patch to prevent decorator flashing
Let's discuss this when Tod is back. I'm not in favour of requiring label providers to deal directly with SWT Items. The whole point of JFace viewers is that they insulate the application from this complexity. Yes, I had thought of this as well. Label providers will not be required to deal with SWT Items as it is not required that general label providers implement the new interface (i.e. the code in TreeViewer determines which interface to use based on what the view's label provider impements). It is only a requirement for those that wish to support deferred decoration (i.e. I expect that DecoratingLabelProvider will be the only label provider that implements the new interface). However, I think it is still a concern that the view is passing the Item (which is part of its internal data structure) to the label provider. I had meant to include this in my list of drawbacks but forgot. I like the changes here as there are a lot of items decorated in the typical view. Adding Dani to the list as Search will want to apply the patch that Michael Valenta provides in order that they can take advantage of it too. Nick I just want to double check with you that there is no problem with this from your side before I apply it. It is a non breaking change and an optional addition for Search and JDT. Created attachment 8464 [details]
Updated patch
Here is the updated patch with changes suggested by Nick and Michael.
I have updated the patch in the attached zip file with the suggestions from all of you. It now uses a wrappering class to handle the update. I have also updated the ResourceToItemsMapper for jdt ui and search in the file. The Search View does not appear to use the DecoratingLabelProvider directly so my version of the search ResourceToItemsMapper is currently not in use. However I think this may be obsolete anyways as there is a new search view on its way. Please review - I will be applying this to HEAD during the week of March 15. Created attachment 8586 [details]
Update after Nicks review
Updated version post a code review and subsequent refactoring after Nicks
review.
Final version of the patch (with a bg fix) has been released into HEAD. I am curious about how to verify that this is fixed? Where can you see the improvements? With the current M8 build I still see the flashing decorators. In which view? I changed all that I could and checked against the Resource Navigator. I am not sure that the PackagesView adapted to this change in time for M8. >I am not sure that the PackagesView adapted to this change in time for M8.
I see patches for ResourceToItemsMapper but nothing for the Packages view. I
also did not receive a bug against the Packages view. Can you be more specific.
Note: if you expected actions from us it would be best to either reassign this
PR to JDT UI or create a new bug against JDT and add this one as dependent.
I have the bug with the patch for jdt.ui that Tod provided. It's bug 55012. Sorry that I couldn't release it for M8 but I'll do it directly after M8. Thanks Martin. Jean-Michel I am going to close this. That's fine, I just wanted to ensure that the other components were aware that they had to made changes. Thanks Martin. Verified in Resource Navigator in 200403251600 |
20030316 1. In the preferences Workbench>Label Decorations turn CVS decorators on 2. In CVS>Label Decorations set all text settings to '{name}{dirty_flag}' and turn off all image decorations 3. Add an existing project with CVS (e.g. org.eclipse.jdt.ui) 4. In the package viewer expand a package node with many CU 5. The node opens, but is followed by a flash of all icons: Labels and images are the same, because all files are in sync When the decoration manager thread fires label updates, it should test if the decorations result has any added decorations. if not, the update is unneccessary