Community
Participate
Working Groups
Convert line delimiter actions are not always enabled when they should be (like when in an editor). It looks like the problem is in FileBufferOperationAction#selectionChanged. There is not a check for if you have an ITextSelection within an IStructuredSelection. There should be a check that if selection instanceof IStructuredSelection and one of the selections in IStructuredSelection is instanceof ITextSelection, the same thing should be done as if selection was just instanceof ITextSelection.
To clarify, the selection implementation in our WTP editors implements both interfaces, and if there's a rule against that, please tell us quick! :) Specifically, StructuredTextSelection extends TextSelection implements IStructuredSelection Perhaps the text selection could just be checked first? Can you recommend if there could/should be any "consistentcy statement" that "actions that work on both ITextSelection and IStructuredSelection (from resources navigators) should look for an honor ITextSelection first" ? Seems intuitive to me, but I haven't thought through all cases.
David, what you describe seems to differ from what Amy reports: Amy talks about nesting: "There is not a check for if you have an ITextSelection within an IStructuredSelection." ... There should be a check that if selection instanceof IStructuredSelection and one of the selections in IStructuredSelection is instanceof ITextSelection," ==> aStructuredSelection.getFirstElement() is an ITextSelection Amy, the text selection does not contain the part i.e. what should we do if the structured selection contains more than one text selection? Should we only handle the first one, would that fix your problem? David, you talk about a selection being both, a text selection and a structured selection. This sounds like an implementation inheritance which none of the actions or code in the base are handling i.e. viewers will look for IStructuredSelection and text-based code will check for ITextSelection. In my opinion it is OK for code to treat the two selection interfaces as being disjoint and hence can test one or the other first.
Actually, I just got how we handle selection mixed up. David's scenario is the same as mine: our selection implements both ITextSelection and IStructuredSelection. > This sounds like an implementation inheritance which none of the > actions or code in the base are handling i.e. viewers will look for > IStructuredSelection and text-based code will check for ITextSelection. I agree it looks like in most places, there is code handling/expecting either one or the other. And in those cases we are fine since we do implement both interfaces. The problem lies in when code is trying to handle both. > In my opinion it is OK for code to treat the two selection interfaces as > being disjoint and hence can test one or the other first. I kind of agree that if I am creating an action that can handle both types of selection, I'd like to dictate which selection I'd prefer first. However, I would also want to add that if the selection I prefer doesn't give me the information I need, then I will try to work with the other type of selection. I browsed through the code and the few places I did find where both ITextSelection and IStructuredSelection were being handled it was always: if (selection instanceof IStructuredSelection) do this to get info i need else if (selection instanceof ITextSelection) do that to get info i need if i have info i need, do it. It'd be better if it was: if (selection instanceof IStructuredSelection) do this to get info i need if (i dont have info yet && selection instanceof ITextSelection) do that to get info i need if i have info i need, do it.
What you outline would make sense if people expected that a selection can be both. While this is technically possible it is not what how he base handles it and also not how most of the clients will handle an ISelection. If it helps I can implement to support the nesting and you could nest your text selection into the structured selection. Note however, that you must not implement getFirstElement() to return itself since this would most likely result in an endless loop in some client code.
I'm not sure nesting, per se, is what we want. Maybe I'll need to study "base handliing" closer, but seems to me the problem is an implied assumption that "IStructuredSelection" means "check for resources" * else * "check for text selection". I think the fix is that the base handling should not make those implied assumptions, and "linearly" check for both, as Amy suggested in her pseudo code: if (selection instanceof IStructuredSelection) do this to get info i need if (i dont have info yet && selection instanceof ITextSelection) do that to get info i need if i have info i need, do it. Am I (again) missing your point?
The assumption is not as you outlined but it is much simpler: the code treats them as being either the one or the other i.e an ISelection is not intended to be both, a text and a structured selection and that's how the code handles it. If we took Amy's code someone else might claim that he'd like the ITextSelection to win or even pull out the information from both selections. Sorry but, we do not plan to add code that handles this.
(In reply to comment #6) > ... i.e an ISelection is not intended to > be both, a text and a structured selection and that's how the code handles it. > Ok, so you are saying that this is an undocumented aspect of ISelection ... "one or the other but not both"? > If we took Amy's code someone else might claim that he'd like the > ITextSelection to win or even pull out the information from both selections. > I agree, while we should not limit Eclipse from fear of what others *might* request, we should come up with some new proposed interface ... IStructuredTextSelection? ... where both are implemented, and then the expected treatment could be spec'd before being coded. We'll review current usages and see if we can come up with a concrete suggestion along these lines. Thanks.
>Ok, so you are saying that this is an undocumented aspect of ISelection ... >"one or the other but not both"? No I am not. The one who handles an ISelection knows/defines what kind of selections it can handle and what information he wants to extract from the ISelection. So far our thinking was that it is either text or structured but not both. Maybe you can explain a bit deeper how a valid i.e. not implementation inheritance based - structured text selection looks like. So far I think we're mixing circles with cubicles but maybe I'm wrong here and I'll have to open up the code.
(In reply to comment #4) > What you outline would make sense if people expected that a selection can be > both. While this is technically possible it is not what how he base handles it > and also not how most of the clients will handle an ISelection. The problem we're running into often is that the code is often structured this way: if (selection instanceof IStructuredSelection) { if(selection element is also an IResource) { do this to get info i need } } else if (selection instanceof ITextSelection) { do that to get info i need } // surprise, any IStructuredSelections of non-IResources leaves you high and dry! I don't think the problem has anything to do with us implementing both interfaces, I think it stems from a lot of assumptions about the elements in that structured selection being resources. We're finding patterns like this in Debug UI and Search UI. (In reply to comment #8) >Maybe you can explain a bit deeper how a valid i.e. not implementation >inheritance based - structured text selection looks like. Imagine that in Java source you've selected two adjacent methods--you have a single ITextSelection spanning the corresponding text but potentially an IStructuredSelection of multiple AST elements representing the two methods and anything in between them. That's the kind of selection we're providing.
Reopening because I don't want to dismiss this bug just yet. > I think it stems from a lot of assumptions about the elements in > that structured selection being resources. I agree. Why not have an IResourceSelection interface and check for that instead? IStructuredSelection is such a vague interface, but the way it is commonly used is to hold a bunch of the same *type* of different selections, not a bunch of different types of the same selection. If that makes any sense. For example, like Nitin said, perhaps you are in a java file and you have selected 2 methods. IStructuredSelection should contain the 2 AST elements representing the 2 methods. It should not contain the 2 AST elements, a text selection for length & offset and a resource selection for the java file the methods are in.
>I don't think the problem has anything to do with us implementing both >interfaces, I think it stems from a lot of assumptions about the elements in >that structured selection being resources. Well, if someone writes an operation that works on resources that's exactly the correct thing to do, isn't it. The code in discussion iterates over all elements in structured selection and asks for an adapter it does not simply do an instanceof check. Since your structured selection (e.g. the 2 AST elements representing the two methods) represents the same selection like the text selection then that's perfect: you request to change the code in FileBufferOperationAction.selectionChanged(...) in a way that it runs through the if (selection instanceof ITextSelection) branch, right? That branch does nothing else than getting the resource out of the text selection. For you to make that work simply register an IResource.class adapter on your structured selection elements (the 2 AST elements) and your just fine i.e. it will go into the IStructuredSelection branch, get the resource from the AST element via adapter and voilà. The resources are collected in a set and hence no duplicates. Note that in JDT UI we have the same situation i.e. Java elements in the structured selection and we get the resources out of them via IResource.class adapter. Of course you must pay attention not to send a structured selection if there's just a text selection which doesn't map to AST elements. >IStructuredSelection is such a vague interface, Fully agree, like I do with the last paragraph of comment 10. Other clients could for example put text selections into the structured selection. We don't handle this either, but it's valid. Please give the adapter mechanism a chance. If this doesn't work out for you I have no problem changing concrete code (like the one in FileBufferOperationAction) but I won't commit to say how everybody in the SDK should handle IStructuredSelection, ITextSelection and its combinations. HTH Dani
Dani, the adapter aproach won't work for our case of structured selection, because the the elements don't have ressources associated with them, if that's what you are saying. And, in one of our combined structured and text selection objects, it is not the same thing to ask the whole object for its slected text, as it is to ask each of the elements for its selected text. Besides being conceptually incorrect, I could imagine that have many interpretations. A small example might help. <tag1 a1="a1"/> <tag2 a2="a2"/> Here, if the selection starts just a character or two before the 'a1', and goes all the way to the end, after the last '/>', then we'd report there were three elements selected (tag1 and tag1 elements and the white space between them. So, asking each of those three elements what their "text" was namely <tag1 a1="a1"/> <tag2 a2="a2"/> would not be the same as asking the one selection-instance what its selected text was, namely a1="a1"/> <tag2 a2="a2"/> I hoped that helped.
Just to confirm: your case would work if FileBufferOperationAction would change: if (selection instanceof IStructuredSelection) { get resources from structured selection } else if (selection instanceof ITextSelection) { get resource from active editor's editor input } into: if (selection instanceof IStructuredSelection) { get resources from structured selection } if (selection instanceof ITextSelection) { get resource from active editor's editor input }
Waiting for confirmation.
Yes, it sounds like the psuedo-code mentioned in comment #3 and comment #5. The only difference is the part where after the first if statement, you might want to check if you already have a resource before going into the next if statement. It's a matter of who do you want to win. First one who has a resource or last one. (Hopefully, only one should have resource)
Does it matter for your case? If both report the same resource it's not a problem since we collect them in a set.
No, it doesn't matter in our case since our IStructuredSelection doesn't return a resource.
Fixed in HEAD. Available in builds > N20060112-0010.
verified this is working in eclipse-SDK-I20060208-0848-win32
Thanks Amy.
