Community
Participate
Working Groups
I'm reviewing the p2 operation API this morning, and the first thing that struck me is the repository add/remove jobs. There are eight classes involved here (add+remove for metadata+artifact+colocated, plus two abstract classes). Since these operations are not long running, jobs and the progress monitoring aren't actually needed. The resulting API doesn't look any easier for clients to use: Using operation API: String label = ...;//come up with a label IProgressMonitor monitor = ...;//setup a progress monitor AddMetadataRepositoryOperation op = new AddMetadataRepositoryOperation(label, session, location); op.setNicknames(new String[] {"My Repository"}); op.runModal(monitor); Using repo manager API: IMetadataRepositoryManager mgr = session.getMetadataRepositoryManager(); mgr.addRepository(location); mgr.setRepositoryProperty(location, IRepository.PROP_NICKNAME, "My Repository"); The operation API does add an extra event (OperationEndingEvent), but the underlying repository manager also fires events, so I'm not sure why the extra event is needed. This new event has much less information than RepositoryEvent (only the location, whereas repo event has location, repository type, nickname, enablement, and kind of event (add/remove/change)). For most usecases it seems this some of this extra detail will be needed by clients anyway. I might be missing something, but these eight classes seem to add a lot of code that isn't really doing very much and makes client code unnecessarily complicated by the addition of (unused) progress monitoring.
Yeah, there's a lot of bulk here but something is needed. The "extra event" is needed to provide batching. If we fixed bug 234199 in the repo managers I could throw all this away... What's needed is a way to say: I am adding (or loading, or removing)... this repository. I don't care about *anything* that happens with repositories until this operation is done. Without this kind of batching, the UI over-responds to all the repo additions and changes that happen when references are encountered during a load. The symptoms include: - losing a newly typed site in the combo (Bug 276884, Bug 282176) - spurious flashing as composites get added (Bug 276192) So I think we need some kind of API here and I'm open to a different shape. One could argue that we only need this workaround during load repo, but I was trying to be a bit more general and not provide API that knows about the underlying implementation.
If it's just the event you need, perhaps there could just be convenience methods somewhere for adding "user" repos: class ProvisioningSession { ... addRepository(URI location, String nickname, int repositoryType); } Where repository type is either metadata, artifact, or both.
There's some other stuff being done in these operations with respect to managing colocated repositories, but after thinking about it a bit more, I see now that there is a better place to put this code. So I think there are two things to be done: 1) we now have a RepositoryTracker that manages the differences in colocation vs. working with only metadata or artifact repositories. I think I can push signalling into the methods on tracker (today it already has to return specific operations, so it can just do the work inline). This will let us punt the repository operations in the API. 2) the operation events have to be API so listeners can respond accordingly. However the way I've defined it was kind of "fuzzy" (a generic event with a generic item that indicates what happened underneath). Instead I think the event should be a RepositoryOperationEvent so that it's clear that this only involves repository events. And rather than signal a generic "item" instead someone can signal the underlying event or null to indicate that the whole world has changed. This will clean things up quite a bit, and also make it much more clear what the signalling is trying to accomplish (batching of repo events). I may keep the non-API operations in the admin UI around, since it deals with bulk addition and removal of multiple repositories, and it's useful to have that code in one place. I'll ping here when I have something new for you to look at.
Changes from comment 3 committed to HEAD. The repository jobs are gone, with the relevant methods pushed into the RepositoryTracker instead. John, can you take another look and comment? With respect to the batching of events. I made the eventing more specific to repositories and give clients the ability to supply an underlying repository event at the end if one makes sense. (In many cases it does not, but for simple adds it does). When someone "ends" an event, I have session.signalOperationComplete(RepositoryEvent event, boolean ignore) I'm not quite happy with this signature, but can't think of a better one. There are really three cases to cover: - signalOperationComplete(null, false) the operation is over, update everything because multiple repos may have been added or removed. - signalOperationComplete(null, true) the event is over, don't update anything (because this was a background operation, such as a background load of repositories), and we don't want to update the UI at all. - signalOperationComplete(aRepositoryEvent, false) we know that what we did was localized to a particular repo and are providing an event that describes it I wanted to have a single parameter method, but couldn't find a good way. I considered introducing a NullRepositoryEvent to represent the "ignore" case, or a MultipleRepositoryEvent to represent the "update everything" case, but that seemed contrived.
(In reply to comment #4) > Changes from comment 3 committed to HEAD. of course not, they are commited to the api branch!
I've decided to move the batching completely into the UI, so I'm taking one more whack at moving some code around and will ping here when done. (The events would still be API, but they'd be in the UI, which feels better since there are cheats about what events we send based on UI knowledge).
fixed in the branch. - I've moved all the repository manipulation methods into RepositoryTracker, it became clear as I did this that this is where they belonged. - This let me get rid of the operation events in the operations bundle and move them to the ui bundle. In fact, since we don't have to cross the bundle boundaries anymore, these events can be private (although the concept of batching appears in the API of the UI via the signalStart/signalComplete methods). I'm very happy with this. Marking this bug fixed. John, I dare you to find to chaff in the operations bundle!
changing milestone to M5. The p2 api branch will be merged back into HEAD early in M5
verified on I20100126-0100, Win7 via source inspection
