Bugzilla – Full Text Bug Listing
| Summary: | [Model] Make BaseWorkbenchContentProvider and LabelProvider adapter manager friendly | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Jean-Michel Lemieux <jean-michel_lemieux> | ||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P5 | CC: | bokowski, charles, eclipse, gunnar, jeffmcaffer, michaelvanmeekeren, paulk, Tod_Creasey | ||||||
| Version: | 3.1 | Keywords: | helpwanted | ||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | stalebug | ||||||||
| Bug Depends on: | 118560 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Jean-Michel Lemieux
Created attachment 22119 [details]
patch to BaseWorkbenchContentProvider and LabelProvider
I'm willing to consider this, but only if the performance impact is negligible. We use these in many of the views. Do you have any feeling (or preferably measurements) showing the performance impact? I didn't re-run the Workbench performance tests, but they should catch any
performance issues. But for the record, let's have a look at the proposed
changes and compare them with the old code:
protected IWorkbenchAdapter getAdapter(Object element) {
IWorkbenchAdapter adapter = null;
if (element instanceof IAdaptable)
adapter = (IWorkbenchAdapter) ((IAdaptable) element)
.getAdapter(IWorkbenchAdapter.class);
if (adapter == null)
adapter = (IWorkbenchAdapter) Platform.getAdapterManager()
.loadAdapter(element, IWorkbenchAdapter.class.getName());
return adapter;
}
And compare it to the previous code:
protected IWorkbenchAdapter getAdapter(Object element) {
if (!(element instanceof IAdaptable)) {
return null;
}
return (IWorkbenchAdapter) ((IAdaptable) element)
.getAdapter(IWorkbenchAdapter.class);
}
For all existing views the first IAdaptable check would succeed and as it did
before. The failure case, which means that the adapters were broken, would mean
looking up in the adapter manager. What I think then is that this won't have a
performance impact on any existing viewers, but any new viewers that show
objects that don't implement IAdaptable would be a bit slower because of the
extra adapter manager call. But there is a catch, some implementations of
IAdaptable already called the adapter manager already, so this won't affect them
either.
> some implementations of IAdaptable already called the adapter manager already,
so this won't affect them either.
The typical case is for getAdapter to call
Platform.getAdapterManager().getAdapter(...), which doesn't activate plug-ins,
so it's still possible for the loadAdapter case to cause activation.
But I agree with your assessment for the existing resource-based viewers, so I'm
OK with this going in for RC2.
thanks MvM: +1? +1, but I'd like to see the before + after performance test results first just to make sure. I've decided not to add this in 3.2 due to performance concenrs. The loadAdapter case happens more frequently than anticipated due to getFont always looking for an IWorkbenchAdapter2, which resources don't adapt to. Created attachment 22564 [details]
Modified patch
This patch has the following changes to the one provided by JM:
- getAdapter2 is private (didn't want to introduce new API at this stage)
- added null check, since loadAdapter doesn't handle a null element, and the
Navigator's frame source was trying to get the label for null (previously did
not fail due to the instanceof check)
- various other Javadoc cleanup
Re "allows you to put objects into viewers that *don't* have to implement IAdaptable directly", note that this can be accomplished via the regular label- and content-provider mechanism. It's not required that they use BaseWorkbenchContentProvider and WorkbenchLabelProvider, and go through the IWorkbenchAdapter redirection. Will reassess in 3.2. *** Bug 120965 has been marked as a duplicate of this bug. *** Reassigning bugs in component areas that are changing ownership. Not for 3.2 Tod, as part of the fix for bug 156581, BaseWorkbenchContentProvider and LabelProvider should now be adapter manager friendly. However, getAdapter() is called instead of loadAdapter(). We can leave this bug open to discuss whether in this case, loadAdapter() is the better choice, like it was for bug 86362. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |