Bugzilla – Full Text Bug Listing
| Summary: | [regression] AssertionFailedException when connecting to New Connection | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Kevin Doyle <kjdoyle> | ||||
| Component: | RSE | Assignee: | David Dykstal <ddykstal.eclipse> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||
| Severity: | major | ||||||
| Priority: | P1 | CC: | ddykstal.eclipse, dmcknigh | ||||
| Version: | 3.0 | ||||||
| Target Milestone: | 3.0 M7 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | 218304, 225911 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Kevin Doyle
That's yours Dave, I think... The adapter might actually be null due to the lazy adapter loading from bug 218304 ... if that's the case then the issue would only be reproducable once while the files.ui bundle is not yet loaded. After it is loaded, the issue should no longer appear. One can check whether files.ui is active as follows: Window > Show View > Other > PDE Runtime > Plugin Registry Filter active plugins only Filter name=org.eclipse.rse.files.ui On the other hand, Kevin Doyle says: I think the assert might just not be valid. I'm not sure but the Pending... node wouldn't have an ISystemViewElementAdapter, so it should be null. I put the assertion in since I believed that the adapter would never be null. I've removed the assertion and have it now returning the former result. Created attachment 97635 [details]
Patch improving element comparison
I took the opportunity to analyze the code in more depth, and found found it to be correct but with only three minor flaws:
1. While we adapted to ISystemViewElementAdapter for the hashCode(),
we'd adapt to ISystemDragDropAdapter for the equals() in SystemRegistry.
Now that should usually never be a problem, but it is inconsistent.
2. We only allow adaptable elements to be compared in the element map. Now
newer Eclipse added IAdapterManager, to support registering adapters even
for objects that are not IAdaptable themselves. I thought that we should
start picking this up, although is nowhere used in RSE yet so registering
adapters for non-IAdaptable objects will almost certainly not bring the
expected results.
3. We'd have to retrieve the ISystemRegistry instance, do an instanceof check
and a class cast -- just in order to call a method that could also be
static and thus directly accessible.
At the same time, I saw that SystemRegistry#isSameObjectByAbsoluteName() was badly inefficient:
* it would ask for adapters even if the absolute name was already given
* it would make wild String concatenation just to compare subsystems,
even if the absolute names were already unequal
* it would make string concatenation, although simple instance comparison
of the subsystems would already suffice
See attached patch with my fixes. The patch should not change semantic behavior in any way (except allowing non-IAdaptables to behave properly), but when testing I think that the SystemView already felt to be performing a bit better - but that was just a personal feeling and not actually measured.
Patch is already committed:
[228774] Improve ElementComparer Performance
The patch looks fine to me. |