Community
Participate
Working Groups
AutomaticUpdateScheduler.IUProfilePropertyByIdQuery overrides perform to cache the profile. However, the cachedProfile field never actually gets set to anything other than null. I think we may want something like: public Collector perform(Iterator iterator, Collector result) { cachedProfile = getProfile(); Collector collector = result; try { collector = super.perform(iterator, result); } finally { cachedProfile = null; } return collector; }
I don't like overriding perform to do this because it forces this query to be a ContextQuery, which conceptually is not correct. I'd rather fix the bug in getProfile().
(In reply to comment #1) > I don't like overriding perform to do this because it forces this query to be a > ContextQuery, which conceptually is not correct. I'd rather fix the bug in > getProfile(). > I agree. The current implementation already overrides perform, that's why I did it this way.
> I agree. The current implementation already overrides perform, that's why I > did it this way. ok. I had forgotten it did this. I'll clean this up.
fixed in HEAD >20090120. Ian, I added some framework hook methods in MatchQuery so that subclasses can do pre and post-perform processing. I think it's reasonable to provide hooks for clients to do any optimizations they need before and after the perform, since the matching itself is final. I debated with myself awhile on how the hooks should work and the names for the methods (and looked for you online but you weren't there) and ended up with prepareToPerform() performComplete() I don't love these names, but couldn't think of anything better. prePerform() and postPerform() seemed too terse, and I didn't want the names to seem too JFace-ish (doPrepareToPerform). Another alternative would have been to implement a final framework method like doPerform() and let subclasses override perform to add their own pre/post hooks, but this seems to complicate the API for the more normal case. If you don't like this direction, let me know... (And I'm sorry if it's rude that I committed this, I just committed a bunch of different small fixes and after the fact realized a more polite route would be to let you see the patch first....)
.
I thought about doing this too, however, I worry that this may change the semantics of MatchQuery. There are places in p2 where people write their own loops to do the matching. (See LocalMetadataRepository#removeInstallableUnits). If a query that provides its own pre / post processing is used, there is no guarantee that these methods will be called. This is one of the reasons that I created MatchQuery and made perform final. MatchQueries have very well defined semantics. Using pre / post & perform, you could write getLatestIU as a match query, however, this query is clearly not a match query since the results depend on the "context".
Context queries were designed specifically to let people override perform (and indicate to the system that perform has been customized). Is there a reason why you don't want to use a context query in this case?
(In reply to comment #7) > Context queries were designed specifically to let people override perform (and > indicate to the system that perform has been customized). Is there a reason > why you don't want to use a context query in this case? > Several reasons: - context query is to be used when the entire context (all elements) are needed to make the decision. This is not true in this case, it is really just a simple match query with one performance optimization/cache. - the query is most appropriately a subclass of IUPropertyQuery, and it seems odd to copy/reimplement all the name/value stuff just so I can clear a cache after the perform. In other words, I don't really view context query as "the query to use if you need to override perform." I'd like us figure out a way to allow these hooks, but I understand the problem/looked at the example. Will look for you online, need to take a call...
reopening. After some discussion with Ian, it seems there are additional changes that need to go in. - we need to change LocalMetadataRepository.removeInstallableUnits(query, monitor) so that it just calls query perform rather than doing the iteration/has match itself. This way the hooks get called and also seems to be the right thing to do anyway. - Ian/Jeff like prePerform() and postPerform() as the method names - CompoundQuery calls hasMatch in order to short circuit compound queries, so it needs to call the hooks - we need to document MatchQuery so that it's clear that typical clients do not need to iterate and call hasMatch, but if they do, they need to call the hook methods appropriately sound right, Ian? I'll attach a patch
I like the idea of making isMatch protected. Also, the documentation should clearly state that each element in a MatchQuery is independent of each other element. I.e. elements cannot be matched based on their order, or any other relation to other elements.
(In reply to comment #10) > I like the idea of making isMatch protected. I had hoped for this, but I thought we couldn't do this since CompoundQueryable calls it. (I notice that other classes call it, also, I'll check these while fixing this in an attempt to eliminate the ones that aren't truly needed). > > Also, the documentation should clearly state that each element in a MatchQuery > is independent of each other element. I.e. elements cannot be matched based on > their order, or any other relation to other elements. >
(In reply to comment #11) CompoundQuery calls it, but it is in the same package. Although, in order to make the hierarchy work I had to create an IMatchQuery interface, and we can't put protected methods in an interface (something I really don't like, but C'est la vie). If you can look at classes that call IsMatch, I can fix up CompoundQuery and add a test case for pre / post.
Created attachment 123263 [details] Patch for the Pre / Post perform with compound queries and test cases ready for review.
Created attachment 123341 [details] Updated Patch
Created attachment 123342 [details] updated patch Found a small problem in the previous patch. ready for review.
fixed in HEAD >20090124. Changed name of bug since this became more general than just fixing the ui bug. I applied Ian's patch. In addition: - changed all callers of isMatch to just perform the query instead. This was mostly the UI queryables and one change to LocalMetadataRepository - added test cases for LocalMetadataRepository.removeInstallableUnits(query, monitor) to ensure that this code was working properly
Thanks Susan. I'm very happy with how this turned out :-).
