Bugzilla – Full Text Bug Listing
| Summary: | [regression] error messages from remote are not shown in SystemView | ||
|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> |
| Component: | RSE | Assignee: | David McKnight <dmcknigh> |
| Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> |
| Severity: | major | ||
| Priority: | P1 | CC: | ddykstal.eclipse, kjdoyle, xuanchen |
| Version: | 2.0 | Flags: | mober.at+eclipse:
pmc_approved+
dmcknigh: review+ |
| Target Milestone: | 2.0.1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 196662 | ||
| Bug Blocks: | 210563 | ||
| Attachments: | |||
|
Description
Martin Oberhuber
Indeed this is one of the known bad side-effects of 196662. Because we don't have an ISystemViewElementAdapter.exists() API we can't check after doing the query whether a system message object is due to a non-existant object or not. We couldn't add API in 2.0.1 so the completion of that defect was deferred for 3.0. To avoid this may be complicated but we'll discuss. Created attachment 79220 [details]
patch to check if remote object exists in job via query
I've attached a patch for this. After the add() callback comes back with either no children or a message object, we start a job that queries the parent object and checks whether the original object still exists. If it does exist, then we leave the original message object alone. If it doesn't exist, then we replace the children with the results of the new query.
Martin, could you please review.
The problem with this patch is, that CheckExistenceJob is a UIJob so we'll have the Adapter.getChildren() query in the UI thread which we want to avoid as per bug 196662. I think that we need CheckExistenceJob extends Job for the first part of the tasks to do (i.e. query the children); and, once the children are known, either Display.asyncExec() or CheckExistenceUIJob extends UIJob for the rest of the work (i.e. update the SystemView with the new data). This should not be too hard to do, just take the things done in the current runInUIThread() method, put the first frew statements into the background job, and pass the resulting array into the UIJob. Created attachment 79222 [details]
ammended patch to use Job and do ui-specifics outside of job
How's this updated patch? Still a couple of comments:
* I think that the CheckExistence Job should not be a system job. It queries
the remote just as if the user had expanded the parent (which shows
"pending.." in the view and forks off the job).
Users should be able to cancel this job if they need, and they need to see
the job in the job list in order to be able to do so.
* Just some syntactic sugar -- I think the code would be more readable if
the UpdateView Runnable would not be a locally declared class. Having to
call the Constructor can be avoided by making the children[] variable final
(_parentItem and others might also need to be final for this):
final Object[] children = adapter.getChildren(_context, monitor);
if () /*...*/
else {
Display.getDefault().asyncExec(new Runnable() {
public void run() {
TreeItem[] items = _parentItem.getItems();
/*...*/
}
});
}
* Using equality ("==") in contains() does not seem to be right, because the
remote object could have been freshly created or could have changed its
status since the previous query (e.g. a read-only file could have become
writable, or similar).
Of course you can check with == first, but in case it fails you'll also
need to check with equals(), and if that also fails, you need to try
adapting to an IRemoteObjectIdentifier or ISystemViewElementAdapter,
and comparing their getAbsoluteName().
Since from your array of returned objects, at most one will actually be
the right one, most of them will not be equal and you'd need to do the
absolute name test on all of them; which means there's no need to check
equals and == , and you can go with the getAbsoluteName() check right
away.
Created attachment 79225 [details]
ammended patch for better readability and use of paths in comparison
I've ammended the patch further considering your comments. Again? More comments:
1. I find the direct manual handling of items in the SystemView problematic.
1a) What about other views (e.g. TableView) showing the same element that
we just found out has been deleted. They should also be updated, right?
1b) What if other elements (that we currently choose to remove and replace)
had been expanded, or selected by the user. Such expansion and
selection will be lost.
I think that at least for now, it is better to just fire another refresh
event and live with the fact that it will re-query what it just has
queried. That's unfortunate, but remember it's only done in the (IMHO
unlikely) case that user asked for refresh of an object, and it turned
out that this very object had been deleted.
2. What should we do if our CheckExistenceJob returns an errorObject for
the parent as well -- once again it could be because the parent did
not exist, or because some other error occurred.
Personally, I think that we do NOT need to recursively update its
parent (original grandparent) because the original user's context
was a child which is now gone -- so we get good results for the original
refresh query, showing an error object (or the original object gone)
is good-enough feedback, there is no need to refresh the grandparent.
I think your current patch already does that, though there might be
a need for exception handling, I'm not sure.
3. I think that your contains() method needs some NullPointer checks, since
we cannot control what kinds of Objects our extenders give us, so it
could happen that getAdapter(ISystemViewElementAdapter.class) returns
null, or that getAbsoluteName() returns null.
BTW, does it work for dstore, now that contains() uses the absolute name?
4. I think that if ISystemViewElementAdapter.supportsDeferredQueries()==false
we might need to perform special handling. Such adapters are typically
written to be queried in the display thread, so I think that in this case
we should perform the query (and check contains() on the children) just
like in the Job, but do it in the same thread.
Actually, the safest in this case might be to just fire the event like
your previous code, since it _will_ run in the same thread. At any rate,
we should test this scenario with the Local file system.
Minor optimization:
5. In case we find out that object X has indeed been deleted, and object X
is currently selected in the SystemView, I think we should select the
parent of X when done with the Refresh event.
This seems pretty easy:
* getSelection() before starting the CheckExistenceJob
* If selection is one item only, and it is the element that has been
deleted, fire a EVENT_REFRESH_REMOTE with the parent item to be
selected.
(In reply to comment #9) > More comments: > 1. I find the direct manual handling of items in the SystemView problematic. > 1a) What about other views (e.g. TableView) showing the same element that > we just found out has been deleted. They should also be updated, right? > 1b) What if other elements (that we currently choose to remove and replace) > had been expanded, or selected by the user. Such expansion and > selection will be lost. > I think that at least for now, it is better to just fire another refresh > event and live with the fact that it will re-query what it just has > queried. That's unfortunate, but remember it's only done in the (IMHO > unlikely) case that user asked for refresh of an object, and it turned > out that this very object had been deleted. Okay..well, I guess that brings up back to what we discussed and wanted to avoid in the first place...but you're right, it's not likely to occur frequently. > 2. What should we do if our CheckExistenceJob returns an errorObject for > the parent as well -- once again it could be because the parent did > not exist, or because some other error occurred. I think what would happen is that the add() method would see the error for the parent, and, as in the original case, we would schedule a new CheckExistenceJob for that case. > 3. I think that your contains() method needs some NullPointer checks, since > we cannot control what kinds of Objects our extenders give us, so it > could happen that getAdapter(ISystemViewElementAdapter.class) returns > null, or that getAbsoluteName() returns null. Yeah, I guess we can't assume that extenders will use the proper adapter. > BTW, does it work for dstore, now that contains() uses the absolute name? Yes, this works for dstore in either case. (In reply to comment #11) > Okay..well, I guess that brings up back to what we discussed and wanted to > avoid in the first place...but you're right, it's not likely to occur > frequently. I also think that firing the event makes the change a lot safer. Because we currently fire the event unconditionally; so, all the change will do is make a query first and then fire the extra parent refresh event or not. This seems safe to me, though we should test properly once the patch is in a form that we can all accept: * On each connection type (Local, dstore, ssh, ftp) 1. Drill down a directory. Delete outside RSE. Refresh the directory. 2. Drill down a directory. Mark it non-readable. Refresh the directory. 3. Drill down a directory. Delete it, and its parent outside RSE. Refresh. - Same as above but with a TableView showing the element being refreshed. - Same as above but with a TableView showing the parent of the element refr. * On dstore-processes: Start a process, select it, kill it outside RSE, Refresh it * Select a shell node and refresh I'd hope that accounts for all scenarios. I can test ssh and ftp, so if you could do dstore and Local? I'll also ask Tobias to test with our commercial product tomorrow. Created attachment 79234 [details]
patch ammended yet again...based on comments
Here's yet another update to the patch containing:
-null pointer checks for the adapters and paths
-using the refresh event instead of the direct add method
I went through basic tests with dstore, ssh and local using the patch:
* On each connection type (Local, dstore, ssh, ftp)
1. Drill down a directory. Delete outside RSE. Refresh the directory.
-works for me
2. Drill down a directory. Mark it non-readable. Refresh the directory.
-works for me
3. Drill down a directory. Delete it, and its parent outside RSE. Refresh.
-works for me
- Same as above but with a TableView showing the element being refreshed.
-updates the table view with the error message (which is okay with me)
- Same as above but with a TableView showing the parent of the element
-works for me
Created attachment 79238 [details]
Improved patch for !supportsDeferredQueries()
Here is an updated patch:
* Keeps current thread in case !supportsDeferredQueries()
* Supports contains() check via equals() -- even if no adapters available
I chose not to implement the selection preserving stuff, since the event of actually trying to refresh a deleted item is so unlikely.
From my point of view, this patch could be the one. If your tests run OK, please commit! Thanks!
I don't see any problems with your updates so I'm okay with this. Shall I commit? I've committed the patch to cvs. |