Community
Participate
Working Groups
Build ID: 3.0 M7 In Europa, if setAllowFolderSelection is set to false in SystemSelectRemoteFileAction class then user will not be able to select anything else other than file. In Ganymede, the setAllowFolderSelection api is removed and there is no replacement apis for restricting selection to files only. The only workaround I have is to set the selectionValidator. And to implement IValidatorRemoteSelection interface, you need to return the SystemMessage which is a bit kludgy for ISVs. We should try to standardize on ONE API for file/folder Selection, and revisit SystemSelectRemoteFolderAction and SystemSelectRemoteFileAction . More information: The addition of customViewFilter in SystemSelectRemoteFileAction is also confusing; since there are both viewFilters and customerViewerFilter as member fields and now viewerFilter is not being used at all anymore. And the addViewerFilter api is not marked as depreciated yet.
It looks like the work on bug 220995 has not been completed properly, because the requested API (setAllowFolderSelection()) has been removed without marking the bug as [breaking]. It might be that the setSelectionValidator() method, or the setCustomViewerFilter() method can be used instead in order to restrict the possible selections to a certain type (file, folder, particular file types) - but these are not documented as the new way of doing things. Xuan please make sure that this regression is fixed ASAP.
If I remembered correctly, we discussed this during one of the committer meeting. Our conclusion was to remove the un-used APIs then as part of the process to move away from a deprecated class, and added if necessary later on (it is always easier to add API then removing). Since there is request from the community, I will add setAllowFolderSelection() API in, probably using IValidator approach.
Ok, but we should have marked the bug as [breaking] while we removed API. That way the Community might have screamed up earlier. Thanks for working on re-adding the API.
PS can you please mark the bug as [breaking] now, mentioning all the methods that have been removed, such that it's properly documented.
I marked bug 220995 [breaking] last night. It already had the list of all the APIs been removed.
Created attachment 100712 [details] fix for this problem
Martin, please review the patch. Thanks.
(In reply to comment #5) > I marked bug 220995 [breaking] last night. It already had the list of all the > APIs been removed. The setAllowFolderSelection method, which has been removed and what we are talking about, is still not listed there.
I don't like it that your action now implements IValidatorRemoteSelection. You're again (totally unnecessarily) changing the API of the action, by adding an interface and a method that were not there in 2.x. I think that the Validator should be a separate class of its own, either as an anonymous nested class, or if you will as a re-usable exposed API class. Your choice, but don't implement the interface right in the action. A separate class could be a "FileOnlySelectionValidator". Do you have to add new PII or can you re-use any existing PII?
Created attachment 100975 [details] Updated patch. Updated according to Martin's commit. Please view. Thanks.
I was not be able to find existing PII that I could reuse. So I add a couple new strings. We should have another chance to add PII.
Patch is better, though I would suggest two changes: 1.) Use a "static" nested class instead of the inner class for the Validator, and have the Validator use a Constructor that takes the allowFolderSelection as a boolean arg to store it locally. This will allow for later instance creation, and easier refactoring of the nested class to a separate stand-alone class if necessary. 2.) Create the validator instance not in the Action constructor, but later (when also the Dialog is constructed).
Did you compare what the "new" action without folder selection looks like, compared to the "old" action without folder selection? A unit test would be nice though that's probably not easy since we're dealing with UI elements here.
Created attachment 100999 [details] Updated to use static inner class.
I committed the change.
Can you explain why you call dlg.setSelectionValidator(selectionValidator); twice in the createDialog(Shell) method? Also, I think you must add Javadoc that the setFolderSelection(boolean) method only works if user does not provide his own selection validator via setSelectionValidator(). A yet better idea would be to modify the RemoteFileSelectionValidator constructor: RemoteFileSelectionValidator(boolean, ISelectionValidator previousInChain) and modify its isValid() method as follows: Whenever it would return null in the current code, it should really return fPreviousInChain.isValid(...) -- that way a selection will only be considered valid if BOTH the RemoteFileSelectionValidator AND the previousInChain think that it's valid. Reopening since I think that without this change, we still have a breaking change because setAllowFolderSelection() would not work as before, although I'm not sure.
Created attachment 101130 [details] updated patch. Please review. Thanks.
Looks good! Thanks. Though the code in RemoteFileSelectionValidator#isValid() could certainly be simplified, e.g. get rid of unnecessary "==true"; try to have one occurrence of the if(previous...) code at the end of the method only. Perhaps not as pretty as it could be, but from reading the code it looks like it should work. Got some code to actually try it out? One idea for a Unit Test here would be that the Unit Test method does not instantiate the Action class directly, but a local subclass of it which overrides (some) methods. That way, the logic in the validator could be checked by the Unit test.
Comment on attachment 100999 [details] Updated to use static inner class. The new patch is an add-on to this one, so this one is not obsolete.
Fix is adding pii.
I tested with client code in one of the IBM plugin, and I tried setting the flag to both true and false. It works fine in both cases. I really don't have time to add Junit for it now (I am not suppose to work on openRSE at all except issue related to our porting at this point). I will see if I could add this later.
checked into cvs.
Thanks!
Close it.
