Bug 255619 - [resolver][external tools] External tools variable ${selected_text} strips double quotes, whitespace.
Summary: [resolver][external tools] External tools variable ${selected_text} strips do...
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.4.2   Edit
Hardware: Macintosh Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Debug-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
: 172472 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-11-18 05:25 EST by Mark Thomson CLA
Modified: 2019-11-14 02:22 EST (History)
4 users (show)

See Also:


Attachments
proposed patch v1 - patches SelectedTextResolver.java (1.75 KB, patch)
2010-12-31 05:23 EST, Andre Berg CLA
no flags Details | Diff
proposed patch v1 - patches DebugPlugin.java (1.65 KB, patch)
2010-12-31 05:26 EST, Andre Berg CLA
no flags Details | Diff
proposted patch v2 - slightly refactored (995 bytes, patch)
2011-01-04 16:24 EST, Pawel Piech CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Thomson CLA 2008-11-18 05:25:13 EST
Steps To Reproduce:
1. Create a simple perl/php/whatever script that echo's back command line arguments. eg: <?php print_r($argv); ?>
2. Create an 'external tool' to call this script from Eclipse.
3. On first try, simply enter the script argument as follows: 

This is a "test"

Upon execution, your external script (tool) will echo back the arguments you passed, without the double quotes. It's easy to escape them though as needed.

4. Change the arguments for your external script from the string above to the variable: ${selected_text}

5. Open an editor, type the example string above (with double quotes), select it and run your external script.

6. As in step 3, the script echos back the passed string. And again the double quotes don't show as they have been stripped by eclipse (or the shell?).

More information:

I realize that this could be the fault/design of the shell or script interpreter, but is it not possible to pass full blocks of code or text to external tools? Perhaps double quotes in ${selected_text} should always be escaped?

Alternate solution would be introducing a new variable like ${selected_text_as_file} which would save the selected text to a /tmp/ file and then pass the temp filename to the external tool? This would have the added bonus of also preserving whitespace.

The (potentially very usefull) ${selected_text} variable seems pretty limited as it is. Sending blocks of text/code to external tools for analysis/aggregation/saving/whatever seems like it would be very useful.

Thanks.
Comment 1 Andre Berg CLA 2010-12-28 19:59:58 EST
This is still not fixed in 3.5 ?
${selected_text} is basically useless in current state.
Comment 2 Andre Berg CLA 2010-12-28 20:37:25 EST
sorry, forgot to add: 
still present in 3.6.1
Comment 3 Pawel Piech CLA 2010-12-28 23:19:16 EST
Yes, I'm afraid it's still open.  It looks like a fairly limited-scope problem so it could be a good candidate for a community contribution.
Comment 4 Andre Berg CLA 2010-12-29 01:00:40 EST
May I ask, what do you mean by limited-scope problem?

Also, do you think this can be fixed by someone with minimal Java exposure and who doesn't have any clue about the Eclipse source code?
Comment 5 Pawel Piech CLA 2010-12-29 11:21:14 EST
(In reply to comment #4)
> May I ask, what do you mean by limited-scope problem?

Mainly that it would not require API changes.

 
> Also, do you think this can be fixed by someone with minimal Java exposure and
> who doesn't have any clue about the Eclipse source code?

No, but immunity to bad sarcasm would also helpful ;-)
Comment 6 Andre Berg CLA 2010-12-29 12:54:55 EST
I would try it... I am an Objective-C/C programmer so maybe I can get a clue... 
It's just that I don't know the API at all. Would it at least be possible to point me in the right direction API-wise? It might even be possible that the fix is as simple as properly escaping double quotes before sending it further down to become shell input.
Comment 7 Pawel Piech CLA 2010-12-29 20:03:21 EST
(In reply to comment #6)
> I would try it... I am an Objective-C/C programmer so maybe I can get a 
> clue... 

Yes, if you haven't worked with Eclipse before it can be rather daunting.  I just searched through the code a bit and set a breakpoint in org.eclipse.debug.internal.ui.stringsubstitution.SelectedTextResolver.  

It seems that quotes are stripped out by DebugPlugin.parseArguments(), which uses quotes to escape spaces in arguments.  To solve your problem we could escape quotes in selected_text as you suggest and you could enclose the ${selected_text} variable in quotes in the arguments text field.
Comment 8 Andre Berg CLA 2010-12-29 20:28:26 EST
Thanks for the pointers. It is night here now, but tomorrow I should have some time on my hands so let's see how far I can get. :)
Comment 9 Andre Berg CLA 2010-12-30 21:30:02 EST
Wow, it is dead easy to check out eclipse bundles and play around with them using Import Plugins and Fragments. Thanks to your pointers I am looking at a possible solution. I just need to play around with it a little more to confirm. The whole resolver interface is quite elegant enabling me in the future to add my own DynamicVariableResolver subclass if I needed to.
Comment 10 Andre Berg CLA 2010-12-31 05:23:25 EST
Created attachment 185923 [details]
proposed patch v1 - patches SelectedTextResolver.java

First part of proposed patch v1. Only for Mac OS X, we properly escape double quotes and the escape character in the right order.
Comment 11 Andre Berg CLA 2010-12-31 05:26:42 EST
Created attachment 185924 [details]
proposed patch v1 - patches DebugPlugin.java

proposed patch v1. Only for Mac OS X, we treat ArgumentParser.parseString() differently in that we change the part about not adding the escape character in case the next char is a qouble quote. In other words, on Mac OS X we immediatly go to the next character if a backslash is found.
Comment 12 Andre Berg CLA 2010-12-31 05:32:01 EST
I have succeeded in building a jar out of "org.eclipse.debug.ui" as well as "org.eclipse.debug.core" with changes from proposed patch v1 applied and replacing the corresponding jars in my local Eclipse installation. 

When I did that, all external tools that I initially added to rely on ${selected_text} started working again. It took a while longer to get to a solution because escaped double quotes where then read incorrectly if and only if a combination of escape character + double quote was found. I arrived at a solution once I finally wrapped my head around what ArgumentParser.parseString() was doing.
Comment 13 Pawel Piech CLA 2011-01-04 16:24:30 EST
Created attachment 186050 [details]
proposted patch v2 - slightly refactored

(In reply to comment #12)
> I have succeeded in building a jar out of "org.eclipse.debug.ui" as well as
> "org.eclipse.debug.core" with changes from proposed patch v1 applied and
> replacing the corresponding jars in my local Eclipse installation. 
> 
> When I did that, all external tools that I initially added to rely on
> ${selected_text} started working again. It took a while longer to get to a
> solution because escaped double quotes where then read incorrectly if and only
> if a combination of escape character + double quote was found. I arrived at a
> solution once I finally wrapped my head around what
> ArgumentParser.parseString() was doing.

Thank you Andre, I'm very glad you were able to get set up and produce this patch.  Unfortunately, I'm still not 100% sure what the changes in ArgumentParser.parseString() are doing for you:

As far as I can tell, the standard behavior is to only un-escape double quotes from argument strings.  I'm don't know why this is, but your change is to remove this and un-escape all characters. This would mean that the treatment of quotes is the same, but all other special characters (\n, \t, \\) would be treated differently.  Is this right?  Also, why is this special to Mac OS?  I would think that linux would behave in the same way.

Thanks,
Pawel
Comment 14 Andre Berg CLA 2011-01-04 16:52:27 EST
My initial idea was to handle escaping once and for all in SelectedTextResolver. 

However I soon found out this has one drawback: The combination of backslash+doublequote ends up being doublequote. And it was only this combination that was affected. 

For example backslash+backslash+doublequote was fine, or any other combination of double or single quotes, double-escaped, tripple-escaped or not escaped at all. I was wondering, why is it only producing a single backslash when it encounters backslash+doublequote. 

After a lot of testing with a lot of different combinations of rare characters that are escaped to arbitrary levels (levels = double escape, triple escape, etc) I finally realized It must be something that is out of my control in SelectedTextResolver and further down the execution stack. 

I then took a look that ArgumentParser.parseString() and saw it was operating on single characters per while loop pass and it does something special if the nextChar is a double quote, so this must be why the drawback mentioned above must be happening. I then concluded that I am already taking care of escaping in the proper order (order is important: backslashes must be escaped first, then double quotes) in SelectedTextResolver, so my strategy for ArgumentParser.parseString() should be that I don't do anything much here other than to add the characters one by one to the parsed string that's produced as output. 

I restricted this change to Mac OS X only, because I don't have a clue about the surrounding API, I am essentially operating under a fog of war so to speak, and I didn't want to introduce a regression for other systems. 

But you are right nonetheless, Linux and Mac OS X shell is very similar so it could in theory apply there as well. 

I also tried to think of a lot of cases to test, you know from normal to wierd stuff that could be passed as selected text, but if you can think of some more I will be happy to give it a shot. 

Finally, I was also worried about whether now call stacks that involve ArgumentParser.parseToken() are somehow affected negatively, so that, in essence dynamic variables with arguments don't work anymore, but this proved to be wrong. They continue to work fine for me.
Comment 15 Pawel Piech CLA 2011-01-04 17:59:34 EST
I see, I didn't realize that the patches were separate, and I only looked at the second one.

> Finally, I was also worried about whether now call stacks that involve
> ArgumentParser.parseToken() are somehow affected negatively, so that, in
> essence dynamic variables with arguments don't work anymore, but this proved to
> be wrong. They continue to work fine for me.

This is what I'd be most concerned about.  Changing behavior in ArgumentParser will affect any variable plus anything that user types in in the argument section of the launch configuration.  I tracked down the explanation of the existing logic to bug 77295.

I'm also trying to understand your use case.  If the SelectedTextResolver escapes quotes that were in the selected text, wouldn't you want the ArgumentParser to un-escape them before they're passed to your external tool.  I.e. if you select some text in the editor, then you want that whole string to be passed to your tool.  To accomplish that, you can put the quotes in the argument text field in the launch config:

whatever "${selected_text}"
Comment 16 Andre Berg CLA 2011-01-04 18:25:06 EST
Exactly that is what I am trying to fix. The intended behaviour that you mention that is also given as hint below the argument field in the launch/external tool config is broken on Mac OS X.

That is,

the 

whatever "${selected_text}" 

case as value in the argument field is broken on Mac OS X. 

It arrived at a tool such as /bin/echo as 

whatever ${selected_text} 

with additionally any quotes that the selected text itself contained as well as sometimes whitespace stripped (depending on the shell tool the argument field value is passed to).

So for example if I select the following text in a Pydev editor (hence a Python example):

print("this is a \"%s\"" % "string")

and try to pass that to /bin/echo with the argument field in the external tool config given as simply

"${selected_text}"

/bin/echo will actually output the following to the console:

print(this is a \%s\ % string)

The expected output would be 

print("this is a \"%s\"" % "string")

No matter the form of escaping strategy employed inside the argument text field, I could never get selected text in the editor that contained double quotes to work with external tools.

Please also note my post at stackoverflow: http://stackoverflow.com/questions/4550796/how-to-get-around-eclipse-swallowing-double-quotes-for-selected-text that prompted my excursion into the bug tracker.

So far I have been continuing to use my custom launch configs with ${resource_loc:<a path>} and similar use cases for dynamic variables, I have been able create inline scripts inside the argument field that are passed to /bin/bash -c and I have been able to use ${selected_text} wrapped in double quotes in the argument field (as per the hint below it) and no matter the actual contents of the selected text it continues to work for me.

But I understand completely that it may be premature or difficult to accept a patch from somone who wasn't had a lot of API exposure with Eclipse. So I guess it becomes a "use as you see or your team see fit" case.
Comment 17 Andre Berg CLA 2011-01-04 18:35:01 EST
Thanks again for the bug cross-reference. Your insight proves invaluable. 

Reading the arguments about unification of ArgumentParser for launch configs and external tools (under the linked https://bugs.eclipse.org/bugs/show_bug.cgi?id=77295) should at least prompt another round of testing for uninteded side-effects to my launch configs. I'll see that I can get to it tomorrow afternoon when I'm home from work.
Comment 18 Andre Berg CLA 2011-01-04 18:38:44 EST
One last thing (before I go to bed :))... keep in mind that in my lingo I may mix up launch configs and external tool configs as from my end-user perspective those are somewhat connected and similar. But it could very well be that those are different beasts, API wise, that is.
Comment 19 Pawel Piech CLA 2011-01-04 20:03:35 EST
I see what you mean, but I don't have clean a solution.
Comment 20 Michael Rennie CLA 2011-06-06 13:10:42 EDT
*** Bug 172472 has been marked as a duplicate of this bug. ***
Comment 21 Lars Vogel CLA 2019-11-14 02:22:02 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

If the bug is still relevant, please remove the "stalebug" whiteboard tag.