Bugzilla – Full Text Bug Listing
| Summary: | [Decorators] Content type decorator: Updating missing | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Martin Aeschlimann <martinae> | ||||
| Component: | UI | Assignee: | Boris Bokowski <bokowski> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | daniel_megert, d_a_carver, Kevin_McGuire, Mike_Wilson, Szymon.Brandys, valentinbaciu | ||||
| Version: | 3.4 | Flags: | Mike_Wilson:
pmc_approved+
martinae: review+ Kevin_McGuire: review+ |
||||
| Target Milestone: | 3.4 RC3 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Martin Aeschlimann
Fixed in HEAD. The WorkbenchContentProvider now also listens for CONTENT changes. So that means that an update event is sent out for each resource modification, and the content type is reevaluated every time. Isn't that a bit too expensive just for seeing a nicer image? Maybe it can be limited to a subset of file suffixes (XML)? (In reply to comment #2) > So that means that an update event is sent out for each resource modification, > and the content type is reevaluated every time. Isn't that a bit too expensive > just for seeing a nicer image? At least it is only reevaluated for files that actually appear in a tree viewer somewhere. Also, I think that you only pay a cost if there is a content type definition that has a content describer which looks at the file contents. > Maybe it can be limited to a subset of file suffixes (XML)? Currently, it is limited to those file types that have file content based content types defined. Probably not much more than .xml, and there is no hardcoded list of file suffixes. Do you still think that this is too expensive? Szymon, is there API to find out whether there are file content based content types for a certain file name? (In reply to comment #4) > Szymon, is there API to find out whether there are file content based content > types for a certain file name? > IContentTypeManager#findContentTypeFor(InputStream contents, StringfileName) where name can be null. (In reply to comment #5) > IContentTypeManager#findContentTypeFor(InputStream contents, StringfileName) > where name can be null. This does not look like what I need. I do have a file name, and I don't want to provide the contents. IContentTypeManager.findContentTypesFor(String fileName) looks better, but its implementation looks horribly inefficient. Also, it only answers half of the question; I would also need to know if choosing between the returned (possible) IContentType objects requires looking at the actual file contents. Something like IContentType.hasDescriber(). Szymon, can you ping me directly when you have time? Can we reopen the bug? I (and also others from the lab here) are really a bit concerned that this updating can be really expensive, especially if a product adds more content type evaluators. I think we pay a high price for something that happens quite rarely: how often does a content type change? Maybe we can leave the 'refresh content type' to the user and add it to the 'Refresh' action? Reopening so that we can discuss what to do. I will check what I can do about IContentTypeManager.findContentTypesFor(String fileName). However I would check one thing. If we have two content types defined, one for an extension (.class) and second which has a describer and we have a file that matches both, which one will be chosen? I'm guessing that it depends on the order in the registry. But maybe it checks extensions first. I have to check that first before we proceed. The case is that the file can be recognized by the content type with a describer and then (when modified) by the one with the extension. >If we have two content types defined, one for
>an extension (.class) and second which has a describer and we have a file that
>matches both, which one will be chosen? I'm guessing that it depends on the
>order in the registry.
See also: IContentTypeManager.ISelectionPolicy
(In reply to comment #11) > >If we have two content types defined, one for > >an extension (.class) and second which has a describer and we have a file that > >matches both, which one will be chosen? I'm guessing that it depends on the > >order in the registry. > See also: IContentTypeManager.ISelectionPolicy > I have some comments: - we could use IContentTypeManager.findContentTypesFor(String fileName) for getting a list of all content types which match the file name - we already have a method to get subscribers for content types ContentType#getDescriber which is not API We could try to use this method and check if it would improve efficiency. (In reply to comment #7) > Maybe we can leave the 'refresh content type' to the user and add it to the > 'Refresh' action? I have tried to implement this but did not find a satisfactory solution. The problem is that a refresh operation does not have information about which elements are visible in viewers so all it could do would be a global refresh, like the one that's happening when you turn on or off decorators. So as a result, we would be penalizing every refresh operation. Is there anything cheap we can do, for example not do content-based content type lookup if the resource isDerived()? Would this help for the common case of lots of .class files that have a content change? We can't know what kind of content type evaluators products install and what elements are touched often by builders. In the case a extra plug-in happens to install an expensive updater on a common file, the performance will be very bad. I assume that content types are rarely changed. So I don't think the risk and performance costs are justified. I would suggest to remove the update feature and look how we can do it in a more performant way in 3.5. This might leave the user to stalled images. Open/close editor or 'Refresh' in the package explorer should solve this (since 3.4 M7 'Refresh' also forces to refresh the view). Alternatively you could add a 'Refresh content type' button in the property page of the element. And in cases where they are, the editor that modifies the file where this typically applies can decided to issue a refresh. An other way to force a image refresh is to use 'Refresh' in the package explorer (since 3.4 M7 Refresh also forces to refresh the view) Please adjust the target milestone, so it does not point at a closed milestone in the past. Moving to RC3. Created attachment 102210 [details]
proposed patch
Martin, is this patch what you were suggesting? It gets rid of automatic updating by ignoring IResourceDelta.CONTENT changes and instead causes a full viewer refresh from the refresh actions in the navigator and the project explorer (the package explorer's refresh action, like you wrote, already does a viewer refresh).
Note that the code change looks big because I had to copy the implementation of run from RefreshAction into the subclasses. If this patch ends up in 3.4, I will enter a bug to clean this up in 3.5. Agree this is the right change at this point. Patch looks ok. +1 I think this patch is an abuse of the refresh action which is designed to bring the resource model back in sync (no matter what side effects it has since M7). Now we use it to fix up the UI.
Couldn't we disable this decorator per default and hence users that want additional accuracy of icons for content types pay for it? It seems strange to add that decorator, enable it and then not being accurate. Then, if they enable it, they will pay but get correct results. It would never come to my mind to use the refresh action to update my UI.
Comment to the patch:
>I had to copy the implementation of
>run from RefreshAction
Could this be centralized somewhere (e.g. create an internal refresh action that's used at the other three places)? If we find a bug in any of the three implementations I'm sure we won't think of updating the other two.
Kevin, sorry - somehow bugzilla removed your '+' on the review and there's no way I can add it back. (In reply to comment #21) > Kevin, sorry - somehow bugzilla removed your '+' on the review and there's no > way I can add it back. Np added myself back. I think users expect that an action called 'refresh' also refreshes the viewer, it's typically what 'F5' does on Windows. I disagree that this is an abuse. Of course it is always better if we can keep decorators up to date, but not at any cost. Another example of a decorator that is not kept up-to date is the Java method override indicator, where it would require to build subtype hierarchies. We have only very few bugs reported against this. So I think the chosen selection is the right one, at least for 3.4. The patch is fine. Question about the patch: Would using an UI job make thing easier here? >I think users expect that an action called 'refresh' also refreshes the viewer, >it's typically what 'F5' does on Windows. I disagree that this is an abuse. Right, I agree too but it should not become the common case that we need to hit F5 to refresh the UI. Actually, I misinterpreted this bug; I thought that also adding new files would not get the right decoration and in that case being forced to hit F5 would not be good. However, the decorator/icon is correct for existing and new files and hence my comment: >Couldn't we disable this decorator per default and hence users that want >additional accuracy of icons for content types pay for it? is obsolete as a content type change of an existing file is not the common case. Also, regarding the code duplication, I now saw Boris's comment: >If this patch ends up in 3.4, I >will enter a bug to clean this up in 3.5. All good. Sorry for the noise. McQ, +1? Released to HEAD. Verified on Windows XP using I20080530-1730, in the Package Explorer, Navigator, and Project Explorer. |