Bugzilla – Full Text Bug Listing
| Summary: | [CommonNavigator] Allow content providers to explicitly specify their ordering | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Dimitar Giormov <dimitar.giormov> | ||||||||||
| Component: | UI | Assignee: | Francis Upton IV <francisu> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P2 | CC: | bokowski, cbridgha, emoffatt, francisu, fyaoxy, kaloyan, martinae, mdelder, Mike_Wilson, paul.fullbright | ||||||||||
| Version: | 3.4 | ||||||||||||
| Target Milestone: | 3.6 M6 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 225647 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Dimitar Giormov
This is quite a major issue for the WTP project. It limits our options for populating the content of navigator view. Adding Michael: We need your expertise here, Michael. I wonder if we don't break other components if we change the Java content provider priority. Wouldn't it be better to have more priority levels? Or a way to express relations like: X has higher priority than Y, Z has higher priority than X I think there should be a more granular priority setting, say an int from 0 to 100. Speaking as the 4th (in the stack of features) content added to a project, the current mechanism presents a *very* limiting choice for specifying my priority. There often isn't even a gap that I can populate, instead presenting me with a choice of "who do I want to tie with?" and hoping that translation doesn't mess up the alphabetization sorting. I agree with comment 3. This could be done in a non-breaking fashion where we now allow both the values 'normal', 'high' 'highest' and numbers. We document that normal stands for 50, 'high' for 70 etc. If you agree then I rename this bug and move it to platform.ui. Or, if somebody with experience can assure me that changing the priority of the Java content provider to 'normal' can under no circumstances break any client, then I'll change it. I am fine with this solution. Moving the bug to Platform.UI to allow more priority levels. As with any extensible framework, priorities are always relative, and you'll always have this problem -- even if you only delay it until there's more extensions that are clashing. While it's possible to add more granularity, the typesafe enum used in the framework will have to modified in some way to understand these variations, as now there's some kind of "UNKNOWN/RELATIVE" --> go to integer value. The original decision was explicitly to avoid an integer based priority system as integer values don't immediately convey which is more important -- e.g. is 100 more or less important than 1? Changing the priority of the Java extension, as Martin indicates, is risky. There are other extensions (Resource, Working Set) that are prioritized based on the Java extension. Can you provide some details about why you need to reduce the priority of the Enhanced Java Web rendering? What is it clashing with, and/or what above it is clashing? I think we need to take a good look at the use case before changing the framework. We might still decide we need more granularity, but I want to be certain of exactly why that is required. There are not enough priorities in order to sort the elements in right order. As for the use case see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=225647 Because of the high priority of jdt it leaves us only Higher priority available. Reducing the Web renderer priority leaves the projects in inconsistent state.
Capturing the offline discussion:
I'm not sure if it's a legal change, but one way would be to indicate this with
"high/low"
within the existing priority attribute, using the same labels, and updating the sorters to be aware that the levels might be nested. It could even made to be indefinitely recursive ("high/low/lower") so that layers of extensions could build on existing relative priorities without really "inserting" themselves into the layer below them. Existing "xxx" values would be interpreted as "xxx/normal". I prefer this approach over a numerical priority as its intuitive to understand what comparison is being made. It also maps to the notion of various extensions coordinating within certain layers of the Eclipse projects.
Thoughts?
Michael
From: Paul Fullbright <paul.fullbright@oracle.com>
To: Martin Aeschlimann <martin_aeschlimann@ch.ibm.com>
Cc: Dimitar Giormov <dimitar.giormov@sap.com>, Michael Elder/Cambridge/IBM@IBMUS
Date: 04/24/2008 01:17 PM
Subject: Re: [Bug 228022] [common navigator] Need more priority levels to avoid clashes of content providers
Or another thought is an optional tag "sub-priority" (default value
"normal") which would just indicate the priority *within* the priority
level. For most people, priority "high" would be sufficient, but some
might want to indiate priority "high", sub-priority "highest".
Paul Fullbright
Oracle Corp.
Eclipse Dali/Java Persistence Tools Development
paul.fullbright@oracle.com
Martin Aeschlimann wrote:
> Maybe a combination of the current levels with numbers?
> high-1, high+4, higher-3, ...
> where higher is always more than high.
>
> Martin
>
>
>
> From: Paul Fullbright <paul.fullbright@oracle.com>
>
> To: Michael Elder <mdelder@us.ibm.com>
>
> Cc: Dimitar Giormov <dimitar.giormov@sap.com>, Martin Aeschlimann/Zurich/IBM@IBMCH
>
> Date: 24.04.2008 18:18
>
> Subject: Re: [Bug 228022] [common navigator] Need more priority levels to avoid clashes of content providers
>
>
>
>
>
>
> I think that long term solution would definitely address our concerns,
> since we know exactly the components we're trying to fit in with. The
> short term solution is basically equivalent to a numbering system, because
> one would have to consult the API comments to determine priority ranking in
> either case (is "slightly higher" higher or lower than "higher"?)
> Forsaking English, and in the name of fun, here are some priority
> suggestions: highish, less high, more high, much higher, really high, epic
> highness ... I exaggerate, but this in general is probably why, once we're
> past 10 or so priority levels, a numbering system would actually be
> clearer. That said, if inserting a few priorities in the short term is
> easiest to implement, we're also quite fine with that.
> Paul Fullbright
> Oracle Corp.
> Eclipse Dali/Java Persistence Tools Development
> paul.fullbright@oracle.com
>
>
> Michael Elder wrote:
>
> It seems like what you really want to do is specify specific
> relationships, e.g. "I want to be higher than X, Y, Z and lower than
> A, B, C". If that's the case, then I recommend that the correct
> solution involve more flexibility in the framework to specify
> priority based on other extensions; because as your use case
> indicates, you're trying to work yourself into specific boundaries of
> existing/new extensions.
>
> Adding more priority granularity will hide the fact that this is what
> you're trying to do. E.g., if I have 30 extensions all with a number,
> it's not clear to a developer looking at just one of these what the
> implications are to change the value of it. It might break or change
> the expected behavior; we saw this with the JDT extensions for the
> CNF. It's not clear to others that the JDT extension has that
> priority because it's coordinating with Resources, and Working Sets
> extensions.
>
> I don't believe I'm allowed to modify the extension for M7 as we're
> past freeze, so I recommend that for 3.4 we adopt a tactical solution
> to add well-labeled new priority values (e.g. the current values:
> lowest, lower, low, normal, high, higher, highest to insert "slightly
> higher", "not quite highest", etc to fluff the levels enough for 3.4)
> to the enumeration, and for 3.4+, we can add the more flexible long
> term solution where you can declare exactly what extensions you're
> coordinating with. Thoughts?
>
> -Michael
>
>
>
> From: bugzilla-daemon@eclipse.org
>
> To: Michael Elder/Cambridge/IBM@IBMUS
>
> Date: 04/24/2008 09:57 AM
>
> Subjec [Bug 228022] [common navigator] Need more priority levels to avoid
> t: clashes of content providers
>
>
>
>
>
>
>
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=228022> Product/Component: Platform / UI
>
>
>
>
> --- Comment #8 from Dimitar Giormov <dimitar.giormov@sap.com>
> 2008-04-24 09:58:46 -0400 ---
> There are not enough priorities in order to sort the elements in
> right order.
>
> As for the use case see:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=225647>
>
> Because of the high priority of jdt it leaves us only Higher priority
> available.
> Reducing the Web renderer priority leaves the projects in
> inconsistent state.
>
>
> --
> Configure bugmail:
> https://bugs.eclipse.org/bugs/userprefs.cgi?tab=email> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
>
>
>
>
(In reply to comment #9) > I'm not sure if it's a legal change, but one way would be to indicate this with > > "high/low" > > within the existing priority attribute, using the same labels, and updating the > sorters to be aware that the levels might be nested. It could even made to be > indefinitely recursive ("high/low/lower") so that layers of extensions could > build on existing relative priorities without really "inserting" themselves > into the layer below them. Existing "xxx" values would be interpreted as > "xxx/normal". I prefer this approach over a numerical priority as its intuitive > to understand what comparison is being made. It also maps to the notion of > various extensions coordinating within certain layers of the Eclipse projects. > > Thoughts? I think this is an acceptable solution for WTP JEE, especially if it still can be done for 3.4. Paul, can you comment, if this satisfies Dali? Absolutely it's satisfactory. In fact, it's a variation of a change I suggested. This way, we could actually specify (for instance) that all JEE content have priority "higher" and then we (JPA) could stratify ourselves further within that priority. If this is 3.4 requirement, then we need to act now... Yes, we'd like to see it in 3.4, if possible. (In reply to comment #12) > If this is 3.4 requirement, then we need to act now... Michael? We would have to come up with a patch today or tomorrow, at the latest. (In reply to comment #15) > We would have to come up with a patch today or tomorrow, at the latest. > I'm working on a patch for this now, can probably have it by today (my time -- I have the advantage of being on the west coast) :) So after looking at this for a while and starting to work on it, I'm pretty unenthusiastic about the proposed solution of nesting the word values. Doing this right will mean hitting several classes. Personally, I prefer the numbers. But I think the larger issue is the fact that certain code knows it's relative position to certain other code and they should be explicitly declared, and all of the rest of it should be decided with possibly some kind of categorization scheme. This kind of scheme could have categories like "utility actions", or "main manipulation actions", or something along those lines, as oppose to just some scale. So I think the long term solution is to remove the scale (does not matter whether it's numbers or high/low/etc), and replace it with something what works better. I'm reluctant therefore to make the "scale" more elegant (particularly at the investment of even a couple of days of work -- and especially this late in the cycle), as that will result it being embraced more and the real solutions being put off. I would prefer fixing up the scale to accommodate the immediate demand for more levels, which is fairly easy and lower risk at this point, and getting to the long term solution sooner. I propose to simply allow numeric values to be also specified, and designate the current enum values to correspond to something like highest = 100, high = 200, low 700, lowest = 900 (for all of the values). This should be a fairly simple change. Thoughts? (In reply to comment #1) > This is quite a major issue for the WTP project. It limits our options for > populating the content of navigator view. The referenced bug 225647 is marked as minor. Why are we even considering a fix at this late stage? I looked at the code (briefly) and I think that the smallest possible change would be to add a secondary priority as suggested by Paul (see comment 9), i.e. the current priority values become highest/normal, higher/normal, high/normal etc. For this, we would have to add a method getSecondaryValue() to Priority.java, deprecate the existing getLiteral() method, and change get(String) to parse compound strings (e.g. ones with slashes, or dots). NavigatorContentDescriptor needs a new method getPriorityObject() to return something of type Priority instead of int. Then, all known callers of Prioroty.getValue() and NavigatorContentDescriptor.getPriority() would have to be changed to also honor the secondary priority value. Note that the existing strings (highest, higher, high, ...) and ints (0,1,2,3,4,5,6) are API and cannot be changed to 100,200, ... without deprecating API and adding new API. (In reply to comment #19) > I looked at the code (briefly) and I think that the smallest possible change > would be to add a secondary priority as suggested by Paul (see comment 9), i.e. > the current priority values become highest/normal, higher/normal, high/normal > etc. For this, we would have to add a method getSecondaryValue() to > Priority.java, deprecate the existing getLiteral() method, and change > get(String) to parse compound strings (e.g. ones with slashes, or dots). > NavigatorContentDescriptor needs a new method getPriorityObject() to return > something of type Priority instead of int. Then, all known callers of > Prioroty.getValue() and NavigatorContentDescriptor.getPriority() would have to > be changed to also honor the secondary priority value. > > Note that the existing strings (highest, higher, high, ...) and ints > (0,1,2,3,4,5,6) are API and cannot be changed to 100,200, ... without > deprecating API and adding new API. > I think any way you slice it we are going to have to mess with the API. I understand that strictly speaking the constant values are part of the API, that is we should be able to support code already compiled that has those numbers embedded in it. However as a practical matter, I would argue that it's very unlikely that such code exists (it would have to be code that programmatically creates priority objects for some reason -- I would think that almost all, if not all uses of the CNF are through the extension point mechanism), and we would be safe in changing the values (but nothing else -- of course we would give the proper notices to this effect in the release notes). To be on the safe side, we could also deprecate the static Priority.get(int) method (since it does not seem to be used anyways). There is also the comment that references the numeric values in the Javadoc, but again, I think it's safe to just fix that comment to refer to the constants. So here is what I propose: 1) Change the values of the constants to their current value * 100 + 100. So HIGHEST becomes 100, HIGHER becomes 200, etc. 2) Change the Priority.get(String) to create a priority with the correct numeric value which will be the translation of the value of the string, or the string's numeric value. So you could specify "higher" or "203" as a value. 3) Deprecate getLiteral() and Priority.get(int). 4) Document in the appropriate extension point documentation that you can specify either a numeric value or one of the constants, and document the constant values. As I understand it, strictly speaking the extension point values are not considered API, so we have some flexibility. 5) WIth the above changes everything else should just work. This results in a fairly quick and dirty messy solution but with minimal changes (can you tell I used to be an engineering manager?). I think we do need to come up with a better solution for this problem in 3.5 that will make this priority stuff go away. Regarding a longer term solution: In thinking about the use cases, they are either "we know about this other thing, so let's say were we are wrt that thing", or we know some category of importance in general, based on the type of function, like a configuration function is less important than a main path update function. For the first case, we can have a mechanism that refers to the dependencies. For the second case, I think we can come up with a meaningful set of categories for these functions. And that that will be a good and useful abstraction. But this will take some thought. And I would be happy to drive this in the 3.5 timeframe. Boris, let me know how you want to go with this one. I will either do it your way (comment 19) or my way (comment 20), and I can get it done tonight. But I need to know soon. If I don't hear anything, I won't do anything. We would be OK with either change, FYI. Dimitar/Kayolan/Paul: I still haven't heard back from you why this is important for you. Please explain (in a couple of sentences at least) why we should do such a risky change this late in the game. How bad would it be to just do nothing for 3.4? (In reply to comment #18) > The referenced bug 225647 is marked as minor. Why are we even considering a fix > at this late stage? Just to be clear: fixing this (regardless of the approach taken) is going to require API changes, which have to be approved by the PMC. So you not only have to convince Francis and me that this is really important, you also have to convince the PMC that this is important enough to warrant an API change. Please let us know either way as soon as possible. Remember that tomorrow (Thursday) is our last day of development for 3.4 RC1. (In reply to comment #23) > Dimitar/Kayolan/Paul: I still haven't heard back from you why this is important > for you. Please explain (in a couple of sentences at least) why we should do > such a risky change this late in the game. How bad would it be to just do > nothing for 3.4? > Let me clarify why the current bug is with severity major and bug 225647 is with severity minor. Bug 225647 is where all begins. The Dali project (Paul), which is on top of the whole WTP stack, faces problems when they try to tune the appearance of the content they contribute to the common navigator. This is not a quite big issue for them and they could live with it in Ganymede. But in WTP we recognize this issue as a major one, because it might be faced also by other WTP adopters (we could think of Dali as a WTP adopter). There is a tendency that WTP adopter contribute more and more content to the common navigator. We would not like that they have this limitation in Ganymede. Therefore, we asked for the change if possible. In the course of discussion it became clear that API changes cannot be avoid. If this changes endangers the release of the Platform SDK, then we would not argue against postponing it for the next release. Just to reiterate Kaloyan's comments, our minor issue is that we can't seem to fit our content into the right niche in the project explorer. If we use one priority, we appear before things we should appear after, and using a lower priority we appear after things we should appear before. It's an annoyance. We thought the issue could be addressed by another contribution changing *their* priority. But it's a delicate dance of priority that turns out to be intractable to solve in this way. When you add more and more users onto the stack, it becomes a major issue. Having seen the above two comments I'm not convinced we should try more hackery to fix this for 3.4, especially with today being the last day and us having to mangle the API even more. Clearly the missing functionality (at least as identified in the above comments) is that one action wants to depend on the placement of another plugin's actions. That is, it wants to be "above" it in some way. We should come up with a real solution that allows these dependencies to be expressed and deprecates the current priority mechanism, as it does not scale nor does it really meet the need. I'm interested in working on such a solution for 3.5 and would certainly want to work with the people commenting on this bug as the use cases are interesting. I agree with Francis - I think it is too late to solve this in 3.4: - We are past the API and feature freeze, and a fix would be non-trivial (see comment 17). - We aren't even sure what is the best way to fix this now (see comment 21). - Even if we did put in a fix now, it would be a stop-gap measure and we need a long-term solution (see comment 20). - I do understand that the current situation leads to real issues for clients, but they appear to be minor at this point and they do not prevent you from shipping your project/product. It would be good if you could invest some of your time into this in the 3.5 cycle, for example in the form of discussions with me and Francis on the best way to solve this problem. Without your input, we might end up solving a different problem just because we don't have the experience of being a couple of levels further up the stack. I am assigning the bug to Francis based on his interest in working towards a long-term solution. Setting target milestone to 3.5. OK. I agree with the respect to the latest comments. Right Now, I today 3.4ing. And my current project cannot wait to 3.5. So I just rude patch it, sort it at point of computeOrdering by use CAPDComparator:) Created attachment 105827 [details]
simple priority sort patch
(In reply to comment #20) > > Regarding a longer term solution: > > In thinking about the use cases, they are either "we know about this other > thing, so let's say were we are wrt that thing", or we know some category of > importance in general, based on the type of function, like a configuration > function is less important than a main path update function. For the first > case, we can have a mechanism that refers to the dependencies. For the second > case, I think we can come up with a meaningful set of categories for these > functions. And that that will be a good and useful abstraction. But this will > take some thought. And I would be happy to drive this in the 3.5 timeframe. > There is a stab at categories: Addons Highlighted - Content to be more frequently accessed than the core content Core - The main focus of the object: Java, C++, PHP Addons Lowlighted - Most of the non-core additions Utility - Resources, common things at the lowest category (The addons highlighted/lowlighted names are stupid, but you get the idea). I'm interested in comments on this soon, as I would like to get a solution for this into M5 is that's possible. Stay tuned for a proposal of a dependency mechanism. I'd be really interested in a generalized dependency/priority mechanism. The 'priorities' bucket approach always runs into granularity issues (as is does here) and we don't seem to have a good replacement. I we can define a valid pattern here it has many areas where it could be reused; Drop target specifications for one and, in e4, the order of the part factories used in the renderer. The only glimmer I've had to a solution is to use the bundle dependencies to sort on so that if plugin "A" depends on plugin "B" then it can 'override' something in "B" (like a drop target for example) since, by declaring a dependency, "A" knows about "B" so it can be expected to do the right thing. This isn't enough by a long shot though. (In reply to comment #34) > I'd be really interested in a generalized dependency/priority mechanism. The > 'priorities' bucket approach always runs into granularity issues (as is does > here) and we don't seem to have a good replacement. I we can define a valid > pattern here it has many areas where it could be reused; Drop target > specifications for one and, in e4, the order of the part factories used in the > renderer. > > The only glimmer I've had to a solution is to use the bundle dependencies to > sort on so that if plugin "A" depends on plugin "B" then it can 'override' > something in "B" (like a drop target for example) since, by declaring a > dependency, "A" knows about "B" so it can be expected to do the right thing. > This isn't enough by a long shot though. > Eric, I plan to start working on these things tomorrow for M6, of course they would be CNF specific, possibly to migrate downward later. Do you have any thoughts on my just doing the categories above as well as the above mentioned dependency mechanism? Francis, not at all. You have a problem (and a solution) for an issue that exists -now- so go to it. This looked to be a good place to raise the general issue since the bug's header was such a good statement of the actual issue...;-). Perhaps we should start a discussion (on the e4 list?) about what a more scalable solution to this problem might look like, what do you think ? Ok (and at the very last minute before API freeze), here is what I decided to do about this: I have added a "appearsBefore" attribute to the actionProvider extension element. This allows you to specify that you want your actionProvider to appear before something else. I think this should solve your problem of being able to be at a higher priority than a specified other actionProvider. I considered adding the categories as mentioned above, but upon reflection felt it would cause a lot of problems in how they worked with the existing use of the priority. These issues need to be thought through before we extend the API further for this. Released to HEAD, N20090309-2000, I20090308-2000, 35M6 the problem we explained was in content providers not in action provider, maybe similar solution could help. Am I right Paul? I'm an idiot, I should have looked more carefully at this. I think I can add this to the content providers (and I should for M7). It's not strictly an API change, and it should have been in the content providers in the first place. And I know you guys have been waiting for this since 3.4, so I really want to get it in. Boris, would it be OK to do this for M7? I will do it today or tomorrow. Would this be fixed by applying the attached patch? (In reply to comment #41) > Would this be fixed by applying the attached patch? No, I have to make the fix. I plan to do that today or tomorrow (since tomorrow is the last day). It's hard to tell whether you need PMC approval if I haven't seen the fix. I will be doing a build submission Sunday afternoon Eastern time, for the 2000 warm-up build. Created attachment 133258 [details]
Patch of just the definition changes
This does not provide the implementation, but this shows the changes to the extension point. I'm still working on the implementation and will either have it done today, in which case (if approved) it can go it, or we wait until 3.6.
I really want to try for this one in 3.5 though, it will help a great deal with the support of large numbers of layered navigator content exentions which seems to be where we are going.
Created attachment 161171 [details]
Patch of incomplete current work
This is just to hold the code somewhere until I finish it, please ignore.
Created attachment 161238 [details]
Submitted patch
This implements the appearsBefore attribute in the navigatorContent element which is similar to what was previously done for the common action providers.
This now allows an NCE to indicate it's more important than some other NCE (in that it appearsBefore it), the priority mechanim is a secondary calculation.
Released to HEAD 3.6M6 Verified in I20100310-1800, the associated tests pass Verified in I20100310-1800, the associated tests pass We have a problem with this behavior. It appears that "appearsBefore" causes a problem if the referenced content provider is not in the workspace. (see bug 349108) We do not want to depend on the plugin declaring that content type. I would prefer that the reference to that content provider would not *require* that dependency. Should this be logged in a separate bug? (In reply to comment #50) > Should this be logged in a separate bug? Yes, please. |