Community
Participate
Working Groups
Created attachment 122475 [details] Refactor ElementQueryDescriptor to include a perform method The ElementQueryDescriptor is used by the UI, and it does a nice job of combining all the things that describe a query. Can I suggest that we add a perform method to the descriptor, and let the descriptor actually delegate the query itself. This way we can control how the queries are run, and possibly add other filters / mechanisms can be put in a single spot. We could also use this to profile different queries. As we start to move away from the collectors that convert the data from IUs to View Model things, we could add the converting code right here. I will open another bug and do that for: o InstalledIUCollector o MetadataRepositoryManagementCollector o ProfileElementCollector Patch is attached and can be reviewed.
this makes sense to me. Unfortunately I'm pretty ill right now and don't want to decide anything until my brain comes back. It might be next week before I can look at this and the other query changes.
Created attachment 123335 [details] Updated Patch The old patch got stale. this fixes that
> As we start to move away from the collectors that convert the data from IUs to > View Model things, we could add the converting code right here. I will open > another bug and do that for: > o InstalledIUCollector > o MetadataRepositoryManagementCollector > o ProfileElementCollector > > Patch is attached and can be reviewed. I'm getting a bit confused by the granularity of the bugs, and I think I'd like to consider the ElementQueryDescriptor changes here as just one part of the general problem, which is that we need to move the element wrapping outside of the collectors. I haven't had a chance to review the current code and see whether the parent element would know how to wrap the children or if that knowledge is still specific inside the query provider. If the latter, then decisions made today in the query provider that choose a collector instead would choose a specific descriptor that wraps a certain way (or plug the wrapper into the descriptor, etc.). In which case the name of the class ElementQueryDescriptor will need to be changed, since it's not just a data structure anymore. I think we need to get that full picture before committing any changes here. Or is there something specific you can already accomplish with just this change?
This patch simply adds a perform method to ElementQueryDescriptor, and instead of: descriptor.queryable.query(descriptor.query, descriptor.collector, monitor); you call: descriptor.perform(monitor).
yeah, it changes the visibility/adds getter/setter, etc. too. I looked at it, just wondering if this buys you anything in advance of the bigger problem (moving the wrapping so we can get rid of special collectors)
Created attachment 123447 [details] Updated Patch I have created a single patch that removes all the QueryElementCollectors. Collectors that did querying (such as LatestIU) are now structured as Queries. Collectors that transformed elements from one model to another (such as AvailableIUCollector) are now "Transformers". There are some new test cases for these things, as well as updated to the previous test cases.
Updating the bug title to better reflect the work that was done here.
I really like the direction here! (disclaimer - I glanced at it for 10 seconds, so I reserve the right to nitpick later). I will try to get this in post-M5 and 3.4.2 fires. (Once HEAD is reopened, could be as early as next week).
assigning to me to put on radar for early M6.
*** Bug 260105 has been marked as a duplicate of this bug. ***
I'm looking at this patch. There are some naming things and other changes I would want to make, but I found a hole in this strategy that we need to sort out first. With the current patch, category merging doesn't work. Only the content from the first category of the same name is shown, rather than the merged content. This really points out an equality inconsistency that we have with categories, some of Thomas' concerns about categories being special, and the work Andrew is proposing in bug 261104. IU's are considered equal if their id and version are equal. And collectors collect the query results in a set. Prior to this patch, the UI collectors saw each category IU coming in, and if they detected a category to be merged, they handled this as part of the accept. The UI operated on each query match as it was being iterated. With the patch, the entire query is run first and then the UI operates on the results. So seemingly "equal" category IU's are treated as the same, and only one of them is placed in the result set, *even though each of them refers to a different set of IU's (and is from different repositories).* So the first IU category wins and others are deemed duplicates and never stored in the query results. I *think* that the changes proposed in bug 261104 will solve this problem, because categories from different repos would be qualified by the repo somehow and versions would also be considered. In the meantime, this patch can't be applied...but I'll post an updated patch with some discussion of naming changes and other things that I'd want to do before applying it.
Created attachment 124103 [details] patch with name changes, etc. This patch is where I was heading until I found the category merging problem. The main change from Ian's patch involved naming. One big subjective change, but some other changes that I think are needed regardless... The big subjective change: I like Transformer as a generic name in a framework, and if the intention were to truly transform the IU's or other query results in some way, I would like it. Since this isn't a core query concept, but just something for the UI elements, I renamed it to ElementWrapper because I think the concrete name makes the UI code easier to understand. We are just wrapping the IU's (sometimes rejecting or merging them), but we never operate on them directly or transform them into something different. This meant changing the class name as well as the method names to involve more wrapping and less transforming, as well as changing test cases to test wrappers vs. transformed strings. (Even if we ended up some day with a Transformer in core, I would choose to name all the UI classes wrappers). Other changes: - I didn't like the use of the accept method name in the wrappers/transformers. A few times I got confused because the old doc implied that the query would end if accept answered false. Since there's no short circuit happening in the wrapper iteration, I figured we should use a different name completely - the test classes were still named after *Collector so I changed those - there were a couple places where accept was made public vs. protected in the wrapper framework so I made them all protected
Thanks so much for reviewing my patch and updating it! > > Other changes: > - I didn't like the use of the accept method name in the wrappers/transformers. Great! I like the new names > A few times I got confused because the old doc implied that the query would > end if accept answered false. Since there's no short circuit happening in the > wrapper iteration, I figured we should use a different name completely Good idea > - the test classes were still named after *Collector so I changed those Thanks > - there were a couple places where accept was made public vs. protected in the > wrapper framework so I made them all protected Perfect I don't really understand the problem with the categories. In the current version, the collector does not get reset between repositories, so if you had two repos (or a composite repo), and each one had "seemingly equal category IUs", then wouldn't the same problem arise? That is, the first one would be in the result set and the second one would *not* get added? (In accept, it checks if the category already exists).
> I don't really understand the problem with the categories. In the current > version, the collector does not get reset between repositories, so if you had > two repos (or a composite repo), and each one had "seemingly equal category > IUs", then wouldn't the same problem arise? That is, the first one would be in > the result set and the second one would *not* get added? (In accept, it checks > if the category already exists). The difference is that the UI's category collector sees each result as it goes by (in the accept method) before deciding that it won't get added. So the collector noted the duplication and merged the contents of equal IU's into the UI element. In the patch, the query is run first and then the UI wrapper runs over the results. So the duplicate IU just disappears, because only one of the category IU's is in the result set. By the time the UI wrapper gets a chance to merge, it's too late... If we have a mechanism by which these "same-named" IU's are not equal, then the UI would still see both in the result set and could merge in its post-processing.
Susan, could we use a list (instead of a set) to collect the results? We actually use this approach in CompoundQueryables. Also, I noticed in your updated patch that my changes to QueryableMetadataRepository are not there. The changes make use of Compound Queryable. What this omitted by accident, or did you see a problem with this approach? Finally, I noticed a problem with shouldWrap / wrap method in the CategoryElementWrapper. In the current system, we always add IUs to the referredIUs list, however, in my patched version, we only do this if shouldWrap returns true. The code that adds IUs to the referredIUs list should be moved to the shouldWrap method.
(In reply to comment #15) > Susan, could we use a list (instead of a set) to collect the results? We > actually use this approach in CompoundQueryables. We intentionally changed to using a set in bug 254955. There was discussion in that bug of making it configurable in a collector whether duplicates are retained, but I don't see that as the way to solve this problem, since the real problem is that those IU's shouldn't be equal. > > Also, I noticed in your updated patch that my changes to > QueryableMetadataRepository are not there. The changes make use of Compound > Queryable. What this omitted by accident, or did you see a problem with this > approach? I didn't understand what the changes were for and forgot to ask...they didn't seem relevant to the collector replacement, they seemed like other cleanup and at one point I removed them while testing the rest of it. Then I found the merging bug and forgot to address that part of it. > > Finally, I noticed a problem with shouldWrap / wrap method in the > CategoryElementWrapper. In the current system, we always add IUs to the > referredIUs list, however, in my patched version, we only do this if shouldWrap > returns true. The code that adds IUs to the referredIUs list should be moved > to the shouldWrap method. > ack. If you want to prepare a patch for the last two things you mention, that's fine. Otherwise I'll look when we go to apply this.
(In reply to comment #16) > (In reply to comment #15) > > Susan, could we use a list (instead of a set) to collect the results? We > > actually use this approach in CompoundQueryables. > We intentionally changed to using a set in bug 254955. There was discussion in > that bug of making it configurable in a collector whether duplicates are > retained, but I don't see that as the way to solve this problem, since the real > problem is that those IU's shouldn't be equal. ok, makes sense to me. > I didn't understand what the changes were for and forgot to ask...they didn't > seem relevant to the collector replacement, they seemed like other cleanup and > at one point I removed them while testing the rest of it. Then I found the > merging bug and forgot to address that part of it. This was for things like LatestIUVersion. In this case, you want the Latest of the Latest. (Not just the latest from each repo, but rather, combine the results). The CompoundQueryable does this, and the changes just use the compoundqueryable. I will update the patch for the last two things. As well, we should probably put a regression test together for the Category thing.
Created attachment 124345 [details] Updated patch I have updated the patch to include: 1. The change to QueryableMetadataRepositoryManager 2. The change to CategoryElementWrapper to properly add the items to the referredUI list in shouldWrap.
I was thinking a bit more about this bug, and we may have to use a list to get the results. We are currently talking about fixing this bug by changing the publisher, but there are likely p2 metadata repositories that already exist where categories have not been given a proper Unique ID. I think by default collecting as a set makes sense, but in the case of categories, we can collect as a list. I was pretty careful in my other query changes not to assume we always collect as a list. Actually, I already coded for the case of List collectors in CompoundQueryable. If we don't need to worry about backwards compatibility(that is, metadata repos that don't properly make their category ids unique), then we can fix this in the publisher. Otherwise, I would be happy to create a ListCollector (or make it configurable in our existing one).
>Otherwise, I would be happy to create a ListCollector (or make > it configurable in our existing one). The backwards compatibility issue is a good point. And I would go back to the idea from bug 254955 and make it configurable in the collector. But something feels funny here. This bug is all about getting rid of custom collectors so that in the long run, we won't have to pass a particular collector at all, right? The whole idea was that you would just get an iterator. And yet we are talking about introducing a list vs. set collector (or a switch) to solve this problem. When we go to the very next (get rid of passing in a collector), then what? We would need to specify in the query how to treat duplicates?
changing dependency to a more specific bug.
Fixed in HEAD >20090305. - updated/committed the patch (it was stale because of some reorg I had done with the QueryableRepositoryManagers) - added a test case for the "merged categories" behavior - updated the UI merging code to merge based on category name, not id (the UI part needed for bug 264389. I don't think we had a separate UI bug for that).
