Community
Participate
Working Groups
Many operations in Eclipse use a StructuredSelection instead of the base selection, and there's a lot of if(selection instanceof IStructuredSelection) messages. If a helper method in HandlerUtil could be provided to add this functionality then it would simplify adding handlers. The code could look like: /** * Return the current structured selection. * * @param event * The execution event that contains the application context * @return the current structured selection, or <code>null</code>. */ public static ISStructuredelection getCurrentSelection(ExecutionEvent event) { ISelection selection = getCurrentSelection(event); if(selection instanceof IStructuredSelection) { return (IStructuredSelection)selection; } else { return null; } } The same thing could be done for the checked equivalent: /** * Return the current structured selection. * * @param event * The execution event that contains the application context * @return the current structured selection. Will not return <code>null</code>. * @throws ExecutionException * If the current selection variable is not found. */ public static IStructuredelection getCurrentSelectionChecked(ExecutionEvent event) throws ExecutionException { Object o = getVariableChecked(event, ISources.ACTIVE_CURRENT_SELECTION_NAME); if (!(o instanceof IStructuredSelection)) { incorrectTypeFound(event, ISources.ACTIVE_CURRENT_SELECTION_NAME, ISelection.class, o.getClass()); } return (IStructuredSelection) o; }
I
New Gerrit change created: https://git.eclipse.org/r/64243
The suggested patch adds a getCurrentStructuredSelection method which returns an empty collection if a structured selection is not available. This is similar to the implementation of StructuredViewer but different from getCurrentSelection in HandlerUtil which returns null. IMHO following the StructuredViewer pattern makes it simpler for caller,e.g, org.eclipse.ltk.internal.ui.refactoring.actions.DeleteResourcesHandler. But if someone disagrees we can change it to return null.
If there are no objects again my approach, I commit the changes this week.
Gerrit change https://git.eclipse.org/r/64243 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c88a1a1e9173fe23d299aa1b1f3290e2e630bedc
In Java8 returning an Optional<ISStructuredelection> would feel more natural but null most likely aligns better with the current API
ah i see you return empty - I'm not a fan of that!
Just to give you an impression how Optional would feel! HandlerUtil.getStructuredSelection(...).ifPersent( System.err::println )
(In reply to Thomas Schindl from comment #7) > ah i see you return empty - I'm not a fan of that! This is consistent to the Viewer API.
Yes but I think you should be consistent with the HandlerUtil-API!
(In reply to Thomas Schindl from comment #10) > Yes but I think you should be consistent with the HandlerUtil-API! I had the same concerns issued in Comment 3. The problem is that this would make the calling code more complex. I contacted Dani about this last week to see if he has a strong opinion about this. I don't mind changing the new method to null, if more people think a (badly designed but) consistent API within a class is better than an easier to use API. Again, this new HandlerUtil API is similar to the Viewer API.
(In reply to Lars Vogel from comment #11) > (In reply to Thomas Schindl from comment #10) > > Yes but I think you should be consistent with the HandlerUtil-API! > > I had the same concerns issued in Comment 3. The problem is that this would > make the calling code more complex. I contacted Dani about this last week to > see if he has a strong opinion about this. > > I don't mind changing the new method to null, if more people think a (badly > designed but) consistent API within a class is better than an easier to use > API. Again, this new HandlerUtil API is similar to the Viewer API. In this case it is fine since we already have a method that allows you to get that information. Returning EMPTY prevents that every user of the new API has to do a 'null' check, which is usually not interesting for clients. However, it should be clearly mentioned in the first sentence of the Javadoc. In addition, this method cannot be used since it is not declared 'static'. I've fixed both issues with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d9ef2d749733bb76ab0a0fe6d534cae027c8b081
Thanks Dani for the clarification and Tom for the input.
> In this case it is fine since we already have a method that allows you to > get that information. Returning EMPTY prevents that every user of the new > API has to do a 'null' check, which is usually not interesting for clients. as is outlined in a java8 API you'd mark this with returning an Optional but I think this would make the API alien to the other code, the problem is that with returning EMPTY I don't know if there's really a selection provider who is pushing an empty selection or the current selection simply isn't a IStructuredSelection but I don't feel very strong on this.
(In reply to Thomas Schindl from comment #14) > > > In this case it is fine since we already have a method that allows you to > > get that information. Returning EMPTY prevents that every user of the new > > API has to do a 'null' check, which is usually not interesting for clients. > > as is outlined in a java8 API you'd mark this with returning an Optional but > I think this would make the API alien to the other code, the problem is that > with returning EMPTY I don't know if there's really a selection provider who > is pushing an empty selection or the current selection simply isn't a > IStructuredSelection but I don't feel very strong on this. If you feel strong on this you call the old API ;-).
