Community
Participate
Working Groups
Here is the list of affected plugins, with number of leakage problems detected: 8 org.eclipse.rse.files.ui 1 org.eclipse.rse.shells.ui 18 org.eclipse.rse.ui
First fix in org.eclipse.rse.ui was relatively simple: [225506][api] make RSEAdapter API This is non-breaking since simply non-API type is moved into API package.
Two more could be fixed really easily by getting rid of the following API: RSEUIPlugin.getSystemViewAdapterFactory() SystemAdapterHelpers.getSystemViewAdapterFactory() IMHO these are a hack anyways and should be done away, any concerns?
SystemTeamViewCategoryPropertyPage#getCategoryNode() could be made private, ok?
These two should perhaps be API to fix two more - ok, DaveD? SystemFilterPoolDialogInterface SystemFilterPoolDialogOutputs
Reassigning to Dave M, as I'm not in today.
Created attachment 94779 [details] patch with some refactorings for dealing with API leaks I'm putting this out as patch because it's a lot of changes and I'd feel more comfortable with a review.
Dave -- I loaded the patch and found several compile errors. Are you sure you exported the whole thing? In particular it looks like there needs to be a method in SystemViewForm that returns a TreeViewer. There are several getSystemView calls against SystemViewForm that expect this. -- Dave
Created attachment 94792 [details] updated patch I missed the files.ui plugin when creating the patch. Can you try now?
I see that you also committed some changes for this bug already, e.g.: * Added org.eclipse.rse.ui.actions/ISystemViewMenuListener * Made SystemTeamViewCategoryPropertyPage#getCategoryNode() private * Removed SystemAdapterHelpers.getSystemViewAdapterFactory() Whereas you chose not to remove RSEUIPlugin#getSystemViewAdapterFactory(), why? Can you summarize what API changes you're making i.e. what new classes or interfaces become API, and which ones are no longer API? On a quick patch review, I see the following: * Extracted ISystemTableViewColumnManager * Extracted ISystemFilterPoolWrapperInformation * Made SystemTableView#getOpenToPerspectiveAction() private ++++ OK for me * Made SystemTableView#getShowInTableAction() private ++++ OK for me * Added ISystemTree#setAutoExpandLevel(int level); * Added ISystemTree#addDoubleClickListener(IDoubleClickListener listener); * Added ISystemTree#isExpandable(Object elementOrTreePath); * Added ISystemTree#expandTo(Object parentObject, Object remoteObject); * Added ISystemTree#expandTo(String filterString); * Added ISystemTree#addFilter(ViewerFilter filter); * Added ISystemTree#addSelectionChangedListener(ISelectionChangedListener); * Made SystemViewForm API ===== IS THIS NECESSARY OR CAN WE HAVE A FACTORY METHOD RETURNING and ISystemTree? * Made SystemViewAdapterFactory API ===== I'M NOT HAPPY WITH THIS!!! * Made SystemResourceSelectionInputProvider API ++++ OK for me * Made SystemResourceSelectionForm API ===== IS THIS NECESSARY? OR CAN WE HAVE A FACTORY METHOD RETURNING ISystemTree? * Made SystemAbstractAPIProvider API ++++ OK for me * Made SystemFilterPoolDialogOutputs API ++++ OK for me * Made SystemFilterPoolDialogInterface API ++++ OK for me * Made SystemFilterPoolDialogInputs API ++++ OK for me * Made SystemNewFilterWizardNamePage API ===== IS THIS NECESSARY? * Made SystemFilterPoolWizardInterface API * Made SystemTestFilterStringDialog API ===== IS THIS NECESSARY? * Made field SystemBaseSubMenuAction#subMenu private ++++ OK for me * Made field SystemSelectRemoteFileOrFolderForm#ps private ++++ OK for me * Extracted ISystemSelectRemoteObjectAPIProvider -- IS THIS NECESSARY? We already have API for SystemAbstractAPIProvider, and for ISystemViewInputProvider * Made SystemSelectFileTypesAction API ===== IS THIS NECESSARY? NOT HAPPY...
Perhaps it would make sense to see if we can agree on a set of refactorings right away and commit these, and then discuss the remaining ones in the context of another patch? - A base for discussion could be my comments (OK / Not OK / question) on the previous comment.
I applied the updated patch and still get compile errors. It looks like the imports in SystemSelectRemoteObjectAPIProviderImpl did not make it into the patch and so are causing problems. I fixed those and now have a clean compile. I will be attaching a new patch with those changes. It also appears to function correctly. I'm wondering, along with Martin, if we can review and commit these changes a bit at a time.
Created attachment 94827 [details] an updated patch including the import fixes
Thanks DaveD. I'd appreciate if you could give your comments about what refactorings are good for you or need further investigation. Our first goal should be to come up with a set that we all can agree on, so you should probably start inspecting those that I marked "OK for me".
> * Made SystemViewForm API ===== IS THIS NECESSARY OR CAN WE HAVE A FACTORY > METHOD RETURNING and ISystemTree? SystemViewForm is reusable composite that a number of RSE dialogs make use of. I'm not sure, but may make sense to allow others to use it too. > * Made SystemViewAdapterFactory API ===== I'M NOT HAPPY WITH THIS!!! I don't mind moving this back if we don't like it as API. > * Made SystemResourceSelectionInputProvider API ++++ OK for me > * Made SystemResourceSelectionForm API ===== IS THIS NECESSARY? OR CAN WE HAVE > A FACTORY METHOD RETURNING ISystemTree? I thought it might be useful as a reusable control. Currently we use the form for our remote resource dialog. I'm on the fence with this. > * Made SystemAbstractAPIProvider API ++++ OK for me > * Made SystemFilterPoolDialogOutputs API ++++ OK for me > * Made SystemFilterPoolDialogInterface API ++++ OK for me > * Made SystemFilterPoolDialogInputs API ++++ OK for me > * Made SystemNewFilterWizardNamePage API ===== IS THIS NECESSARY? I'll have to look at this again. I can't remember why I made that API. > * Made SystemFilterPoolWizardInterface API > * Made SystemTestFilterStringDialog API ===== IS THIS NECESSARY? SystemResolveFilterStringAction extends SystemTestFilterStringAction and needs to set the protected dlg field with it's own extensio of SystemTestFilterStringDialog, SystemResolveFilterStringDialog. > * Made field SystemBaseSubMenuAction#subMenu private ++++ OK for me > * Made field SystemSelectRemoteFileOrFolderForm#ps private ++++ OK for me > * Extracted ISystemSelectRemoteObjectAPIProvider -- IS THIS NECESSARY? > We already have API for SystemAbstractAPIProvider, and for > ISystemViewInputProvider SystemAbstractAPIProvider is extended by a number of classes but I'm not sure if all the APIs for ISystemSelectRemoteObjectAPIProvider would be applicable to them. > * Made SystemSelectFileTypesAction API ===== IS THIS NECESSARY? NOT HAPPY... I'm on the fence about this. The class is extended and appears to be intended for that but I'm not sure if it makes sense outside of rse.ui.
I think that if in doubt, we should avoid making anything API. We can always add API later but we can hardly revoke anything that we've made API. Therefore I'd rather be restrictive and NOT ADD SystemViewForm SystemViewAdapterFactory SystemResourceSelectionForm SystemNewFilterWizardNamePage SystemTestFilterStringDialog SystemSelectFileTypesAction API for now, if we can avoid it. Regarding SystemTestFilterStringDialog, can we go the other way round and remove the *Action classes from our API which want to use it?
> > * Made SystemTestFilterStringDialog API ===== IS THIS NECESSARY? > SystemResolveFilterStringAction extends SystemTestFilterStringAction and needs > to set the protected dlg field with it's own extensio of > SystemTestFilterStringDialog, SystemResolveFilterStringDialog. Actually, I can deal with this since the dlg field is actually not necessary. So SystemTestFilterStringDialog doens't need to be API.
Created attachment 94875 [details] patch with updates I've changed this so that the following are now non API: SystemTestFilterStringDialog SystemViewForm SystemResourceSelectionForm SystemSelectFileTypesAction SystemViewAdapterFactory SystemNewFilterWizardNamePage Can you review?
You still make the following API, I'm not sure if this is required: SystemAbstractAPIProvider SystemResourceSelectionInputProvider SystemRemoteFileSelectionInputProvider perhaps we could do with all of these being non-API, at least for now. In addition to those being mentioned by you in comment #17, your patch also makes the following non-API which used to be API: SystemTestFilterStringAction But basically I'm ok with the change. I'd propose committing it and potentially revising again early next week. Tobias could you please also check if your code requires any of the classes that we're proposing to remove from API space, thanks.
I've committed the current patch for now.
Current committed status as of today: API Added: ------------ * + (ui.actions) ISystemViewMenuListener * + added ISystemFilterPoolWrapperInformation#addWrapper(...) (2 methods) * + added ISystemTree#setAutoExpandLevel(int level); * + added ISystemTree#addDoubleClickListener(IDoubleClickListener listener); * + added ISystemTree#isExpandable(Object elementOrTreePath); * + added ISystemTree#expandTo(Object parentObject, Object remoteObject); * + added ISystemTree#expandTo(String filterString); * + added ISystemTree#addFilter(ViewerFilter filter); * + added ISystemTree#addSelectionChangedListener(ISelectionChangedListener); * + added SystemRemoteResourceDialog#getSystemTree() * + added SystemRemoteResourceDialog#isPageComplete() * + added SystemSelectRemoteFileOrFolderForm#getSystemTree() * + added SystemSelectRemoteFileOrFolderForm#getSelectedParent() * + (ui.filters) Made SystemFilterPoolDialogOutputs API * + (ui.filters) Made SystemFilterPoolDialogInterface API * + (ui.filters) Made SystemFilterPoolDialogInputs API * + (ui.filters.dialogs) Made SystemFilterPoolWizardInterface API * + (ui.view) Extracted ISystemSelectRemoteObjectAPIProvider -- NECESSARY? * + (ui.view) Extracted ISystemTableViewColumnManager * + (ui.view) Made SystemAbstractAPIProvider API * + (ui.view) Made SystemResourceSelectionInputProvider API -- NECESSARY? API Removed: ------------ * private SystemTeamViewCategoryPropertyPage#getCategoryNode() * removed SystemAdapterHelpers.getSystemViewAdapterFactory() * private SystemTableView#getOpenToPerspectiveAction() * private SystemTableView#getShowInTableAction() * private SystemBaseSubMenuAction#subMenu * private SystemFileFilterStringEditPane#testAction * private SystemFileFilterStringEditPane#typesAction * private SystemFileFilterStringEditPane#getSelectTypesAction() * private SystemSelectRemoteFileOrFolderForm#ps * private SystemSelectRemoteFileOrFolderForm#tree * removed SystemSelectRemoteFileOrFolderForm#getSystemViewForm() --> use getSystemTree() instead! * private SystemSelectRemoteFilesForm#getSelectTypesAction() * internal (ui.actions) SystemTestFilterStringDialog * removed SystemSelectRemoteFileOrFolderDialog#getSystemViewForm() * private SystemTableView#getShowInTableAction() * private SystemNewFilterWizard#namePage * private SystemNewFilterWizard#createNamePage() * private SystemRemoteResourceDialog#_form API Changes which are not reflected by additions/removals above: ---------------------------------------------------------------- * RSEUIPlugin#getAdapterFactory() now returns IAdapterFactory No Change (yet) after discussions: ---------------------------- * Not made SystemViewForm API ===== IS THIS NECESSARY OR CAN A FACTORY METHOD RETURN an ISystemTree? * Not Made SystemViewAdapterFactory API ===== I'M NOT HAPPY WITH THIS!!! * Not made SystemResourceSelectionForm API ===== IS THIS NECESSARY? OR CAN A FACTORY METHOD RETURN ISystemTree? * Not made SystemNewFilterWizardNamePage API ===== IS THIS NECESSARY? * Not made SystemSelectFileTypesAction API ===== NECESSARY? NOT HAPPY... * Not made SystemTestFilterStringDialog API I'm ok with these changes, but I'd like to discuss the following: ----------------------------------------------------------------- * Does ISystemSelectRemoteObjectAPIProvider need to be API? * Does SystemResourceSelectionInputProvider need to be API? * Does SystemRemoteFileSelectionInputProvider need to be API? -- it seems to be internal to SystemRemoteFileDialog
Two more instances of leakage remain: * SystemProcessFilterStringEditPane leaks SystemTestFilterStringAction --> Make the edit pane "internal" ? * SystemFileTreeAndListGroup leaks ResourceTreeAndListGroup --> Make SystemFileTreeAndList Group "internal" ?
Created attachment 95073 [details] patch with additional changes as per martin's last comment In this patch I've dealt with the following: SystemResourceSelectionInputProvider -> no longer api SystemRemoteFileSelectionInputProvider -> no longer api ISystemSelectRemoteObjectAPIProvider -> no longer api ISystemResourceSelectionInputProvider -> added to api
The changes from comment #22 are really simple in terms of code, since the relevant classes don't have any extra dependencies to the outside. The question is really, are we OK with moving these out of API. I certainly am, since these are being used in dialogs only; and I don't see these dialogs being extended in other ways. But that's probably more for IBM people to assess. What I'm actually more interested, is getting the remaining leakages from comment #21 fixed.
Created attachment 95075 [details] updated patch I had to change this to put ISystemSelectRemoteObjectAPIProvider back as api.
Created attachment 95076 [details] patch for processes.ui I'm not sure if I included this in my earlier patches so I'm including it here as it resolves the leaks in processes.ui.
(In reply to comment #21) > Two more instances of leakage remain: > * SystemProcessFilterStringEditPane leaks SystemTestFilterStringAction > --> Make the edit pane "internal" ? > * SystemFileTreeAndListGroup leaks ResourceTreeAndListGroup > --> Make SystemFileTreeAndList Group "internal" ? For the process filter string edit pane, I think the last patch addresses that. For SystemFileTreeAndListGroup, I can make this internal however the tool still complains about the leak.
I'd like to finish this off. I'm good with the recent two patches, what about others? Can we commit them?
I've committed the fixes to cvs.
One more remains: RSEUIPlugin#getShowPreferencePageActions() This is just a remnant from the former remoteSystemsPreferencesPages extension point, and I think the method can safely be moved into SystemCascadingPreferencesAction. Actually, the method's been deprecated in 2.0 already and I had kept it in RSEUIPlugin just as a reminder that this should really be implemented by the new command/handler/menus framework. I propose moving this away, and adding a new bug for implementing the prefsPages via menu framework. Xuan you've been dealing with dynamic menus, do you think this is something you can handle? Comments?
(In reply to comment #29) Moving RSEUIPlugin#getShowPreferencePageActions() into "internal" class SystemCascadingPreferencesAction for now. Committing change: [225506][api][breaking] Move RSEUIPlugin#getShowPreferencePageActions() to internal Bug 186769 alrady exists for migrating the former remoteSystemsViewPreferencesActions extension point into an org.eclipse.ui.handlers extension that can be referenced by an org.eclipse.ui.menus extension. Now we still have 1 leakage in RSE UI due to bug 220999.
(In reply to comment #30) > Now we still have 1 leakage in RSE UI due to Correction: leakage is due to bug 221138.
After making SystemSelectRemoteFileOrFolderDialog internal, the SaveAsDialog now leaks non-API SystemSelectRemoteFileOrFolderDialog via extends. This should be fixed, either by making SaveAsDialog "internal" and only exposing ISaveAsDialog via a factory method; or, by changing SaveAsDialog to extend SystemRemoteFileDialog instead. DaveM or Xuan can one of you look at this?
I checked in the change for SaveAsDialog.
I would propose naming the factory FileDialogFactory rather than SaveAsDialogFactory, such that it can be used as a factory for more advanced dialogs in the future if we want. I'd also propose naming the methods "makeSaveAsDialog" by convention, since they create a new SaveAsDialog instance each time (rather than re-using or sharing an existing instance). Or would we ever want to try and re-use an instance? class FileDialogFactory { public ISaveAsDialog makeSaveAsDialog(Shell); }
Committed the changes according to Martin's suggestion.
Created attachment 95491 [details] Patch adding Javadoc for FileDialogFactory I've added Javadoc for FileDialogFactory (already committed). I'm wondering if the factory methods are already sufficient? - Right now it seems impossible to get a "Browse for Folder" dialog with a custom title, because SaveAsDialog#setTitle(String) is not exposed in ISaveAsDialog. I like it that ISaveAsDialog is a very thin interface, because it can theoretically be implemented also without SWT (e.g. in a text-only-UI or in a browser), and it allows us to migrate to SystemRemoteFileDialog / SystemRemoteFolderDialog under the hood more easily in the future. But we might need to have the factory methods more versatile, e.g. have a single factory method only: /** * @param title Title for the dialog. Use <code>null</code> for a standard * title, e.g. "Browse for File" or "Browse for Folder". * @param fileMode <code>true</code> for allowing file selection, * <code>false</code> to force folder selection. */ ISaveAsDialog makeSaveAsDialog(Shell shell, String title, boolean fileMode); Looking at this signature, does it even make sense to allow file selection for "save as"? Does it make sense to allow folder selection for "save as"? Where is this currently being used?
I found two places where this method is called (inside my workspace): One is in SystemUpdateConflictAction#widgetSelected() ISaveAsDialog dlg = FileDialogFactory.makeSaveAsDialog(getShell()); Another one is in a IBM plugin: ISaveAsDialog saveAsDialog = FileDialogFactory.makeSaveAsDialog(shell, LpexResources.message(LpexMessageConstants.MSG_FILEDIALOG_SAVEAS) ); for the later case, I haven't found a scenario to invoke it.
(In reply to comment #37) > I found two places where this method is called (inside my workspace): > One is in SystemUpdateConflictAction#widgetSelected() > ISaveAsDialog dlg = FileDialogFactory.makeSaveAsDialog(getShell()); > Another one is in a IBM plugin: > ISaveAsDialog saveAsDialog = FileDialogFactory.makeSaveAsDialog(shell, > > LpexResources.message(LpexMessageConstants.MSG_FILEDIALOG_SAVEAS) > ); > for the later case, I haven't found a scenario to invoke it. Xuan, for the later case, have you tried using File->save as when the remote systems lpex editor is in focus?
Actually, there is some mistake in my previous comment (comment 36). What I meant was that for the first case, I did not find a scenario. For the second case, I did see this dialog been invoked.
I think I'm in favor of closing this bug if you IBM folks are OK with it. The ISaveAsDialog / FileDialogFactory API is really thin, so even if it doesn't quite what we want now, we can easily evolve it in the future. Perhaps the one thing I'd like to see is marking ISaveAsDialog @noimplement such that we can evolve it in the future.
PS All API Leakage Warnings are gone now, even on a Clean Build. Thanks for all the hard work!
I'm in favour of closing this one now.
Fixed for M6!
