Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 252912

Summary: SystemRemoteFileDialog shows Local contents even when specifying a SystemType
Product: [Tools] Target Management Reporter: Kevin Doyle <kjdoyle>
Component: RSEAssignee: David McKnight <dmcknigh>
Status: RESOLVED FIXED QA Contact: Martin Oberhuber <mober.at+eclipse>
Severity: normal    
Priority: P3 CC: kjdoyle, rdibugs
Version: 3.0.1   
Target Milestone: 3.1 M3   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Bug Depends on:    
Bug Blocks: 253769    
Attachments:
Description Flags
patch to determine valid connection based on system types
none
updated patch none

Description Kevin Doyle CLA 2008-10-30 21:39:21 EDT
We are using SystemSelectRemoteFileAction and setting the systemType to IRSESystemType.SYSTEMTYPE_ISERIES_ID.  However when the dialog loads up it shows the Local connection filters.  It should be showing the filters for the connection specified in the combo box.

SystemSelectRemoteFileAction.createDialog(Shell) does:

SystemRemoteFileDialog dlg = null;
		if (dlgTitle == null)
		  dlg = new SystemRemoteFileDialog(shell);
		else
		  dlg = new SystemRemoteFileDialog(shell, dlgTitle);

SystemRemoteFileDialog does:

new SystemRemoteFileSelectionInputProvider()

Which does:

// choose random host
		ISystemRegistry registry = RSECorePlugin.getTheSystemRegistry();
		IHost[] hosts = registry.getHosts();
		if (hosts != null && hosts.length>0) {
			_connection = hosts[0];
		}

hosts[0] is the Local connection.
Comment 1 David McKnight CLA 2008-10-31 16:21:56 EDT
Created attachment 116654 [details]
patch to determine valid connection based on system types

This patch checks the system types to determine whether a connection is valid or not.

Kevin, does this help?
Comment 2 Kevin Doyle CLA 2008-11-04 12:07:04 EST
No, I still get the contents of the Local file subsystem when it displays the dialog even though it's not an iseries connection.
Comment 3 David McKnight CLA 2008-11-04 12:19:07 EST
(In reply to comment #2)
> No, I still get the contents of the Local file subsystem when it displays the
> dialog even though it's not an iseries connection.
> 

Do you call setSystemType() on the action?  For example:

IRSESystemType systemType = RSECorePlugin.getTheCoreRegistry().getSystemTypeById(IRSESystemType.SYSTEMTYPE_ISERIES_ID);
			action.setSystemType(systemType);

Comment 4 Kevin Doyle CLA 2008-11-04 12:32:14 EST
(In reply to comment #3)
> Do you call setSystemType() on the action?  For example:
> 
> IRSESystemType systemType =
> RSECorePlugin.getTheCoreRegistry().getSystemTypeById(IRSESystemType.SYSTEMTYPE_ISERIES_ID);
>                         action.setSystemType(systemType);
> 

Basically, we extend SystemSelectRemoteFileAction and in our constructor after calling super() we do:

IRSESystemType systemType = RSECorePlugin.getTheCoreRegistry().getSystemTypeById(IRSESystemType.SYSTEMTYPE_ISERIES_ID);
		super.setSystemType(systemType);
Comment 5 David McKnight CLA 2008-11-04 12:46:29 EST
Created attachment 116975 [details]
updated patch

Kevin, can you try again with this patch?
Comment 6 Kevin Doyle CLA 2008-11-04 13:30:38 EST
Works well and don't see any problems with the patch diff.

Thanks Dave.
Comment 7 David McKnight CLA 2008-11-04 14:17:54 EST
I've committed the fix to cvs.  Does this need to be backported?
Comment 8 Kevin Doyle CLA 2008-11-04 14:21:04 EST
Yes please.  We are still going through the list of which ones we want backported, but this is on the list.
Comment 9 David McKnight CLA 2008-11-04 14:33:42 EST
(In reply to comment #8)
> Yes please.  We are still going through the list of which ones we want
> backported, but this is on the list.
> 

I've created bug 253769 for the backport.
Comment 10 Martin Oberhuber CLA 2008-11-06 12:21:55 EST
In SystemResourceSelectionInputProvider line 62, please do NOT compare IRSESystemType objects with ==:
    hostType == type

Somebody could wand to implement a proxy to a systemType, which has the same ID but returns different Properties for some reason. PLEASE, compare with .equals()!! FindBugs should issue an error for == comparisons, and I think even Eclipse can issue an error for this.

In getSystemViewRoots() -- line 143 -- you can run into an NPE if none of the hosts are valid. You should return 
   new Object[0]
in that case.

Reopening to get fixed.
Comment 11 David McKnight CLA 2008-11-06 12:46:42 EST
(In reply to comment #10)
> In SystemResourceSelectionInputProvider line 62, please do NOT compare
> IRSESystemType objects with ==:
>     hostType == type
> 
> Somebody could wand to implement a proxy to a systemType, which has the same ID
> but returns different Properties for some reason. PLEASE, compare with
> .equals()!! FindBugs should issue an error for == comparisons, and I think even
> Eclipse can issue an error for this.
> 
> In getSystemViewRoots() -- line 143 -- you can run into an NPE if none of the
> hosts are valid. You should return 
>    new Object[0]
> in that case.
> 
> Reopening to get fixed.
> 

I wouldn't have expected the proxy to systemType.  Anyway, I've updated the fix in cvs.
Comment 12 Martin Oberhuber CLA 2008-11-06 13:06:19 EST
expected or not, equals() should be the standard in Java.
Use == only in very special cases and with precautions.
Comment 13 Martin Oberhuber CLA 2008-11-06 13:09:58 EST
What about the potential NPE in line 140 of SystemResourceSelectionInputProvider ? Note that an NPE here is a potential regression because before your fix, the "restrict to" would not have been honored.
Comment 14 David McKnight CLA 2008-11-06 13:39:46 EST
(In reply to comment #13)
> What about the potential NPE in line 140 of
> SystemResourceSelectionInputProvider ? Note that an NPE here is a potential
> regression because before your fix, the "restrict to" would not have been
> honored.
> 

Updated to return new Object[0] in the case where no connections are available.