Bugzilla – Full Text Bug Listing
| Summary: | Ensure update of working set team state decorations | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Michael Valenta <Michael.Valenta> | ||||||
| Component: | UI | Assignee: | JDT-UI-Inbox <jdt-ui-inbox> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | daniel_megert, martinae | ||||||
| Version: | 3.2 | ||||||||
| Target Milestone: | 3.2 M6 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Michael Valenta
I don't see how exactly we can take advantage of this API, but maybe I missed something. Our views all decorate their elements using platform's general label decorator. This decorator shows all decorations configured in the preferences; in jdt.ui we don't really know what decorators these are and how they behave. All we know is that when a change happens, they will send a LabelProviderChangedEvent to all participating viewers. Currenty all these events have to be communicated in term of IResources as it is the only thing everybody understands. The Java viewers mapped the resources to Java elements and requested a label update of the corresponding Java elements. This then invoked the lightweight label decorators to perform a background computation. When done, another LabelProviderChangedEvents was sent (this time containing the actual elements) to refresh the images. If I understand correctly, you suggest to not listen to LabelProviderChangedEvent anymore, but get changes directly from the team infrastructure. But team is just one of many decorators, and we don't really know that team participates. As I see it, it is also for compatibility reasons that we can't do anything else that listening to LabelProviderChangedEvents and map them to our world. It's always good if the events broadcasted are as minimal as possible, e.g. in your case only contain the element that really have changed state. I couldn't observe that in I20060119, the broadcasted changes still seem to be the full parent chain. A problem is that the current implementation of the lightweight decorator also uses the broadcast mechanism to notify about the images ready to update. So viewer decides that a update of resource X also mean to refresh all it parent (because that makes sense in the structure that it presents), these refeshes will end up as new LabelProviderChangedEvents on all viewers. A especially nasty scenario is that a if you open a new dialog that renders elements for the first time, this results in LabelProviderChangedEvent for all these elements, causing all viewers to refresh. So I think we should really improve the lightweigth labeldecorator to send the updates in a more specificly to the viewers that requested them. But that's a different issue that shouldn't go in this bug. Have I missed anything? You still need to react to label updates. You are right to say that there are issues in the lightweight decorator update mechanism. I agree that we should address them at some point but unfortunately, now is not the time. For now, we will just have to live with the fact that label updates happen too frequently. I think it is more important to ensure that the label being shown is correct. That is what this API tries to address. Currently, if you do nothing in JDT, all your labels will update properly except those on working sets. That is, if I make a change in a resource that is contained inside a working set, the working set will not become dirty because CVS has no way of knowing that the working set is being displayed by a view. This API provides a mechanism whereby viewers can determine when label updates are required for model objects that do not have a one-to-one mapping to resources (i.e. CVS knows how to decorate the element, it just doesn't know when the element needs to have it's label changed). This API defines a contract between a view that uses lightweight decorators and a team lightweight decorator. The SynchronizationStateTester has a method (getDecoratedStateMark) that allows clients to determine if team state is actually being decorated. If it isn't, a label update isn't required. It also has a subscriber which will notify clients when team state has changed. Clients will need to listener to both local changes and team changes and issue label updates when the state of the element changes (getState). If the viewer does not issue a label update for elements that do not have a one-to-one mapping with resources, these elements will never have their labels updated. What you're suggesting is to introduce a special treatment for the team decorator. But there might be other decorators taking part with platform's global decorator service that might decorate a working set element and also need a refresh when it reports a label decorator change on one of its resources. I guess we could declare the team decorator to be something special. I'll also have a look if we can implement it with only listening to LabelProviderChangedEvents. I just realize that that team is decorating IWorkingSet's now. That means so we're just lacking the updating? I think Dirk's biggest problem was to get the decoration running with good performance. Although it is conceivable that there may be other decorators that need similar treatment as Team decorators, I don't know of any concrete cases. I agree that a more general solution would be preferrable but I couldn't come up with an acceptable one (I am open to suggestions but time is short). The advantage of the proposed approach is that it allows views to handle team decoration themselves or disable it (i.e. when used in team operations, the lightweight dirty decoration is disabled and sync decorations are added by the label provider). As for trying to determine when to update working sets by listenering to label change events, there is just no way for you to know that a label update of a project should result in a label update for the working set. In the end, you will need to update the label of working set whenever a project changes (even if the change in the label of the project had no impact on the label of the working set). The new API allows a view to cache the previous state of the decoration of any model element and only fire a label change when that state changes. We have a mapping of root elements of a working set to a working set, so we can do the resource to working set mapping quite efficient. The working set updating is currently implemented (already in I20060119), so with your new support to decorate elements of type IWorkingSet we're in fact done. I made some tests, and they seem to work. Dirks problem was always to have good performance when evaluating the decoaration of a working set, but as you do that now, this is solved. We are currently relying that an update always contains the full parent chain. If you plan to improve on that, give us a note, so I adapt our code to not rely on it. Setting this to worksfore, reopen if you disagree. Here's the scenario that fails: 1) Setup a workspace that has one or more projects shaed with CVS and no local changes 2) In the Packages Explorer, turn on working sets. The working set should be decorated with the CVS shared image 3) dirty one of the files in a project in the working set. The working set will not become dirty. 4) Turn off working sets and then turn them back on. The working set will now be decorated as dirty Created attachment 36498 [details] Patch Here is a patch that modifies the Packages explorer so that working sets will update when the team state of relevant resources change. The patch will only fire label updates for working sets if the team state associated with the working set has changed. This patch relies on API changes made (and just released to HEAD) as part of bug 131440. Thanks Michael. IMO is the patch you're suggesting not the right way to go. The package explorer nor any other Java view know about the team decorator, it is coming in through the global decorator mechanism. Through that, team decoarators are just one of many decoarators, and it isn't the job of the package explorer to make sure they all get updated in their specific way. The right solution should be that the team decorator sends out the label change events for the concerned working sets, so all viewer that show working sets (not just the package explorer) will update. Martin, what you suggest would require that Team decorators assume that working sets are part of the resource model. However, they are not part of the resource model and may even contain non-resources (and still be displayed in a view somewhere). And working sets are just one example of a logical model element that doesn't have a one-to-one mapping to a resource. To do what you suggest would mean that Team (or CVS) would need to know about every non-resource model object that is being displayed in a view (Working sets are only one example. Support is needed for any logical model that has an element that doesn't have a one-to-one mapping to a resource). When deciding on the architecture, our choices were to make all view models available to the Team providers or to make the Team state available to the logical models. The former approach, which you are advocating, is extremely complicated as it would need to expose API for traversing logical models and for receiving logical model deltas. Actually, it is even more complicated than that since it would require the ability to traverse and receive deltas for the models as they appear in the view so, for instance, there may even be multiple Java models depending on how many views are showing Java content. The later approach of exposing the Team state and Team delta was fairly straight forward especially considering that Team providers will already have most of the required pieces to implement the support. I think it is reasonable to expose the fact that there are team state decorators to the model views(i.e. it is a vital piece on the IDE) and require them to update there model elements that don't have a one-to-one mapping to a resource. It's always a decorator that chooses what elements it decorates. And for the ones it does, it also has to make sure that update changes are sent out. Making an exception here for the team provider really sems wrong. What about all other contributed decorators? Note that only since the ResourceMapping, JDT has any knowledge of team. But we still don't know about the team decorator, if it is turned on, and how. To answer your specific points first, the patch I provided has code in the hasRealChange method that checks if decoration is enabled for a particular model element (i.e. ITeamStateProvider#isDecorationEnabled). We are not concerned about other contributed decorators because we don't have any client scenarios that require them. Also, I believe that making the existance of Team providers/decorators a first class notion in the IDE is acceptable and even benefitial. As for the current architecture, The JDT use of working sets is only one of many scenarios we have to support. Here are some factors we had to consider: 1) a model element may only represent a portion of a file. 2) the resources contained in a model element may change (i.e. resources can be added to a working set) 3) a model element may or may not be visible in a view 4) the view presentation may not match the model presentation What you are suggesting is that the CVS decorator keep track of all elements it ever decorates and fire a label update whenever a decorated element changes. To do this, we would need the following: - API to delegate the sync state calculation to the logical model (point 1). The API would need to be tied to the view to handle point 2 as well. - A logical model delta API so that the CVS decorator would know when the resources contained in a logical model element changed (point 2) - A view level API to let the decorator know that the element is no longer visible in a view so that the decorator could stop calculating the sync state for it (point 3) Given the manpower we had, this was too much API to consider for 3.2. I believe the current API is the best we could do, given the time constraints we were under and the involvement we received from clients. If you still have a strong objection to this, then I believe the best course of action would be to have a meeting next week after EclipseCon and discuss what our alternatives are. Being at EclipseCon my answers were rushed and and I forgot half of the story again. Sorry. You're completly right. Team can't know all the elements it decorates. In fact we solve the problem in our tree viewers where we map all label change events for resources to item updates of the actual elements. I just debugged the package explorer, and we do this correctly also for working sets: A change in a file results also sends an update to the working set it is in. I verified that with the debugger and CVSLightweightDecorator gets these events. All my (simple) test case seem to work. CVS dirty decorations are correctly added and removed. However, I found a problem if a working set contains unshared and shared projects: Such a working set is always computed as dirty. So can you please again verify your implementation? And maybe also do some debugging? I still think that everything is correctly solved on our side. BTW, the code that adds working sets to the element to be updated is PackageExplorerProblemTreeViewer.addAditionalProblemParents Using build I20060327-0010, if I dirty a resource within a previously clean working set, the dirty decorator does not get applied to the working set. Is this working for you? I've looked in the class you mentioned but I don't see any code related to label updating. At one point, Dirk had tried implementing a strategy of updating the label or a working set whenever the label of a direct child was updated but it was my understanding that he did not put it in because of efficiency concerns (i.e. too many unncessary label updates were performed on working sets). Yes, this is working for me. I also verified this in the debugger. Please have a look in the debugger if your decorator gets the request for decoarating working sets and what it answers. ProblemTreeViewer.handleLabelProviderChanged receives all events of updated resources and uses addAditionalProblemParents to evaluate the working sets that show these resource (and adds them to the set of elements to be updated). I think I found the problem in our code. Currently, working sets are only updated when problems decoarators are involved. In my examples they always were, so maybe that's the problem. Created attachment 37071 [details]
Screenshot
Here's a screenshot of what my Packages Explorer looks like after I edited the .classpath file in a previouslt clean working set. Notice that the .classpath file and its containing project are marked as dirty but the working set is not. This is the behavior I see with build I20060328-0010.
I released the fix for the problem found in comment 16 for M6 (> 20060328) The example in comment 17 now works as well. marking as fixed Now it works :-) Thanks. verified in I20060330-0010 |